Drive-by Code Review

I decided to write a blog post about code review. It is something I get very passionate about when I see it being done incorrectly and it frustrates me that it has become a tick-the-box exercise for some.

I call it Drive-By Code Review. This is where you grab the guy next to you for "a quick two-minute code review" and nothing constructive happens...you just get to check your code in with a nice comment that it was reviewed by X. What is the point in that?!

Of course this can go to the other extreme too. Formal reviews with groups of people that take hours where every line of code is scrutinized to death...normally this is too much but of course has its place in places where the smallest mistake could be fatal (literally...)

I have read some interesting articles on code review recently and I would like to offer my cut-down lightweight process that I would like to see implemented where I work. I think these steps are generic and can be applied to any team but we are a .NET team that uses a CI server along with StyleCop and FXCop so I have mentioned these as part of my process.

1. Explain context of the change I am reviewing.

Every change should be tracked to a bug or feature or ticket or story or whatever is the driver for this change.

2. Show me your unit tests.

This obviously allows me to see that you have actually written unit tests but it really allows me to see your code from the consumer of the code's perspective.

3. Show me your code.

Your code should do what it needs to and no more. It should follow the coding standards of the team.

4. Show me zero StyleCop warnings and zero FXCop violations.

No broken windows here - every warning or violation of our rules should be treated as an error and fixed.

5. Show me all this building on the Continuous Integration server.

Our builds should fail if there are any violations of our rules or any unit test fails.

I believe if our code passes these tests and gets this far then it is in pretty good shape to be handed over to our QA team for system testing, integration testing etc.

One other important point I feel is worth mentioning is that if your change impacts on any other component or library etc. then somebody who has knowledge in that area should be part of the code review. This just makes sense to me.

Valid use of GOTO?!

I think I may well have come across a valid use of GOTO...
I'm thinking about fault tolerance - perhaps you have a dodgy wireless connection and you want to poll a server once a day by calling a webservice. What happens if that one time a day you go to call the service there are communication problems? The following code adds some fault tolerance for a situation like this.

int limit = 5;
int attempts = 0;

start:
attempts++;

try
{
    service.SomeMethod();
}
catch (WebException ex)
{
    if (attempts < limit) goto start;
    throw;
}

C# Readability Tips

Just a couple of C# tips I came across that might improve code readability – these are minor style tips really, nothing too earth-shattering.

Tip 1

For events, in C# before calling the delegate you need to check if anyone is listening to avoid a NullReferenceException, which gets ugly the more events you have (like in an MVP pattern).

if (this.DownloadCompleted != null)
{
   this.DownloadStarted(this, e);
}

Instead, declare it with an empty anonymous method as a handler like so…

public event EventHandler DownloadStarted = delegate { };

You no longer need to check if anyone is listening every time you fire the event. (However, your code will call the empty method everytime regardless of whether or not an event is subscribed to.)

this.DownloadStarted(this, e);

However you do need to change how you now check for listeners as you can no longer check for != null as there will always be 1 listener – the anonymous method.

if (this.DownloadCompleted.GetInvocationList().Length == 1)
{
   // no one listens to me.
}

Tip 2

You do not need to explicitly declare a new EventHandler () when subscribing to an event

this.applicationUpdaterManager.DownloadStarted +=new DownloadStartedEventHandler(Updater_OnDownloadStarted);

Instead, you can write the following

this.applicationUpdaterManager.DownloadStarted += this.Updater_OnDownloadStarted;

Note: you will still get a compile-time warning if you attempt to wire up a method with a different signature

Serializing Generic Types

There is no way currently of constraining a Generic type to ensure it is serializable at compile-time. However, there is a quick and easy way to check.

     if (!typeof(T).IsSerializable)
     {
         throw new ArgumentException("type is not serializable");
     }

You Gotta Constrain to Expand...

When you create a Generic class and constrain the Type parameter T, you are actually increasing the number of operations available. I know it sounds like a contradiction but it makes sense when you think about it. By constraining my Type parameter with an Interface for example, I get access to all the operations that Interface defines. If you do not apply a constraint then you are limiting yourself to operations on System.Object!

Quick example...

    
    public interface IPerson
    {
        int Age { get; set; }
    }

    public class PersonRepository<T> where T : IPerson
    {
        private T m_person;

        public PersonRepository(T person)
        {
            this.m_person = person;
        }

        public void Save()
        {
            // The Age property is available
            if (this.m_person.Age > 65)
            {
                // Save to free bus pass database...
            }
        }
    }

Hello World!

I am a 28 year old software engineer and this blog is going to help me to leave (a hopefully valuable and relevant) internet footprint. I can be found on Twitter and am also as a fledgling user on StackOverflow.

I watched a very interesting presentation from Scott Hanselman yesterday where he postulated that every developer should have a blog. If you think you don't have time, then think of all those long emails you write up every week that only reach an audience of maybe 4 or 5 people and are never seen again. Why not put your keystrokes and time to better use and write a post on your blog? Thank you Scott for inspiring me to start this blog. If I write one post that is even 1/1000th as relevant or interesting as the posts on hanselman.com I will have achieved something...