Anatomy of a "Small" Software Design Change

asp.net mvc, code comments edit

File this one away for the next time your boss comes in and asks,lumberg[1]

Yeaaah, I’m going to need you to make that little change to the code. It’ll only take you a couple hours, right?

Software has this deceptive property in which some changes that seem quite big and challenging to the layman end up being quite trivial, while other changes that seem quite trivial, end up requiring a lot of thought, care, and work.

Often, little changes add up to a lot.

I’m going to walk through a change we made that seemed like a no-brainer but ended up having a lot of interesting consequences that were invisible to most onlookers.

The Setup

I’ll provide just enough context to understand the change. The project I work on, ASP.NET MVC allows you to call methods on a class via the URL.

For example, a request for /product/list might call a method on a class named ProductController with a method named List. Likewise a request for /product/index would call the method named Index. We call these “web callable” methods, Actions. Nothing new here.

There were a couple rules in place for this to happen:

  • The controller class must inherit from the Controller class.
  • The action method must be annotated with the [ControllerAction] attribute applied to it.

We received a lot of feedback on the requirement for having that attribute. There were a lot of good reasons to have it there, but there were also a lot of good reasons to remove it.

Removing that requirement should be pretty simple, right? Just find the line of code that checks for the existence of the attribute and change it to not check at all.

The Consequences

Ahhh, if only it were that easy my friend. There were many consequences to that change. The solutions to these consequences were mostly easy. The hard part was making sure we caught all of them. After all, you don’t know what you don’t know.

Base Classes

So we removed the check for the attribute and the first thing we noticed is “Hey! Now I can make a request for */product/gethashcode*, Cool!

Not so cool. Since every object ultimately inherits from System.Object, every object has several public methods: ToString(), GetHashCode(), GetType(), and Equals(), and so on... In fact, our Controller class itself has a few public methods.

The solution here is conceptually easy, we only look at public methods on classes that derive from our Controller class. In other words, we ignore methods on Controller and on Object.

Controller Inheritance

One of the rationales for removing the attribute is that in general, there isn’t much of a reason to have a public method on a controller class that isn’t available from the web. But that isn’t always true. Let’s look at one situation. Suppose you have the following abstract base controller.

public abstract class CoolController : Controller
{
  public virtual void Smokes() {...}

  public virtual void Gambles() {...}

  public virtual void Drinks() {...}
}

It is soooo cool!

You might want to write a controller that uses the CoolController as its base class rather than Controller because CoolController does some cool stuff.

However, you don’t think smoking is cool at all. Too bad, Smokes() is a public method and thus an action, so it is callable. At this point, we realized we need a [NonAction] attribute we can apply to an action to say that even though it is public, it is not an action.

With this attribute, I can do this:

public class MyReallyCoolController : CoolController
{
  [NonAction]
  public override void Smokes()
  {
    throws new NotImplementedException();
  }
}

Now MyReallyCoolController doesn’t smoke, which is really cool.

Interfaces

Another issue that came up is interfaces. Suppose I implement an interface with public methods.  Should those methods by default be callable? A good example is IDisposable. If I implement that interface, suddenly I can call Dispose() via a request for /product/dispose.

Since we already implemented the [NonAction] attribute, we decided that yes, they are callable if you implicitly implement them because they are public methods on your class and you have a means to make them not callable.

We also decided that if you explicitly implement an interface, those methods would not be callable. That would be one way to implement an interface without making every method an action and would not require you to annotate every interface method.

Special Methods

Keep in mind that in C# and VB.NET, property setters and getters are nothing more than syntactic sugar. When compiled to IL, they end up being methods named get_PropertyName() and set_PropertyName(). The constructor is implemented as a method named .ctor(). When you have an indexer on a class, that gets compiled to get_Item().

I’m not saying it was hard to deal with this, but we did have to remember to consider this. We needed to get a list of methods on the controller that are methods in the “typical” sense and not in some funky compiler-generated or specially named sense.

Edge Cases

Now we started to get into various edge cases. For example, what if you inherit a base controller class, but use the new keyword on your action of the same name as an action on the base class? What if you have multiple overloads of the same method? And so on. I won’t bore you with all the details. The point is, it was interesting to see all these consequences bubble up for such a simple change.

Is This Really Going To Help You With Your Boss?

Who am I kidding here? Of course not. :) Well..., maybe.

If your boss is technical, it may be a nice reminder that software is often like an iceberg. It is easy to see the 10% of work necessary, but the other 90% of work doesn’t become apparent till you dig deeper.

If your boss is not technical, we may well be speaking a different language here. I need to find an analogy that a business manager would understand. A situation in their world in which something that seems simple on the surface ends up being a lot of work in actuality. If you have such examples, be a pal and post them in the comments. The goal here is to find common ground and a shared language for describing the realities of software to non-software developers.

Technorati Tags: Software,Software Design,ASP.NET MVC

Comments