Thursday, December 28, 2006

Putting methods in the correct place

Object-orientated programming is all about encapsulation - the principal whereby an object exposes only what is needed by other objects and, by doing so, remains in control of its state.
For example, your Collection class exposes methods to add, retrieve and remove items, as well as a Count property, which indicates how many items it contains.


In real life, however, the concept of an Item or a Collection are way too abstract. For example, you might have a collection of stamps, trains, pc components, mobile phones, screw drivers, or you may have a list of things to do, telephone numbers or groceries to buy (a shopping list!). The terms "collection" and "list" need qualifying - what is it a collection of? What does the list list?
In your program, you no longer have a collection, you have a Pupil Collection (first example that came to mind). This is exposed as a property of your Class class.

You've got this far, and then you find you need to check if a Pupil is in a Class, so you write code like this:
public class Class
{
    public PupilCollection Attendies;
    //other methods
    public bool ContainsPupil ( Pupil pupil )
{
for ( int i = 0; i < Attendies.Count; ++i )
{
if ( Attendies.GetPupil(i) == pupil )
return true;
}
return false;
}
}
Then, a few days later, you find another class needs to see if a Pupil Collection contains a Pupil. So, you write that same method again. And then, in another few days, you find another class that needs to do the same thing. In fact, checking whether a Pupil is in a Pupil Collection is a really common thing.
So, what could you have done differently?
How about if, instead of having the Class iterate through the Pupil Collection, have the Pupil Collection iterate through itself, like this:
public abstract class PupilCollection
{
    public bool ContainsPupil (Pupil pupil )
{
for ( int i = 0; i < pupils.Count; ++i )
{
if ( pupils[i] == pupil )
return true;
}
        return false;
}
}
public class Class
{
    public PupilCollection Attendies;
    //other methods
    public bool ContainsPupil ( Pupil pupil )
{
        return Attendies.ContainsPupil ( pupil );
    }
}

And the next time you need to see if a Pupil Collection contains a Pupil, the code's already there. Your code looses a whole load of duplication, and reads nicer, simply because you moved a method.
In this case, we can start to think about things like specifications [pdf], and other more intricate designs, because we no longer look at the code for the Class and have to think about how it's doing what it's doing.

Using tools (and Code Smells)
I've been using Resharper for the past two years, and I can hardly use Visual Studio without it (well, I can, but I always get frustrated that + and ++R do nothing).

Its Extract Method option is probably the one I use most. Not only does this decide what to have as parameters for the method, but the majority of the time, it's "Declare Static" box is checked too - meaning that the method it will create will be a class level method.

When it does this, I take it as a sign to assess whether this newly created method wouldn't be better off somewhere else. Sometimes it doesn't make sense to move it, but often, just thinking about what the new method does means I put it somewhere more appropriate, and I find my code reads a lot better for it.

So, next time you see a static/class level method, ask yourself if it doesn't belong elsewhere.

No comments: