Posts from  April 2012


Commit Review Questions

Note: in this article when I refer to a commit, I mean the commit you plan to share with the rest of the team, if you have local commits that you plan to amend/combine, I am referring to the final result.

In time you will find these easier to do as you develop, however, all of these are valuable before checking in! The pre commit review is a nice time to polish what might have been several hours of intense work, during which these things were the last things on your mind! If you are concerned about losing your work in the process of responding to these questions, first do a check-in and amend it as you go (assuming you are using a tool such as git that supports this), rolling the result into one nice commit for everyone else.

Did you review your commit, change by change, with a diff utility?

  • If not, this is a list of reasons why you might want to start!

Did you test your changes?

  • If the test is valuable to be automated, is it?
  • If it’s a manual testing scenario, did you at least try the basics manually?

Are the additions/changes formatted consistently with the rest of the project?

  • Lots of automated tools can help here, don’t try to manually format the code, that’s a waste of time and as a human you will fail repeatedly.
  • Are these consistent: tabs versus spaces, indentation, spacing, braces, line breaks, etc
  • Resharper is a great example of a tool that can automate this for you (.net)

Are naming conventions respected?

  • Did you accidently use abbreviations, unless you have a good reason to use them?
  • Does capitalization match the conventions in the project/language?

Are files partitioned?

  • Sometimes we add new code in existing files in a pinch, it’s a good idea to split these out if they don’t belong
  • ie: are new classes defined in new files, if this is something your project values?

Is there commented out code?

  • If you are removing an existing feature, get rid of it, that is why we have VCS
  • If it’s not done yet, then why are you checking it in?
    • Perhaps a stash commit (git)?

Did you leave debug or unnecessary changes?

Do you understand all of the changes?

Are there spelling mistakes?

  • Including your commit message!

Is your commit message concise?

Is there follow up work?

  • Are there tasks you didn’t write down that you need to follow up with?
  • Are readability or reorganization changes needed?
  • This might be amended into the final commit, or it might be future work that needs added to the backlog.

Are there other things your team values that you should review?


Programming doesn't have to be Magic

Computer Locke

In the show LOST, the Swan Station had a button that “had to be pushed” every 100 minutes to avoid disaster. Several characters in the show took it upon themselves to have faith and religiously push the button, resetting the clock and averting the unknown “disaster”. There are striking similarities in this story to the code we write every day. Here are some common ones that I encounter:

  • “I don’t know what it does but the application doesn’t work without it”
  • “I added that code because I saw it in other similar places, I didn’t understand it, but thought it was necessary.” (for consistency, or to make things “work”)
  • “An error message recommended it”
  • “I copied that code” (and didn’t look at what it was doing)
  • “It was suggested in a forum online and it fixed my problem so I left it”

In all of these cases we haven’t done our due diligence to understand what the code we are writing is actually doing. In the rush to get things done it seems like we’re willing to push any button (add any line of code) just to get our desired result and move on. All of the above explanations are common things we encounter, and are valid ways to work through a problem we have, but when we find a solution to a task we are working on (whether a bug or a feature), we should take a moment to reflect on what we don’t understand. Remove what isn’t necessary, comprehend and simplify what is.

Why is it detrimental to commit code we don’t understand?

  • Perpetuates unnecessary code
    • If you copy code that isn’t necessary, someone else is more likely to do so, especially peers
  • Perpetuates tech debt
    • Adding unnecessary code leads to extra code that must be understood, maintained and eventually cleaned up in longer lived projects
    • Tech debt begets tech debt as other developers copy or use this code as guidelines in similar situations
  • Increases maintenance
    • How do we know the code is simplified if we don’t understand it?
  • Perpetuates a lack of ownership
    • Makes it seem ok to commit anything so long as it “gets the job done”
  • Perpetuates the notion that programming is magic
    • If we don’t take the time to understand every line of code we add, then we are contributing to the notion that it is simply enough to make the code work, regardless of how.


Don’t commit code that you don’t understand, take the time to understand it, simplify it and then commit it!