Gilded Rose 6

Today I plan to move the Conjured item code to the new scheme, and see what that tells me. Something will probably go wrong.

Here we are, Wednesday according to my watch. I have almost no notion of what day it is any more, here in the slam. I’m hoping folks can prove my innocence and let me out of here before the weather gets much nicer. I miss my chai and the occasional “hi how are ya” at the bookstore coffee shop we call BAR. I even kind of miss Chet. Anyway …

If I recall where we were less than 24 hours ago, we came up with the notion of a wrapper object that we create on the fly, custom-fitted to the type of shop item we’re dealing with, and then use that wrapper to do the update for that kind of item.

What pattern is this idea, anyway? It’s a bit like Gang of Four Strategy, and a bit like Adapter. If a bunch of programmers were sitting around the campfire, talking about those, I’d probably chime in with the time I wasn’t allowed to touch a class because of a goblin so I built this wrapper thing which seems much like the patterns we’re talking about. Maybe there’s exactly one like this, but if there is, I’ve forgotten it.

In any case:

We need custom behavior for a collection of objects, depending on a property of the object. This might be thought to call for creation of a class hierarchy or at least a common interface for objects of varying types. We are constrained to use exactly these objects without changing them.

Solution: Create a wrapper object with the behavior we need for each type of object, Wrap the objects temporarily with the suitable wrapper, invoke the behavior, then discard the wrapper.

This is, of course, just one way. I am quite certain we’ll end up trying another way, mostly because I’m here to play and learn.

Quit babbling and do something

Right, right. The plan today is to convert at least the Conjured items to our new scheme. That should be “easy” because they’re handled outside the legacy update, and they have at least a few tests. We’ll review the tests first and see whether we want to add more.

I’m tempted to quote from the spec inside the test file. That tells me that the tests aren’t communicating everything they need to. Sometimes we can improve the tests, sometimes comments are really the best way. Here’s what we have now:

require "./gilded-rose"
require "minitest/autorun"

class TestGildedRose<Minitest::Test
  
  def test_hookup
    assert true
  end

  def test_foo
    items = [Item.new("foo", 0, 0)]
    GildedRose.new(items).update_quality()
    assert_equal "foo", items[0].name
  end

  def test_conjured
    gr = GildedRose.new([])
    item = Item.new("Conjured thing", sell_in=3, quality=10)
    gr.update_item(item)
    assert_equal(2, item.sell_in)
    assert_equal(8, item.quality)
  end

  def test_conjured_quality_non_negative
    gr = GildedRose.new([])
    item = Item.new("low-value conjured item", sell_in=2, quality=2)
    gr.update_item(item)
    assert_equal(1, item.sell_in)
    assert_equal(0, item.quality)
    gr.update_item(item)
    assert_equal(0, item.sell_in)
    assert_equal(0, item.quality)
  end

  def test_conjured_sell_in_negative
    gr = GildedRose.new([])
    item = Item.new("another conjured item", sell_in=0, quality=2)
    gr.update_item(item)
    assert_equal(-1, item.sell_in)
  end

  def test_conjured_double_dip_past_sell_in
    gr = GildedRose.new([])
    item = Item.new("double-dip conjured item", sell_in=0, quality=10)
    gr.update_item(item)
    assert_equal(6, item.quality)
  end

end

We could get rid of those two dummy tests at the top, they were just there to make sure we were hooked up. Byebye, dummy tests. Four runs, eight assertions, all good. Shall we commit the code? Yes, it’s a good habit to have and one that I often skimp on.

To move the conjured item to our new scheme, we need to create a conjured item wrapper, then change our wrapper selection method to select it. Now you know, and I know, that creating this new wrapper is going to duplicate some code. We probably have ideas on what to do about that, perhaps making all the wrappers inherit from some base wrapper.

When I foresee something like that, I prefer not to build toward the future that I foresee, but instead to go ahead, create the duplication, and then look at it and explore what to do about it. Making the problem more concrete helps me in two ways. First, I don’t run off implementing infrastructure before its needed, and second, I am working with concrete problems instead of imagined ones. My imagination is good, but I like to keep it real. Here goes:

In the Gilded Rose class, I just needed to upgrade the appropriateWrapperClass method:

  def appropriateWrapperClass(item)
    if match_name(item, "conjured")
      return ConjuredWrapper
    end
    return OriginalWrapper
  end

