Sunday, October 25, 2015

9 simple practices for writing better object-oriented code

Consider a fantasy game that must track a collection of items, each having a certain amount of quality (or value) that increases or decreases after time passes. This collection contains the following six items:


  •  +5 Dexterity Vest
  • Aged Brie
  • Elixir of the Mongoose,
  • Sulfuras, Hand of Ragnaros
  • Backstage passes to a TAFKAL80ETC concert
  • Conjured Mana Cake


How much each item's quality increases or decreases over time depends on an intricate algorithm that involves the quality and sell-in (days) properties of each item. Fortunately a prior C# developer encoded this for us. Let's see what this algorithm looks like:

private static void UpdateQuality(Item item)
{
    if (item.Name != "Aged Brie" && item.Name != "Backstage passes to a TAFKAL80ETC concert")
    {
        if (item.Quality > 0)
        {
            if (item.Name != "Sulfuras, Hand of Ragnaros")
            {
                item.Quality = item.Quality - 1;
            }
        }
    }
    else
    {
        if (item.Quality < 50)
        {
            item.Quality = item.Quality + 1;

            if (item.Name.Equals("Backstage passes to a TAFKAL80ETC concert"))
            {
                if (item.SellIn < 11)
                {
                    if (item.Quality < 50)
                    {
                        item.Quality = item.Quality + 1;
                    }
                }

                if (item.SellIn < 6)
                {
                    if (item.Quality < 50)
                    {
                        item.Quality = item.Quality + 1;
                    }
                }
            }
        }
    }

    if (!item.Name.Equals("Sulfuras, Hand of Ragnaros"))
    {
        item.SellIn = item.SellIn - 1;
    }

    if (item.SellIn < 0)
    {
        if (item.Name != "Aged Brie")
        {
            if (item.Name != "Backstage passes to a TAFKAL80ETC concert")
            {
                if (item.Quality > 0)
                {
                    if (item.Name != "Sulfuras, Hand of Ragnaros")
                    {
                        item.Quality = item.Quality - 1;
                    }
                }
            }
            else
            {
                item.Quality = item.Quality - item.Quality;
            }
        }
        else
        {
            if (item.Quality < 50)
            {
                item.Quality = item.Quality + 1;
            }
        }
    }
}

So? What do you think? If you want, you can find the full sources of this version here. Is it readable enough for you?  I hope not, because that would render this entire article useless. So let's use this opportunity to introduce the first practice: use only one level of indention per method. In other words, a method can have an if statement as long as it is not nested in another construct, other than the class member itself. Obviously the code snippet above violates this practice big time, so let's rewrite the algorithm.

private static void UpdateQuality(Item item)
{
if ((item.Name != "Aged Brie") && (item.Name != "Backstage passes to a TAFKAL80ETC concert"))
{
ReduceQualityUnlessNameIsSulfuras(item);
}
else
{
HandleInsufficientQuality(item);
}

if (!item.Name.Equals("Sulfuras, Hand of Ragnaros"))
{
item.SellIn = item.SellIn - 1;
}

if (item.SellIn < 0)
{
HandleInsufficientSellIn(item);
}
}

private static void ReduceQualityUnlessNameIsSulfuras(Item item)
{
if ((item.Quality > 0) && (item.Name != "Sulfuras, Hand of Ragnaros"))
{
item.Quality = item.Quality - 1;
}
}

private static void HandleInsufficientQuality(Item item)
{
if (item.Quality < 50)
{
item.Quality = item.Quality + 1;

HandleBackstagePasses(item);
}
}

private static void HandleBackstagePasses(Item item)
{
if (item.Name.Equals("Backstage passes to a TAFKAL80ETC concert"))
{
UpdateQualityBasedOnQualityAndSellIn(item);
}
}

private static void UpdateQualityBasedOnQualityAndSellIn(Item item)
{
if ((item.SellIn < 11) && (item.Quality < 50))
{
item.Quality = item.Quality + 1;
}

if ((item.SellIn < 6) && (item.Quality < 50))
{
item.Quality = item.Quality + 1;
}
}

private static void HandleInsufficientSellIn(Item item)
{
if (item.Name != "Aged Brie")
{
HandleItemsThatAreNotAgedBrie(item);
}
else
{
HandleOtherItems(item);
}
}

private static void HandleItemsThatAreNotAgedBrie(Item item)
{
if (item.Name != "Backstage passes to a TAFKAL80ETC concert")
{
IfNotSulfurasAndQualityIsSufficient(item);
}
else
{
item.Quality = item.Quality - item.Quality;
}
}

