Tuesday, November 07, 2006

Code reuse, next level

The code for this article is written in C# (tested in VS2005)

I'm still catching up with my blog reading; a few days ago I read an article on latindevelopers: sorting objects with IComparer (code in English, comments and article in Spanish); it reminded me of a common mistake: the use of conditional statements in (not so obvious) loops (e.g. callbacks) which results in a type of (not so obvious) code duplication.

In Delphi this error is commonly seen in component development, specifically on the Paint event where they use if statements to decide which font to use, colors, etc, and what's worst, many times an instance (of whatever objects are required) is created on each iteration; all those objects, of course, don't change unless you change properties of the component!!, so those should be cached and kept on variables where they can just be reused; yes, code reuse doesn't just mean re-using classes, or refactoring code into methods, caching objects is a form of code reuse because it avoids conditional statements and places code calls directly.

Let's see the code from the original example, specifically the PersonComparer class, it has a constructor which takes the SortField and stores that on a variable that is later used on the Compare method

public PersonComparer(SortField sortField) {
this.sortField = sortField;
}
public int Compare(object x, object y) {
if (!((x is Person) & (y is Person))) {
throw new
ArgumentException("Los objetos deben ser de tipo Person.");
}

switch (sortField) {
case SortField.Name:
return
Comparer.DefaultInvariant.Compare(
((Person)x).Name, ((Person)y).Name);

case SortField.LastName:
return
Comparer.DefaultInvariant.Compare(
((Person)x).LastName, ((Person)y).LastName);

case SortField.Age:
return
((Person)x).Age - ((Person)y).Age;

default:
throw new
ArgumentException("Tipo de ordenamiento desconocido.");
}
}

Perhaps you don't see it very clear because the example doesn't use generics, let's see what we could do using generics (there is a TimeZone2, so why can't I have my PersonComparer2?):

    public class PersonComparer2<T> : System.Collections.Generic.IComparer<T> where T:Person {
delegate int ComparerDelegate(T x, T y);
ComparerDelegate comparerDelegate;

Now we have a delegate declaration, a variable of that type and we got rid of the SortField variable, let's see the constructor:

public PersonComparer2(SortField sortField) {
switch (sortField) {
case SortField.Name:
comparerDelegate = delegate(T x, T y) {
return x.Name.CompareTo(y.Name);
};
break;
case SortField.LastName:
comparerDelegate = delegate(T x, T y) {
return x.LastName.CompareTo(y.LastName);
};
break;
case SortField.Age:
comparerDelegate = delegate(T x, T y) {
return x.Age - y.Age;
};
break;
default:
throw new
ArgumentException("unknown sort method.");
}
}

Instead of caching the SortField variable, I'm caching the method that will be executed when the comparison takes place; finally, let's see the actual Compare method:

public int Compare(T x, T y) {
return comparerDelegate(x, y);
}

do you see it? the decision of what method needs to be used for the comparison gets executed only once (on the constructor), then we can call it directly, instead of asking the same question over and over again on each iteration.

Every time you see a conditional statement on a loop or a method that gets executed in some kind of loop (like paint events, callbacks, etc) consider using this technique, it will help you build more structured and faster code, also consider using arrays or dictionaries of methods.

The concept applies to programming in general, but the implementation may vary from language to language, the main idea is to cache objects / methods in some way so you don't have to repeat conditional statements to create objects or decide which code block to execute.

The full code for this example can be found here, as always, play with it, learn from it, improve it.
kick it on DotNetKicks.com

3 comments:

Leif Wickland said...

What you're describing is essentially the State design pattern.

Anonymous said...

Really elegant!!.

This did lead me to speculate further, if there are possiblities of removing the switch statement chain completely.
Instead of passing the sortfield parameter to get to the delegate, how about passing the delegate directly to the constructor of the comparer.

The caller would pass the related sorter delgate created specifially for a sort type

public PersonComparer2(ComparerDelegate Sorter)
{
this.ComparerDelegate = Sorter;
}

BlackTigerX said...

you can, but are you saving your self any work? or are you giving your self more work?
are you making your code more tightly coupled?
having the parameter take care of the decision for you in the constructor makes it nice;
there are other choices in case there is need to optimize that for whatever reason

for example you could use an array of methods to accomplish that

http://ebersys.blogspot.com/2006/08/arrays-of-methods-in-c.html

it depends on the situation, if it's used inside a loop, etc