You’ll notice that I’m using a Guard Clause pattern here, not an if-elsif style. Dealer’s choice, really. We could do it as a one-liner for more clarity. I’ll try that in a moment but first let’s see what we have.

In the Wrapper file, I added a new Wrapper class:

class ConjuredWrapper
  def initialize(item)
    @item = item
  end

  def conjured_degradation
    normal_degradation = 1
    if @item.sell_in < 0
      normal_degradation = 2*normal_degradation
    end
    conjured_degradation = 2*normal_degradation
    return conjured_degradation
  end

  def update
    item = @item
    item.sell_in = item.sell_in - 1
    item.quality = [0,item.quality - conjured_degradation].max
  end

end

I changed the conjured_degradation method to reference the member variable, because we have it. And reading this now gives me a bit of a surprise.

I was expecting that each wrapper class would have an init and an update and nothing else. That seemed to me to be a lot of duplication, all those initialize methods. As it stands now before us, that init isn’t troubling me.

That’s a perfect unplanned example of why I don’t start setting up a hierarchy right away, or why I generally don’t build much infrastructure in advance of a clear need. I’m just not smart enough.

I’ve been programming in this sort of ignorant way now since Extreme Programming started, back in 1996. Wow. Long time.

I started working this way because I had the luxury of working in any experimental way that I might choose, because I’m not coding for a living, so it makes sense to code for learning. I had always been big on up front design and infrastructure, so I wanted to completely break that habit and see when I got in trouble. To my surprise, if I just kept the code clean, I seemed never to get in trouble. When the design wasn’t quite enough to support the growing application, it was easy to improve the design just enough. The designs I wound up with tended to be closer to “just right”, rather than too big, as my pie in the sky we’re gonna need everything designs often tended to be. Plus, I was able to deliver running code sooner.

I don’t believe in going back on our own time line. We might think we’re making a simple mistake correction, but that change makes us able to go to lunch a bit early, and we order the fish instead of the chicken and the girl at the table next to us doesn’t like fish so she never tells her friend about the nice guy she saw at a table near her at lunch and so she never brings her friend back to that restaurant and she doesn’t “accidentally” spill her soda on you, so you don’t wind up asking her out and next thing you know you’re a drugged out loveless shell of a man living on the street. But I digress.

All that aside, if I were condemned to live my professional life over, one thing I’d change would be to try to always have running code, in good condition, ready to be shipped, missing only the next most important features. That change, I believe, would have made my life better, and would have given a number of not-so-successful projects a fair chance to succeed.

Bottom line, I’ve developed the habit of doing simple things, then observing what the code actually is, and improving it, and it has worked just fine.

You are droning on today, aren’t you, Ron?

Well, musing. Thinking about how I work and why. Anyway, I’ve now got green tests and a new ConjuredWrapper class. Let’s commit the code and then look at what needs improvement.

How about this GuardClause style?

  def appropriateWrapperClass(item)
    if match_name(item, "conjured")
      return ConjuredWrapper
    end
    return OriginalWrapper
  end

That could be done like this, and it would be good idiomatic Ruby:

  def appropriateWrapperClass(item)
    return ConjuredWrapper if match_name(item, "conjured")
    return OriginalWrapper
  end

That’s rather nicer, I think. It makes the fact that we’re doing a Guard Clause more obvious, and it’s fewer lines of code, which makes for fewer places to make a mistake.

That method name doesn’t match our style. We’re using words separated by underbars, not the mixed-case name we have there. I’ll rename that method:

  def appropriate_wrapper_class(item)
    return ConjuredWrapper if match_name(item, "conjured")
    return OriginalWrapper
  end

  def update_item(item)
    wrapper_class = appropriate_wrapper_class(item)
    wrapped_item = wrapper_class.new(item)
    wrapped_item.update
  end

That’s better, isn’t it? Tests still run. Time to commit the code again.

Summing up

I think that’ll be enough for this morning. We’ve got our somewhat hacked handling of Conjured items set up in a new scheme, and we have a decent looking way of selecting wrapper classes.

I think that to go further, I’ll want to look at writing some decent micro tests for each type of shop item (one type at a time, of course, unless we get on a roll), then move each type to our new wrapper scheme, and adjusting the OriginalWrapper class not to handle that type of object any more.

My tentative notion for that last bit will be just to change the string constant references so that they’ll never match. I’d like to be able to refactor that code to the point where we can identify the operational code for each type, but right now that looks like it can’t be done readily.