private static void IfNotSulfurasAndQualityIsSufficient(Item item)
{
if ((item.Quality > 0) && (item.Name != "Sulfuras, Hand of Ragnaros"))
{
item.Quality = item.Quality - 1;
}
}

private static void HandleOtherItems(Item item)
{
if (item.Quality < 50)
{
item.Quality = item.Quality + 1;
}
}

I tried to replace all the nested constructs with a simple Extract Method refactoring pattern. Did it help? I suspect you've discovered by now that backstage passes increase in value the closer you get to their end-of-life. You may also have noticed that Sulfarus never looses its value and that the value of an item never exceeds 50. But we can do better. Let's introduce the practices Don't Use The Else Keyword and Use Only Dot Per Line.

Most of the dots in the code snippet above are caused by the reference to the item variable, which is a sign of Feature Envy code. Similarly, you could say most of the elses are caused by the procedural nature of this code example. And what else could be the the best solution to this than introducing a class per item? This is the revised implementation for the Elixer of the Mongoose item. 

public class ElixirOfTheMongoose : Item
{
    public ElixirOfTheMongoose(int sellIn, int quality)
        : base("Elixir of the Mongoose", sellIn, quality)
    {
    }

    public override void UpdateQuality()
    {
        if (Quality > 0)
        {
            Quality = Quality - 1;
        }

        SellIn = SellIn - 1;

        if (SellIn < 0 && Quality > 0)
        {
            Quality = Quality - 1;
        }
    }
}

And now the algorithm is clear; each day the quality decreases gradually, but until the end-of-life has been reached, after which the quality decreases twice as fast. You could say that these little practices force you to consider a more object-oriented design, don't you agree? But, if you take a closer look you'll notice something else. Apparently, the quality can never become negative.

You could have solved this by using an unsigned int, but by applying the Wrap All Primitives and Strings practice you can do better. By wrapping the two numeric properties of the Item base-class in classes with value-type semantics such as overloads for Equals, GetHashCode, various equality operators and by adding intention revealing members like Decrease, IsOverdue and Decrement, you can rewrite the UpdateQuality method as:

public override void UpdateQuality()
{
    Quality = Quality.Decrease();

    SellInDays = SellInDays.Decrement();

    if (SellInDays.IsOverdue)
    {
        Quality = Quality.Decrease();
    }
}

We're left with four lines of code. Few enough to almost instantaneously understand the algorithm. But we're not done yet. Suppose we want to extract some characteristics from a collection of items such as the highest value item as well as the items that are overdue. We could take that List from the GildedRose class and do the following.

Item highestValueItem = items.OrderBy(i => i.Quality).Last();
IEnumerable itemsOverdue = items.Where(i => i.SellInDays.IsOverdue);

But then we're not only violating Use Only Dot Per Line again. The mere fact that we use a simple List is violation of another practice, which is the one named First Class Collections. And now that I think of it, how would one call a collection of items that need to be sold? Ah, what about an inventory. So let's create the inventory class.

public class Inventory : IEnumerable
{
private readonly HashSet items = new HashSet();

public Item HighestValued
{
get { return items.OrderBy(i => i.Quality).Last(); }
}

public IEnumerable Overdue
{
get { return items.Where(i => i.SellInDays.IsOverdue).ToArray(); }
}

public IEnumerator GetEnumerator()
{
return items.GetEnumerator();
}

IEnumerator IEnumerable.GetEnumerator()
{
return GetEnumerator();
}

public void Add(Item item)
{
items.Add(item);
}

public void HandleDayChanges(int nrOfDays)
{
for (int i = 0; i < nrOfDays; i++)
{
foreach (var item in items)
{
item.HandleDayChange();
}
}
}
}

Which means we can rewrite the original two calls like this:

Item highestValueItem = inventory.HighestValued;
IEnumerable itemsOverdue = inventory.OverDue;

Wrapping simple collections in first-class types is actually a very smart thing to do. How many times have your classes exploded in size with private methods which only task was to modify or query your private collection field? If you would have applied the practices I mentioned up to now, I'm pretty sure you agree you would have ended up with better distribution of responsibilities. It might feel all like overkill, but since I've learned about these practices, it has really helped me create better object-oriented code.

