by ·7 Comments

My team at work has a system whereby after a task is completed and committed to source control, it is assigned to a developer unrelated to the task for a code review. The idea here is that fresh eyes on the problem may pick up something not considered by the original participant(s).

But what happens during a “code review”? There’s no single answer to this question and my experience is that it varies considerably based on a number of factors. After thinking for a little bit about what I do when code reviewing, I came up with a list of stages in a code review and figured I might just share it with everyone. The list below outlines the process I (personally) go through when reviewing a change. I don’t actively pursue these topics one by one but I noticed that I do consider them subconsciously.

  • What was the problem being solved?
  • Do I have a clear understanding of the problem?
  • Can I reproduce the problem?
  • What code was checked in?
  • Is the code change unit tested?
  • Does the code change fix the problem (functional test)?
  • Does the code change address the problem (code analysis)?
  • Does the change consider other possible related problems?
  • Does the code change introduce new bugs?
  • Are there any stylistic recommendations?

What was the problem being solved?
Coming in as a fresh set of eyes means that I don’t know the problem apart from the very high-level description I heard about during our stand-up meeting. If the change is trivial, or if there is enough information in the issue system, then I won’t bug the developer, otherwise I will ask them what the change was about and get them to describe the problem being solved.

Do I have a clear understanding of the problem?
For issues I don’t immediately understand, I tend to ask questions until I get an “aha” moment. With a solid understanding of what the enhancement or fix was, I’m better armed to think both inside and outside the box for any potential problems.

NB: Thinking outside the box is great, but if you can’t even think within the box, then you’ll struggle – ask as many questions as you need to until you completely get what’s going on.

Can I reproduce the problem?
This step is optional depending on the situation. I say that very carefully, because I’m of the opinion that if you can reproduce the problem then you should just to confirm it actually existed in the first place. When working with a junior developer once, he spent a few hours trying to fix a problem and during code review we discovered that it was an environmental specific setting on his machine, not a problem with the system. This is a good reason to reproduce the problem first. Depending on the seniority of the developer, I’ll make a judgmental call as to whether to try and repro or not, where more senior developers are more likely to be trusted in having identified the real cause of a problem.

One situation where you might not reproduce the problem at the start is based on your development workflow. If you perform reviews after the change has been made, then the only way you can reproduce is before you sync to the latest version (which contains the change) or you have to rollback the change. I don’t particularly like using this as an excuse though, and if i can comment out a few “key” lines in order to repro a problem, i will.

What code was checked in?
Source control is great. What was checked-in for the fix? Generated code files? Hand-made changes? modified related binaries?

If a file was committed because a modification was necessary, then (der) it had to be checked in. But what about some files which are modified but do not necessarily need to be checked in? One example in Delphi is when you modify a module, it makes changes to both the code file (.PAS) and the visual form file (.DFM). The PAS changes consist of your code, and unless you made specific UI changes, there is no reason why the designer should have changed the .DFM. It’s silly, it’s redundant and worse yet if it gets committed it adds unnecessary noise to your change history.

Another example more .NET pertinent would be autogenerating code from a DB or XML or whatever. If the schema hasn’t actually changed, then the only difference in your files will be an autogenerated comment of the date/time the file was modified. But the interfaces haven’t changed so who cares what date the file was last generated?

I hate making this mistake so i’m sure to point it out to anyone’s code I review.

Is the code change unit tested?
I wont get into the good vs evil of unit testing so much as to say that in most cases, I would expect to see some form of automated test checked in with the code. If it’s not tested, it’s impossible to prove the problem ever existed. I say “most cases” because on some projects (particularly the very brown/black-field ones), some testing is near impossible. I expect that new code which doesn’t interact a lot with old code to have a number of comprehensive tests.

Does the code change fix the problem (functional test)
This is pretty straightforward. Usually you will have a list of steps to reproduce the problem, or a way of examining the new feature. Follow these steps carefully and ensure that the base scenario works as expected. Then start trying alternatives in a methodical approach in order to determine that something immediately around the changed area hasn’t unintentionally been affected.

For instance if you’re testing that when a button is clicked and option A is turned on, the widget will somersault. Then check that when the button is pressed with only option B enabled, the widget does a pirouette. Once satisfied check that with both options, the widget still somersaults and also pirouettes at the end.

