Anatomy of a "Small" Software Design Change

asp.net mvc, code 0 comments suggest 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

Found a typo or error? Suggest an edit! If accepted, your contribution is listed automatically here.

Comments

avatar

36 responses

  1. Avatar for Haacked
    Haacked April 24th, 2008

    Note that in the latest MVC code, we changed what an action looks like again. But being up to date with our latest code is not the point of this post.

  2. Avatar for Gary Sherman
    Gary Sherman April 24th, 2008

    It’ll only take you a couple hours, right?
    "only" is a 4 letter word.
    http://www.37signals.com/sv...

  3. Avatar for Chad Myers
    Chad Myers April 24th, 2008

    Good post, Phil. 37signals had a post kinda like this about the 'four letter words' of collaboration:
    http://www.37signals.com/sv...
    (credit to Gary Sherman @ Dovetail for finding it)

  4. Avatar for GlenH
    GlenH April 24th, 2008

    Whew! After reading this, I'm thinking you should go back to the original paradigm of requiring a [ControllerAction] attribute. Seems like it would be easier to apply an attribute exactly where you want it vs. having to remember all the rules to apply to prevent something you DON'T want to happen.

  5. Avatar for Sean Chambers
    Sean Chambers April 24th, 2008

    When it comes to Controllers this is something that MonoRail has struggled with very frequently. Once you start using inheritance in Controllers you expose all of said ugliness that you dont want in derived controllers.
    What MonoRail has done to mitigate this problem is introduce DynamicActions. It is a lot easier to use associations over inheritance when it comes to Controllers and some form of reusable actions should be introduced that can be plugged in or not without having to add extra code to not use an action. When designing controllers I rarely use inheritance as it almost always doesn't fit well. Sometimes it does work, however this is the exception.
    I dont think I like the idea of introducing attributes to simply hide actions however, this adds extra noise that isn't as much as [ControllerAction], but noise all the same.
    You must really dislike people like me by now, but I respect honesty and I try to give the same to everyone =) Just my 2 cents!

  6. Avatar for DM
    DM April 24th, 2008

    Ummm... To be honest, I don't fully understand this problem, so I may be way off here...
    Doesn't this seem like a security problem waiting to happen? You really want to make public methods on the controller class callable by default? Through the web?

  7. Avatar for Haacked
    Haacked April 24th, 2008

    @Sean I disagree with the noise claim. It's only "noise" if you use it. And when you need it, you're glad it's there.

  8. Avatar for Jeff
    Jeff April 24th, 2008

    The obvious solution here is to mark what is acceptable as a web callable method, instead of trying to mark what isn't. Less chance of errors that way.

  9. Avatar for Lou D
    Lou D April 24th, 2008

    I feel your pain and it sounds like there's a good handle on what's come up. In the end you get to think about these issues so the rest of the world doesn't have to. So thanks for that. :)
    I know it wasn't really the point of your post, but I have to say I agree with the decision to omit the ControllerAction attribute from a DRY standpoint. I've made a controller, I've declared a method public, and 99% of the time an explicit attribute would be an "Are you sure?" prompt.
    Maybe the [NonAction] could be blended away into a restful-allowed-method attribute... As in [Accept(Get|Post|None)]?

  10. Avatar for Eric Hexter
    Eric Hexter April 24th, 2008

    oh you had me at Lumberg. I appriciate the team making this change and dealing with all the unknowns that came with it!

  11. Avatar for Vijay Santhanam
    Vijay Santhanam April 24th, 2008

    Good story. So true.
    Fortunately, cos of testability, things swung round in your favor.
    Global warming shrunk the iceberg?

  12. Avatar for Haacked
    Haacked April 24th, 2008

    @Lou the idea is that [NonAction] makes it so that the action doesn't even exist. Whereas what you proposed would mean that the action does exist, but returns a 405 "Method not allowed". They are semantically different.
    I suppose, could remove the NonAction attribute and you could make a public method "not an action" by simply calling HandleUnknownAction.

  13. Avatar for Haacked
    Haacked April 24th, 2008

    @DM the part you missed because I left it out is that there's pretty much no good reason to have a public method on a controller class be public unless you intend for it to be web callable. In other words, you opt in to this web callable behavior by inheriting from our Controller class and by hosting your class in our framework. So these things aren't web callable by accident.
    And since every method in a controller is generally an action method and intended to be web callable, it's pretty repetitive to have to mark each one as web callable. You already told us you wanted these things to be callable by way of using our web framework and inheriting from Controller.
    This addresses @Jeff's question too. The reason we still need the [NonAction] attribute (or equivalent) is there are ways in which a method might be public due to inheritance that you wish to hide.
    For example, an abstract base controller for your company might have common actions and you're supposed to use it. But you want to hide one of those actions.

  14. Avatar for Jarod
    Jarod April 24th, 2008

    "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."
    So is that better than having [Action] in the first place? Seems like we went backwards...
    "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."
    See previous point :)

  15. Avatar for Benny Thomas
    Benny Thomas April 24th, 2008

    This sounds like a stupid idè, but shouldn't ControllerActions return a ActionResult. So only methods that return ActionResult would be accepted as ControllerActions. Inheritance on Controllers requires the Class to implement the Actions it want's to expose from it's base.
    The implementation only needs to redirect the action to the base. This seems like a troublesome solution, but the gain is that it is easy to see which Actions a Controller exposes to the world.
    My 2 cents.

  16. Avatar for Vijay Santhanam
    Vijay Santhanam April 24th, 2008

    I can imagine this must've been an annoyance before the ActionResult came along.
    Out of curiosity, how much time was allocated for the switch to public web-callable? how much time did that blow out to?
    Really good example btw.

  17. Avatar for Richard
    Richard April 24th, 2008

    I understand the "methods in a controller are implicitly actions" line of reasoning, but IMHO the potential for hard to spot problems related to interface inheritence (e.g. with your IDisposable example) mean a "safe by default" policy would require explicitly marking which methods are actions rather than which are not.

  18. Avatar for Mike
    Mike April 24th, 2008

    What was the great argument for ditching the attribute?
    The whole dry thing really gets me as it's not really dry, the config is jushed pushed of to another location, like the POCO folks that hack out xml mapping files.
    IMHO using [NoAction] is pure insanity.

  19. Avatar for Offering A Solution
    Offering A Solution April 24th, 2008

    It's interesting that no one has offered up a real-world analogy. I'll give it a shot. Most guys, and some gals, will understand on a basic level, the concept of working on a car, right? Well, maybe.
    I used to own a 1968 Land Cruiser -- nice rig. Had the bare-bones basics. Three on the tree, flat six, no stereo, manually locking hubs. I needed to replace the fuel pump once, so I popped open the hood, and there it was on the right side of the block. Took me less than 30 minutes to replace.
    Then, years later, my dad needed his fuel filter replaced in his rig -- a 1998 (?) Jeep Cherokee Sport. Now, I wasn't there, but my uncle was. I was figuring, eh, it'll take them about 30-60 minutes to do the job. Problem was, solving the same problem on that rig was quite a bit different, as it turns out that the automobile industry had progressed quite a bit in thirty years. After looking in the engine compartment, searching around, and finally referencing the appropriate manual, they learned that the pump wasn't in the engine compartment at all, but was in the gas tank itself. So, "changing the fuel pump" (same problem on my '68 Cruiser) ended up being a very complicated task, indeed.
    Perhaps not the best analogy, but it might be a start. Of course, I'm not sure that likening software types to mechanics is such a good idea... We all know how many people love mechanics.

  20. Avatar for Fredrik Normén
    Fredrik Normén April 24th, 2008

    I think everything should be locked from start, and then we unlock things that should be open for public. By open up everything by default require us to make sure we don’t forget to close the things that shouldn’t be open. It’s easy to unlock things that should be public than lock thinks that shouldn’t at least that is my opinion.
    Btw, why should it be possible at all to make a call to a method that doesn’t return an ActionResult? I can’t see why that should be possible at all.

  21. Avatar for BrianE
    BrianE April 24th, 2008

    In my experience it is generally the edge cases that cause all the problems. Non-technical bosses tend to think in general terms so it is very hard for them to internalize all of the problems edge cases create.
    To help them understand you need to find something they are interested in and relate it to that. If they are a skier, you might relate it to a bird landing on a particular patch of snow that causes and avalanche. For some one into home improvement you might relate it to removing a section of a wall to expand you kitchen only to find you have a termite infestation.

  22. Avatar for Haacked
    Haacked April 24th, 2008

    @Benny, you just described our current implemention. We now do have action methods return an ActionResult.

  23. Avatar for Al Pascual
    Al Pascual April 24th, 2008

    Great post as also sounds like you are complaining about your boss!

  24. Avatar for Jacob
    Jacob April 24th, 2008

    Here are some possibly useful analogies for things that seem easy to an outsider.
    Accounting: changing the GL Account number for COGS.
    Finance: stock buybacks.
    Board of Directors: hiring a CEO/adding a new director.
    C-level officer: hiring an executive assistant.
    Sales: closing.
    Operations: adding/changing a product line.
    Marketing: adding/changing a product line.

  25. Avatar for Stephen
    Stephen April 24th, 2008

    While I understand the reason this was done, and I've often found I've been really proud at how clean something worked out, only to later realize that, while clean.. its actual goal didn't really make sense..
    WCF is a good comparison to this in that WCF took the route to be explicit.. everything has to be explicitly marked to be callable.
    Of course it isn't likely to change back, but personally I would of preferred an attribute to state what IS an action..
    I spent a lot of time thinking about the MVC in comparison to WCF.. for example, what about controllers that were only defined by an attribute? no inheritance.. the controller currently actually holds a lot of the execution handling, and makes them a bit messy.. in what I see as a controller.. it should be related only to what you define...
    WCF then lets you get context via static methods (much like how one would use HttpContext.Current).. they have OperationContext etc etc..
    It just appeared to me that WCF has a lot in common here.. services vs controllers.. they are highly similar concepts.. and WCF is such a clean implementation I would of liked the MVC to follow..
    However, I've never built an MVC, so you guys probably have a list of reasons why the MVC shouldn't be inspired by WCF..

  26. Avatar for JacobM
    JacobM April 24th, 2008

    I agree about the issue of edge cases -- sometimes I get requirements where there are only ever going to be two types of something. Ever. Except, well, maybe very occasionally there will be three types. Non-technical people don't seem to understand that we have to design architecture to support the limit of what may happen, not just the typical case.
    So an example might be feeding a bunch of kids. We'll start with hot dogs. Most kids like hot dogs, right? But it turns out some of them are vegetarians. So the boss comes down and says, hey, let's also have peanut butter sandwiches -- that'll be easy to add. And it is -- except that it turns out that some kids are so allergic to peanuts that they can't even be at a table where peanut butter is being served. So now suddenly we have to query all the kids and their parents and divide up the group, and have two sets of utensils and so forth.

  27. Avatar for Benny Thomas
    Benny Thomas April 24th, 2008

    @Haacked
    I believed you only did the ActionResult on RenderView. But it's nice to hear that you did this for Actions to. If I could say it, I would say that this is a good design ;)
    I'll look @ the solution, my wife needs a little shop, so I'll will try implement it with Asp.Net MVC.
    Have a nice weekend.

  28. Avatar for engtech
    engtech April 25th, 2008

    I try to put features into a holding pattern if they don't have an obvious benefit.
    Just today I got to avoid work because we didn't close the customer we would have needed the feature for. I'm glad I waited a few days to start it :)

  29. Avatar for SuperJason
    SuperJason April 25th, 2008

    I wrote an article on how to show your boss you're doing a good job:
    http://www.ytechie.com/2008...
    I realize it's not related to MVC, but it discusses that common problem in software, where your boss just doesn't understand how good a job you're doing.

  30. Avatar for Lou D
    Lou D April 25th, 2008

    I have another analogy one for you that's also based on cars and personal. Scuffed paint and broken window.
    I scuffed the front bumper on a parking thing, not bad but enough to show and it had some yellow paint highlights. I'm thinking it's just painting over it with a standard color. I asked for an estimate when I was getting some routine stuff done anyway and I'm thinking a hundred or two maybe on the outside. Nope! Seven or eight hundred with materials and labor because you also have to blend the paint with the surrounding area and it has an automotive finish and the small size really doesn't matter in the end.
    On the other hand one day my window didn't go up. So it's halfway down and I'm thinking I don't even know where the fuses are to check that, and it could be the car's electrical is fried, or maybe the door will need to be torn down if it's a mechanical failure in the window itself. After the paint surprise I'm just hoping it's less than a thousand but the guy doesn't even need to look at it to say, "Probably the motor, they go all the time. It'll be eighty bucks and about an hour."
    So - scuffed paint is disaster and broken window is nothing.

  31. Avatar for Chris Chandler
    Chris Chandler April 25th, 2008

    Thanks for the great take on this.
    Love the behavior, glad to see something at microsoft that has a stance. Most other teams would implement a switch between negative and positive attributes with a class level attribute, thus making my life more and more configuration nightmare. However you guys realized, the inheritance is the opt in, I love it.
    Analogies (bad and otherwise):
    - a mullet, looks short up front, on the back end goes forever.
    - buying a house, everyone owns one, we've been doing it for centuries, it still takes 50 people and 60 days to make the purchase.
    - setting the time on your vcr, small task, big problem
    - its mole hill, if your a giant, problem is, we're ants

  32. Avatar for Michael
    Michael April 25th, 2008

    Real-life examples of "only"?
    Client would love this shirt to be also waterproof.
    This books comes in French too, right?
    Can we have just one more door in this car?
    The house is lovely but can you move it just a few inches south?
    Darling, would you mind also to keep an eye on kids while I am getting my PhD?

  33. Avatar for Kevin Gabbert
    Kevin Gabbert April 27th, 2008

    I've never heard "only", I always get the dreaded:
    "How hard would it be...."

  34. Avatar for Karl
    Karl April 29th, 2008

    Thank you for sharing your thoughts Phil. You are always very interesting. I certainly agree about the issue of edge cases.
    Best wishes

  35. Avatar for Ian Suttle
    Ian Suttle May 5th, 2008

    The first business-like analogy I can think of is absolutely anything that sounds simple yet requires Legal to get involved. Say goodbye to a fast turnaround, and hello to "hey, what ever happened to that project?"

  36. Avatar for Rob
    Rob May 25th, 2008

    I've been writing software four about thirty years now. During that time I've always been searching for the silver bullet to make my projects run smoothly and result in a high quality product. I tried many languages, frameworks and integrated application development environments. I looked at different design methodologies, diagramming techniques and paradigms. I got all involved in CMM levels, continuous process improvement, six sigma, ect. at the many companies I've worked for.
    Now I develop and operate small small web-enabled applications for clients. I only take on projects I can handle myself using one very flexible tool; Lotus Notes/Domino.
    I know, I know ... you're all rolling your eyes and groaning. I hated it at first, thinking they were trying to be all things to all people ... and they were. But I got past that and now I use Domino for one thing only; web application development/operation. I've never found a more flexible, simple and complete set of tools that lets me create, operate and maintain small web applications.
    Even though IBM/Lotus has sadly neglected the further development of the dynamic HTML generation for many years, I've still found nothing that can replace this set of tools. It just hits a sweet spot, at least for me, that I just love.
    Now for the final heresy; I make changes to the production code. That's right. I know the conventional wisdom is to make the change, test the change, them merge it into production very carefully. Well, Notes/Domino makes it easy for me to make changes to the production system without the users even noticing it, in most cases. That is until some new feature just magically appears.
    This all lets me keep costs at an absolute minimum while responding to feature requests and bug fixes sometimes in just minutes. I often add a new feature for a client while they're still on the phone with me. It doesn't get any better (or more fun) than that!
    Peace,
    Rob:-]
    p.s. And yes, I'm aware this won't work on larger projects written in compiled languages (however, most of my code is written in Java that runs in the server side within the Domino environment.) rs