Another practice, aptly called Don't Abbreviate, tells us not to abbreviate names of classes, members and variables. This practice can be confusing, because the question you should ask yourself should be why you want to abbreviate names in the first place. I guess it's either because you couldn't find a shorter name to describe your class, or because you are trying to avoid repeating the same words over and over. The former usually happens to me if I'm trying to name a class that has too many responsibilities (hence the principle Single Responsibility Principle in SOLID). The latter could be, and usually is, caused by not following First Class Collections and Wrap All Primitives and Strings.

It's awesome to realize how these practices work together so well to accomplish another practice named Keep All Entities Small. Of course, small is not an objective measurement, but I highly doubt anyone would think the code example I started this post with is small. And that's the purpose of this practice. Whenever your class turns into anything but small, start applying the previous practices to make it small again. One heuristic that can help you detect this as early as possible is to practice No Classes With More Than Two Instance Variables. I know that is going to be difficult to meet. Hey, you might even call it unrealistic and overkill. Regardless, I challenge you to consider the other practices whenever you pass the first two instance fields.

Now consider the following implementation of the Item class.

using System;

public abstract class Item
{
private readonly string name;

protected Item(string name, SellInDays sellIn, Quality quality)
{
this.name = name;
Quality = quality;
SellInDays = sellIn;
}

public SellInDays SellInDays { get; set; }

public Quality Quality { get; set; }

public string Name
{
get { return name; }
}

public int DaysOverdue
{
get { return SellInDays.DaysOverdue; }
}

public abstract void HandleDayChange();

public override string ToString()
{
return name;
}
}

Technically, this class doesn't have any fields (instance variables in C#), but the properties used here are just compiler-generated getters and setters around a hidden field. So in reality this class violates the two instance practice with one field. But do we need this getters and setters at all? If you look closely at the code, you'll see that those properties are either used for sorting or filtering as illustrated in the previous practice, or to compare them in unit tests. Quite often, properties open up the possibility for calling code to apply algorithms that really belong inside the class that exposes the properties. So you could say that they are generally not a favorable solution, hence the No Getters/Setters/Properties practice.

In our case, this theory is actually true. For instance, two items are semantically equivalent if their Quality, SellinDays and Name are the same, which is what the unit tests rely on. We can easily replace that behavior by the correct implementation of Equals. Likewise, sorting items based on the quality of these properties can be replaced by exposing an implementation of IComparer. The only one that we can't really remove is the DaysOverdue property. And I don't mind too much either. Being able to ask an item how many days it's due sounds like something you should be able to ask an item in this domain. Anyway, this the current implementation, although I'm discovering refactoring opportunities every time I look at the code.

public abstract class Item
{
private readonly string name;
private Days shelfLife;
private Quality quality;

protected Item(string name, Days shelfLife, Quality quality)
{
this.name = name;
this.quality = quality;
this.shelfLife = shelfLife;
}

public bool IsExpired
{
get { return shelfLife < new Days(0); }
}

public Days DaysOverdue
{
get { return (shelfLife > new Days(0)) ? new Days(0) : -shelfLife; }
}

public static IComparer ByQualityComparer
{
get { return new QualityComparer(); }
}

public abstract void OnDayHasPassed();

public override string ToString()
{
return string.Format("{0} (quality {1}, sell in {2} days)", name, quality, shelfLife);
}

protected bool Equals(Item other)
{
return shelfLife.Equals(other.shelfLife) && string.Equals(name, other.name) && quality.Equals(other.quality);
}

public override bool Equals(object obj)
{
if (ReferenceEquals(null, obj))
{
return false;
}

if (ReferenceEquals(this, obj))
{
return true;
}

return Equals((Item)obj);
}

public override int GetHashCode()
{
unchecked
{
int hashCode = shelfLife.GetHashCode();
hashCode = (hashCode * 397) ^ name.GetHashCode();
hashCode = (hashCode * 397) ^ quality.GetHashCode();
return hashCode;
}
}

protected void IncreaseQuality()
{
quality = quality.Increase();
}

protected void DecreaseQuality()
{
quality = quality.Decrease();
}

protected void Devaluate()
{
quality = new Quality(0);
}

protected void ReduceShelfLife()
{
shelfLife = shelfLife.ReduceByOneDay();
}

protected bool IsDueWithin(Days days)
{
return shelfLife < days;
}

private class QualityComparer : IComparer
{
public int Compare(Item x, Item y)
{
return x.quality.CompareTo(y.quality);
}
}
}


So what do you think? Do these practices, collectively called Object Calisthenics, make sense? Did I motivate you to try them right away? If so, tell me your success stories by participating in the Disqussions below. And please follow me at @ddoomen to get regular updates on my everlasting quest for better solutions. 