The number of permutations will obviously change and increase exponentially the more variables you introduce to the problem. You can improve this by using the theory behind Pair-wise testing

Does the code change address the problem (code analysis)
Okay so you’ve tested the change from the front-end and it appears to work. Now what was changed in the code? There might be plenty of ways to solve a problem and it’s entirely plausible that the worst one was taken.

For example assume the following bug: When you click button A, an exception dialog is shown claiming that the widget was null and the process couldn’t continue. The worst fix in this case, is to swallow the exception and do nothing about it, potentially leaving the system in an unstable state. A better solution would be to find out the cause of why the widget was null and prevent the situation from arising in the first place.

This is not a question about style, or design (anti)patterns or comments – this is about whether the implementation was appropriate for the problem.

Does the change consider other possible related areas?
Joe makes a fix to the CarpetWidget class and sends it to you for review. Has Joe considered that the same problem exists in the HardwoodWidget, or the TileWidget? Sure you might have functionally tested all relevant combinations, but tracing through the code in your head may expose other areas conditions which you previously may not have considered, or may have deemed irrelevant.

Does the code change introduce new bugs
So the developer has fixed the problem and they’ve covered all possible conditions – great! Now what’s wrong in THEIR code? New bugs can be easily introduced without realising and can be quite insidious. One example of this might be using an unmanaged resource and not disposing after use. Or catching an exception and not re-throwing it the correct way. Simple mistakes which may never show up, but if they did they could be considerably problematic.

Are there any stylistic/design recommendations?
This is by far the most abused by most people performing a code review. In my experience most developers generally fall into three camps. The first camp are developers who have suggestions to improve style and/or design but are too afraid or shy to come forward and express their opinion. These people are not helpful to a development team. They are short-circuiting the feedback cycle which doesn’t promote knowledge sharing. Everyone has an opinion. Some people have better ones than others. If you don’t communicate, one (or both) parties will never improve.

The second camp consists of developers who have nothing but petty and small suggestions. They’re approach to a code review is “My way would be different, so i’ll pick on all the small things which make me feel better”. For example, changing a “while” loop to a “do..until” loop because it might be semantically correct. Seriously, who cares? The while loop is a much more common looping paradigm than a do-until and therefore more likely to be understood first time by anyone else reading the code. In the absence of any technical reason why the loop should be changed, it’s an unnecessary waste of time and not productive to the end goal (delivering the product).

The third camp consists of developers who acknowledge the flaws in the other two camps and consciously balance what is important and what isn’t. These are people who have a fair number of years under their belt and appreciate the value of time as a commodity. Sure, changing that while loop to a do-until would be cute but the code is already written, it works, it’s tested and just because it’s not mine doesn’t mean it’s bad!

A lot of developers think that a “code review” involves only this one stage and fail to really consider many (if any) of the other points above. (“Yeah it looks good, that’ll do”). I once worked in a place where the code review process consisted of my manager scrolling up and down through the diff-comparison and checking that there looked to be enough lines modified. Needless to say some people are not cut out for the job.

And that’s how it works for me. Writing this post has helped me solidify the process a little more, even if only in my own head. I welcome your thoughts on the topic – maybe there’s something i’m missing?

by ·0 Comments

I’ve just read a blog post about why you should pass interfaces instead of concrete classes as arguments to your methods.

I normally try to think about the most appropriate usages of interfaces for my own classes, but what this post alerted me to was the necessity to use interfaces when working with framework classes.
IE: IDictionary instead of Dictionary

The reasons the author discusses i believe are quite valid….It’s something i’m going to more actively do when writing code…

by ·2 Comments

Reading an article about unit testing got me thinking about some of the tools i’d love to sink my teeth into.

I’m documenting them here in case i forget.

Backend
NHibernate (ORM)
SQLLite (DB)

Framework
Castle Windsor/Ninject (DI)
Lof4Net (Logging)
LINQ (Language Querying)
Tree Surgeon (Environment setup)

Testing
NUNit (Unit testing)
Rhino.Mocks/Moq (Mocking)
WatIn (UI testing)

Build Integration
Nant (Build tool)
CC.NET (CI server)

UI
WPF (GUI)
ASP.NET MVC /Monorail (Web engine)
PRISM (WPF App framework)
NHaml (MVC View Engine)

