The other day we were refactoring some code and moving things around which I was all excited about. Most of the stuff was being moved to make it more modular and there wasn’t much actual change to the code needed. Doing a quick review I noticed something like this:
had been updated to this:
get_power and get_list are hard coded for this example but are really computed at runtime.
Since I strongly believe that having a clean object model greatly improves your application I dutifully left a comment. The comment basically stated that we’re now mixing instance and class method functionality and although this could help performance it could also lead to weird things like
I immediately got responses stating “who would ever do that?” which is a great question. I don’t know anyone that would ever do that purposely but I also don’t know anyone that purposely writes bugs in code. The reason this pains me much is it now leaves an big opportunity for someone to do something accidentally. Even worse is if they did that this method would fail silently (meaning it would produce a result as if nothing wrong happened, but that’s an whole different post).
The other argument I got was the “little” bit of risk is worth the benefit (referring to increased performance). Performance is always a concern but is there even a performance problem here? We might be optimizing a bit early here but some metrics could prove me incorrect here. Then I got to thinking more about the “little” risk of this change. Is it really little? I started to write down all the things that had to be done now for this method:
- Add a comment to describe what the new parameter is (and it’s default)
- Comment what happens when passed nil
- Comment what happens when passed empty
- Comment what happens when passed an array of a different type
- Which array takes precedence? The one passed in or the one called from within the method?
- Update the return comments
- What does happen if you pass a different list in
- Add tests (mostly duplicates) for when passing in the parameter
- Test for nil passed in
- Test for empty passed in
- Test for array of different type passed in
- Test which array takes precedence
- Test what happens if you pass a different list in
Wow… that’s a lot to keep track of. So much in fact I stopped writing things down. I already know that people will say “you don’t have to test all those variations since they are already tested in the non-passed in version.” Most of the cases may be covered by one set of the tests but tests (especially unit tests) are most powerful when used as regression tools. You don’t know what’s going to change in the future or how it will change so you need to do your diligence now to try and catch it when it happens.
Yes, now when it is fresh in everyone’s mind this doesn’t seem like a big issue but think about a year from now when there is a bug and you have to revisit this. Having to figure out all the quirks is time consuming and difficult. It’s also good to plan for the bad things people can do (intentionally or not). Imagine this method computed the amount you were going to charge someone. This would be a terrible bug that I wouldn’t want to have to track down:
It sometimes help to imaging that the next person to read your code will be an axe murderer…