Hello World!

“For whom am I writing?” is a question we should ask our self continuously when programming. I often see code that is hard to use, difficult to understand and next to impossible to maintain. That is a clear sign that the author of the code did not consider the readers.

The least important reader is the compiler. The compiler is pretty good at understanding what you mean, and as long as you keep within the language, it will handle it. Instead, we should write for:

• The reader of this code
• The reader of code that calls this code
• The writer of code that calls this code

Thinking of these three roles will naturally lead to many of the best practices for good code (such as those found in books like “Clean Code”). You will have to find good names and good signatures for the methods and classes you create. Also, it is seldom a good idea to be too clever in the solutions we pick. Maybe you’re able to save a few lines of code, but if it means that all that come after you have to spend hours just trying to figure out what you’re doing, it’s not good code (unless you’re writing for a code obfuscation contest…)

We write the code only once, but it is usually read many times. As developers we spend a lot more time reading code than typing in new, even if we often don’t think about it.

Today we came across a code snippet that rewritten in C# would look like this:

class InheritedClass {

  public void SetFilter(FilterBaseType filter) {
    this.filter = filter;
  }

  public override object SetNewFilterObject(object filter) {
    object oldFilter = this.filter;
    this.filter=filter as FilterBaseType;
    return object;
  }
}

As we fist only looked at the inherited class, this looked very strange. Why have the method that sets the filter without type safety, and why is the method returning the old object when setting the new? After searching through our quite large code base, we found that this method was only used in two places – in a test which could simply be replaced with the typed version, and in the base class. The base class looked something like this:

class BaseClass {
  // ...

  public void InitializeNewRound() {
    // ...
    object oldObject = SetNewFilterObject(null);
    // ... do a bunch of initialization stuff that requires there to be no filter

    setNewFilterObject(oldObject);
    // ... do some more initialization that should have the filter in place again
  }

  public virtual abstract object SetNewFilterObject(object handler) { }
}

Since the base class was in a different architectural level than the inherited class, and the filter types was all in the level of the inherited class, the base class couldn’t know about these types (architectural rules in our code base). Therefor someone has at one point found that this was a good way of achieving what they needed – to temporary disable the filter (by setting it to NULL which in this context meant no filter), and then turning it on again.

But the downside was that it took us a while to understand why it was written like this, and how the code worked. So we decided to rewrite the method
into two methods:

class BaseClass {
  public virtual void TemporarlyDisableFiltering();
  public virtual void ReenableFiltering();
}

class InheritedClass {
  private FilterBaseType disabledFilter;

  public override void TemporarlyDisableFiltering() {
     this.disabledFilter = this.filter;
     this.filter = null;
  }

  public override void ReenableFiltering() {
     this.filter = this.disabledFilter;
  }
}

This, of course, comes at the cost of adding a field to the inherited class, but the improved readability is well worth it in my view. The next person who comes into this code, will not have to wonder why there are two set methods. Also, someone coding against these classes won’t have to figure out which of the two sets methods to use.

In reality, this code was in C++ and quite complex – otherwise we would probably have found a lot more ways to make it even nicer. But now we’ve at least done a bit to make things somewhat better.