Friday, October 09, 2015

The real challenges of building the right thing right

Suppose you're in the following situation.

  • You and your team are supposed to start working on some important architectural changes that were long due.
  • Your team is then asked by your manager to help build an important feature that requires the unique skills of your team, and which you think will take 5-6 weeks to build plus two weeks to support quality assurance. That feature is the final check in the box for the company to send a big fat invoice.
  • Accepting the task will mean that those architectural improvements will get postponed even more.

Now suppose you do understand the need for this and know management has no other options, you agree, and commit for eight weeks. Over those 6 weeks of development, you make a lot of little technical decisions, some good, some bad. For instance, introducing a couple of HTTP APIs on top of ASP.NET controllers rather than using a proper OWIN pipeline. Or struggling with a client-side control and the right framework that fits with it, resulting in JavaScript code that’s difficult to test with Jasmine.

After delivering the feature, it's time to focus on those architectural improvements again. But wait, should we not first repair those bad decisions we made during the last 6 weeks? You wonder, what's more important? Well, those architectural improvements will open up all kinds of significant opportunities that will improve the performance and scalability of the system. On the other hand, using an OWIN pipeline for those HTTP APIs serves as an example for other teams to build more efficient code. Then let's ignore that untested JavaScript for now. But you can’t really do that either. You yourself have been instructing teams to design all JavaScript code in a testable fashion. "So what", you think? You had a deadline, didn’t you? But, do you remember what you told that team when they had the same excuse?

Although this is a bit of a contrived scenario, it is a situation I'm pretty sure you will recognize. So how will I deal with such a situation? Well, I'd probably skip the OWIN pipeline conversion. Although it's nice to have a reference example for the other developers, I might be able to pick this up as part of a future story. Or, if the opportunity arises, I might be able to convince another developer needing similar functionality to include in his work. But refactoring the JavaScript code to make It testable is not something I would postpone. How can I ask other developers to follow those rules if I don't follow myself? I really believe in practicing what you preach and building the right thing right. So only after I've finished this I would switch my focus to the architectural changes.

So what do you think? Would you follow the same line of thought as I would? Let me know by participating in the Disqussions below. Oh and follow me at @ddoomen to get regular updates on my everlasting quest for better solutions.

Thursday, October 01, 2015

My thoughts on documenting your code

A recurring topic in every project I've been involved with is when and where you should use XML comments and/or annotate classes and members with proper documentation. I've always been very specific about this, but in real live I meet a lot of developers who don't seem to care about this, or worse, they reject the entire concept. Some of these people claim that the only real documentation is the code itself. And although I fully agree that a well-factored and properly named class is a crucial element in achieving a maintainable code base, proper documentation is just as important.



In my opinion, these three kinds of documentation are required for every professional code base.
  • Class-level and member-level documentation helps you understand what behavior to expect without having to jump into the actual implementation details. It should also explain the responsibilities and the roles the class plays in the overall design, or the member within the class. This kind of documentation is not a replacement for proper naming. Both are crucial, and one should reinforce the other. So always document the expected behavior of all your public and protected members. But, don't add documentation that repeats the name of the member. I'd rather have no documentation at all than that kind of useless crap.
     
  • Inline comments, which you should use scarcely, are there to help you understand the thought process the original developer went through, to point you in the right direction, or to emphasize a particular non-trivial design decision. I use the word scarcely purposely, because in most cases, refactoring the code can help you avoid the need to have in-line comments. For instance, wrapping a complex expression in a method which name explains the goal of the algorithm can already make your code a lot easier to understand. Robert C. Martin's book Clean Code contains numerous of these refactorings.
     
  • Commit comments should not only explain the nature of the change, but more importantly, why a particular change was needed in the first place. When a lot of people are involved, it becomes pretty important to be able to see why something that happened a long time ago happened. Yeah, an internal blog can help, but ultimately, it’s the source control history that will contain the really interesting changes. I've seen numerous occasions where I noticed some code that seem to be superfluous, but ended up being quite critical. The commit message for that change helped me understand why. Unfortunately I've also seen less helpful commit messages requiring me to figure out the reason the hard way.
So what do you think? Do you properly document your code? Does code documentation help you use an API? Or do you still jump into its implementation details? Let me know by participating in the Disqussions below. Oh and follow me at @ddoomen to get regular updates on my everlasting quest for better solutions.