I’m a big believer in learning the fundamentals of coding and design. To me, things like design patterns, design principles, and code smells are more important to learn than specific technologies. To paraphrase a conversation I had yesterday, I think it’s vastly more important for a newbie develeloper to understand OO principles before memorizing the inner workings of ADO.Net. It can seem like a bunch of academic hotair, but over the last couple weeks my group has come across some cases where a knowledge or ignorance of these little fundamental things has had a real impact on our success that I thought I’d share. Here you’ll find one principle, one code smell, and a dash of ancient wisdom that we need to occasionally relearn.
I think I’d be a ringer at a software design version of Trivial Pursuit, but rattling off the names for things like the Liskov Substitution Principle isn’t as important as internalizing that principle to the point that it’s just something you do.
Liskov Substitution Principle
As interpreted by me, the Liskov Substitution Principle states that if a method or class has a dependency on an abstract type it should be able to accept and work with any implementation of that abstraction without changing the original code. The official wording is:
Let q(x) be a property provable about objects x of type T. Then q(y) should be true for objects y of type S where S is a subtype of T.
That doesn’t do that much for me, so how about “downcasting from an abstraction is a code smell.”
To explain this principle let’s look at a violation from my current project that’s just begging for a fix. We use a third party grid control (not Telerik) in an unbound mode for maximum flexibility of display and rendering. Before the grid is displayed, we create an “ICell” object for each cell in the grid that models the value and formatting of each cell. When the grid starts to draw any cell for the first time it raises an event requesting information for the row and column coordinate. One of the arguments to the event handler is an object that contains all of the attributes of the cell display like the following:
public class CellFormatArgs : EventArgs
{public string Contents;
public Color BackgroundColor;
public Color FontColor;
}
Our event handler simply has to find the correct ICell for the requested row and column, then set the properties on the CellFormatArgs object appropriately. Fine and dandy, but the code that does that work looks like this:
// The ICell implementations are just data holders in // this first versionpublic interface ICell
{}
public class CellDouble
{public double Value;
}
public class CellRate : CellDouble
{}
public class CellPrice : CellDouble
{}
public void RenderCell(int rowIndex, int columnIndex, CellFormatArgs args)
{ ICell cell = fetchCell(rowIndex, columnIndex);if (cell is CellPrice)
{ // render the price cell}
else if (cell is CellRate)
{ // render the rate cell}
else if (cell is CellDouble)
{ // render the double Cell}
}
Pay particular attention to the RenderCell method. It can’t just accept any old version of an ICell and go with it. The RenderCell method has to know exactly which type of ICell it is then act accordingly. If you need to add a new type of ICell you’ll have to also change the RenderCell, violating the Open/Closed Principle. When we modify our code to comply with the Liskov Substitution Principle, we’ll get this:
public interface ICell
{void Format(CellFormatArgs args);
}
public class CellDouble : ICell
{public double Value;
public void Format(CellFormatArgs args)
{args.Contents = Value.ToString();
if (Value < 0) { args.FontColor = Color.Red;}
}
}
public void RenderCell(int rowIndex, int columnIndex, CellFormatArgs args)
{ ICell cell = fetchCell(rowIndex, columnIndex);cell.Format(args);
}
We’ve made the ICell interface implementations responsible for knowing how to render and format themselves. All RenderCell() has to do is find the right ICell and call the Format() method on it. RenderCell() no longer has to have any knowledge whatsoever of what exactly each ICell is. We can now add new ICell types without touching the RenderCell() method, putting us back into conformity with the Open/Closed Principle. I’d also argue that RenderCell() is easier to understand, and the formatting rules for each ICell type is easier to understand because the logic is in the context of each ICell instead of being in the middle of a long method containing all of the formatting rules. Our formatting rules also got easier to test here as well by pulling the formatting of the CellFormatArgs away from the third party grid’s event handler. We’ve improved coupling from RenderCell() to the ICell implementations. We’ve improved the cohesion of our classes by putting the formatting responsibilities into the ICell implementations. Now all of our rules and logic for formatting rates are completely contained inside the CellRate class instead of scattered over the code.
In general, I’d say that any downcasting from an abstract type to a subclass or specific implementation is a code smell. There may be a perfectly justifiable reason to downcast in your code, but when you see it creeping in you should assume that it’s “guilty until proven innocent.” As a general rule, you want the interfaces and contracts of your classes and services to be taked completely at face value. Everything you need to use the class or service should be evident or visible through the contract. If you need to take into account the internal workings behind the interface or contract, you’ve got a nasty form of tight coupling.
Why is there a fancy name for this? Isn’t this just common sense engineering and polymorphism? Well yes I guess it should be common sense, except that it’s commonly violated to negative effect. Besides, if you had a fundamental principle of software design named after yourself, wouldn’t you want people to make over it just a bit instead of blowing it off?
Shotgun Surgery
Shotgun Surgery is one of the Code Smells from Martin Fowler’s Refactoring book. Have you ever found yourself in a situation where changing one piece of code always forces changes to one or more other pieces of code? That’s doing Shotgun Surgery. What the smell tells you is that all of those pieces that have to change together should probably be unified into a single entity so that changes can be streamlined. It’s following the DRY Principle so you can make changes with fewer mechanical steps.
My group hit something like this last week. We have an enumeration that basically identifies every possible type of financial number in the system. In another namespace altogether is a class that keeps a string description for each unique value of the enumeration for screen display. Every time we add a new value to the enumeration we have to remember to go over to this other class and add a new string to the other string array or bad things will happen when the client runs. Our eventual fix is to create a small new class to represent the enumeration values that will also contain the description. New enumeration values will then be completely defined in one place.
Optimize by Measurement
So you know how for your entire career you’ve heard that the best way to optimize performance is to measure the performance first because the hotspots may not be where you think they are? And you’ll be better off in terms of effort spent to deal with the hotspots before anything else? I lived that little exhortation the other day. I have a screen that was far too slow a couple days ago and knew I was going to spend some time optimizing. While I was stuck in a useless meeting I started jotting down ideas to eliminate some fat out of my calculation and aggregation code that would have required some significant changes. Once I got back to my desk I ran the JetBrains profiler on the screen just to get some benchmarks. Unsurprisingly to the experienced folks out there, the biggest performance problem was something completely unexpected. The entire set of calculations and expensive grid rendering was performed twice inside the twisty event rippling of the screen. Thirty minutes later I got the double screen rendering solved and the screen was much more responsive — without so much as touching the calculation code.
Lesson learned. Pay attention to the older folks.