a lot of these i have or currently do use….some of them i have only ever played around with and a few i’ve never even touched.

At least me putting it down on paper (or bits, in this case) is a reminder of what i’m keen to try, and will get to it soon.

by ·0 Comments

The Hollywood principle is a software design methodology that takes its name from the cliche response given to amateurs auditioning in Hollywood: “Don’t call us, we’ll call you”. It is a useful paradigm that assists in the development of code with high cohesion and low coupling that is easier to debug, maintain and test.

http://en.wikipedia.org/wiki/Hollywood_Principle

by ·5 Comments

I’ve seen this now twice in about 30 mins, and its bugging me.

One of the developers i work with is writing code like this:

string postingUrl = CmsHttpContext.Current.Posting.Url.ToLower(CultureInfo.InvariantCulture);

Whats bugs me with this code is that it shows no understanding or care for defensive programming.

Q1: Why do you assume that CmsHttpContext.Current is safe? Sure, within ASP.NET the framework creates a new Context for you upon request, and (in this case) Microsoft CMS wraps the HttpContext and guarantees you a copy of a context for you. Under these known conditions, the CmsHttpContext.Current is safe.

Now that’s been chained onto Posting, which throws the exception. Why assume that you’ve hit a posting? Why assume that there would be a posting at all? This kind of lack of thinking just demonstrates lazy programming, to me.

*grumble*

by ·0 Comments

Covariance and Contravariance are terms used in programming languages and set theory to define the behaviour of parameter and return types of a function.

Yes, that’s a mouthful, but in a nutshell:

  • Covariance mandates that the return type and the parameters of a function must be a subtype of the original base class type for any superclass
  • Contravariance allows the return type and/or the parameter types to be super-types of the defined types and not necessarily sub-types

Nothing better than using an example:

[HTML1]

In this example:

  • Animal is a superclass.
  • Human is a subclass of Animal, with a covariant (no change) override to the CreateChild method to return the looser type Animal
  • Dog is a subclass of Animal, with a contravariant override to the CreateChild method to return the stronger type Dog

More reading on Eric Lippert’s blog series on Covariance and Contravariance in C#

EDIT: I thought it best prudent that I clarify that this is only one example of where variance is used. Method signatures, delegates and arrays are some more examples of where the theory of co and contra variance can be found.

by ·1 Comment

I found this article explaining the differences between a good programmer and an average one

I’m pleased that i can read that list and honestly say I am/do most of those things. Possibly the notable exception is reading books. I agree with one of the commenters in that books are virtually outdated from the moment they’re printed. Software is changing so fast, the most beneficial information you could get from a book is not instructions on implementing a solution (ie: “Teach Yourself XXX in 24 Hours”), but the theories behind good solutions (eg: “Design Patterns: Elements of Reusable Object-Oriented Software“).

I own both, and I have rarely ever picked up the former after reading it.

by ·1 Comment

I love this one. I don’t know the name of the pattern, but i love using it.

Imagine your system uses strings to represent the value of a status field. potential values of this field are:

  • New
  • Requested
  • Open
  • Closed
  • Deferred
  • In Progress
  • More Information Required

…and you want to write a method (or webservice) called UpdateStatus, which takes in one of these values.

Standard signature would look something like this:

public void UpdateStatus(string newStatus)

The use of “string” for the static type has the problem that someone could pass through a rubbish value as a string, and the UpdateStatus method is now responsible for domain validation of the input.

However, following the pattern below, you’re able to statically define your domain (if known at design-time) and enforce the stronger type in subsequent method calls.

public class IssueStatus
{
	void IssueStatus(string description)
	{
		this.description = description;
	}
		readonly string description;

	public static IssueStatus New = new IssueStatus("New");
	public static IssueStatus Requested = new IssueStatus("Requested");
	public static IssueStatus Open = new IssueStatus("Open");
}

and you can then use the strong IssueStatus type in your signatures:

public SaveButton_Clicked(object sender, EventArgs e)
{
	bo.UpdateStatus(IssueStatus.New);
}

public void UpdateStatus(IssueStatus newStatus)
{
	if (newStatus == IssueStatus.New)
	{
	// do something here
	}
}

I learnt this one on the job a few years back, and think it’s fantastic.