That’s a problem. If we can refactor that hideous method to the point where each item’s handling is explicit, then we can extract the update for each item type directly, giving us a smaller chance of making a mistake. If we can’t do that, we’ll really be re-implementing the updates, looking at the spec and trying to understand the hideous code. That will be more error-prone, and we’ll feel less confident about doing our refactoring.

With real legacy code, with limited tests, vast tracts of complexity, and little time to work, confidence is important. If we’re afraid we might break it, we won’t improve it. So we’ll need to think hard about how to gain confidence in what we’re doing.

We’ll continue that process next time. See you then! Here’s all the code, and it’s on GitHub as well:

require "./gilded-rose"
require "minitest/autorun"

class TestGildedRose<Minitest::Test

  def test_conjured
    gr = GildedRose.new([])
    item = Item.new("Conjured thing", sell_in=3, quality=10)
    gr.update_item(item)
    assert_equal(2, item.sell_in)
    assert_equal(8, item.quality)
  end

  def test_conjured_quality_non_negative
    gr = GildedRose.new([])
    item = Item.new("low-value conjured item", sell_in=2, quality=2)
    gr.update_item(item)
    assert_equal(1, item.sell_in)
    assert_equal(0, item.quality)
    gr.update_item(item)
    assert_equal(0, item.sell_in)
    assert_equal(0, item.quality)
  end

  def test_conjured_sell_in_negative
    gr = GildedRose.new([])
    item = Item.new("another conjured item", sell_in=0, quality=2)
    gr.update_item(item)
    assert_equal(-1, item.sell_in)
  end

  def test_conjured_double_dip_past_sell_in
    gr = GildedRose.new([])
    item = Item.new("double-dip conjured item", sell_in=0, quality=10)
    gr.update_item(item)
    assert_equal(6, item.quality)
  end

end

# wrapper classes

class ConjuredWrapper
  def initialize(item)
    @item = item
  end

  def conjured_degradation
    normal_degradation = 1
    if @item.sell_in < 0
      normal_degradation = 2*normal_degradation
    end
    conjured_degradation = 2*normal_degradation
    return conjured_degradation
  end

  def update
    item = @item
    item.sell_in = item.sell_in - 1
    item.quality = [0,item.quality - conjured_degradation].max
  end

end

class OriginalWrapper
  def initialize(item)
    @item = item
  end

  def update
    item = @item
    if item.name != "Aged Brie" and item.name != "Backstage passes to a TAFKAL80ETC concert"
      if item.quality > 0
        if item.name != "Sulfuras, Hand of Ragnaros"
          item.quality = item.quality - 1 #1 for sword
        end
      end
    else
      if item.quality < 50
        item.quality = item.quality + 1
        if item.name == "Backstage passes to a TAFKAL80ETC concert"
          if item.sell_in < 11
            if item.quality < 50
              item.quality = item.quality + 1
            end
          end
          if item.sell_in < 6
            if item.quality < 50
              item.quality = item.quality + 1
            end
          end
        end
      end
    end
    if item.name != "Sulfuras, Hand of Ragnaros"
      item.sell_in = item.sell_in - 1
    end
    if item.sell_in < 0
      if item.name == "Aged Brie"
        if item.quality < 50
          item.quality = item.quality + 1
        end
      elsif item.name == "Backstage passes to a TAFKAL80ETC concert"
        item.quality = 0
      else
        if item.quality > 0
          if item.name != "Sulfuras, Hand of Ragnaros"
            item.quality = item.quality - 1
          end
        end
      end
    end 
  end

end

# Gilded Rose

require "./wrappers"

class GildedRose

  def initialize(items)
    @items = items
  end

  def match_name(item,name)
    return item.name.downcase.include? name.downcase
  end

  def appropriate_wrapper_class(item)
    return ConjuredWrapper if match_name(item, "conjured")
    return OriginalWrapper
  end

  def update_item(item)
    wrapper_class = appropriate_wrapper_class(item)
    wrapped_item = wrapper_class.new(item)
    wrapped_item.update
  end

  def update_quality()
    @items.each do |item|
      update_item(item)
    end
  end
end # Gilded_Rose

class Item
  attr_accessor :name, :sell_in, :quality

  def initialize(name, sell_in, quality)
    @name = name
    @sell_in = sell_in
    @quality = quality
  end

  def to_s()
    "#{@name}, #{@sell_in}, #{@quality}"
  end
end