Anthony Steele's Blog : code review

Oxite

Anthony.Steele — Sun, 21 Dec 2008 21:14:00 GMT

Today I am looking at Oxite. Not this Oxite, but this Oxite. It’s a sample application on the ASP.NET MVC platform. It’s a blog engine, so it’s doubly interesting to me. New MVC blogging engines crop up on CodePlex every month or so (and I have contributed code to BlogSvc). Generally they pass without much (or any) notice.

Oxite, however, arrived with an impressive degree of completeness, and generated a lot of publicity. Not all of it was accurate – some seemed to think it was a major new part of Microsoft’s internet strategy rather than a sample app by a couple of guys at Microsoft. As it stands, it's not a competitor to WordPress. I like WordPress, I have deployed and configured it and several times. While I'd like something like that which I can also hack in C#, Oxite is not there. Yet.

Code reviews followed. See Rob Connery's blog, here and here for details. Oxite is clearly not ready for general use as finished software, it’s not even ready as a best-of breed code sample, but developers may be interested. Oxite has a few code issues (see Rob Connery’s links above), but it's also got advantages:

Advantage 1: The guys who wrote it have put a lot of time and work into it - they can afford to, it's part of their day job.

Advantage 2: Oxite got instant publicity and community, including a code review from Rob Connery, and lots of other people poking at it- orders of magnitude more downloads and comments than the other MVC blog engines on codeplex got, combined.

I'm watching Oxite with interest - I want to know if they will take the advice on board, and will it become more than just a "sample app".

Next time I will show putting OpenId support into Oxite, with help from DotNetOpenId and sample code from Rob Connery via a few of my colleagues.

A method should have only one return statement. Discuss.

Anthony.Steele — Mon, 14 Jul 2008 14:56:00 GMT

I heard again recently that "A method should have only one return statement" and of "The single-return law", that more than one return was a poor coding practice. I disagree.

public int IsThisCodeSoBad(int param) {    if (param == 0)      return 0;

   foreach(foo in bar)    {       If foo.fish()       {         return 1;       }    }

   return 2; }

I have never seen any evidence for the belief that that multiple exit points pose a coding risk, or are bad style. I do not know of any formal study that has found this to be the case, on C# code or on code in any other language.

Coding horror recently talked about "Spartan programming", and mentioned "frugal use of control structures, with early return whenever possible."

One of the few links on the topic that I could find was "Return statements and the single exit fantasy":

http://www.leepoint.net/JavaBasics/methods/method-commentary/methcom-30-multiple-return.html

Take a look at some of the comments on the Daily WTF (http://thedailywtf.com/Articles/Refactoring_to_the_Max.aspx ) where this issue is raised, particularly this one:

Re: Refactoring to the Max

2006-04-05 02:25 • by John Hensley

The main reason for the single return "law" in C is to make sure you clean up memory, locks, handles, etc. before you leave a function.

This, on the other hand, is Java code.

Apparently the reason for this "law" was that when coding in C or the like, it was natural to deallocate resources near the end of the function. Thus any extra exit points were an invitation to memory leaks. There are two reasons why this is no longer the case:

Firstly, garbage collected languages make explicit deallocation unnecessary in most cases.

Secondly, try...finally blocks and using statements allow deallocation to happen with greater certainly at the end of any block of code when it is needed.

In any event, a function with multiple exit points is a far lesser issue than a goto. In some cases it is the simplest way to code now that we have control structures to deal with it. But beware of returns in the middle of loops or highly nested code. All other things being equal, avoid them, since they are less readable and predictable.

It is absolutely fine to use it as a first check on the parameters after entering a function, before getting down to the serious work and allocating those resources,– it's more readable than embedding the rest of the function in an if-block. However throwing an exception is the more usual case for this.

My view of the matter is:

There are cases where a single return is more readable, and cases where it isn't. See if it reduces the number of lines of code, makes the logic clearer or makes the control structures less deeply nested.
More than one return style may be a bad habit in C code, but there is no evidence for it being bad in more modern languages.
There is no law requiring only one exit point for a method. Some people have an unsubstantiated belief in the superiority of this style.
Requiring fewer return statements for purposes of centralising resource deallocation is not a common need in modern languages that have garbage collection and try..finally blocks.

Therefore, use as many returns as suits your artistic sensibilities, because it is a layout and readability issue, not a technical one.

There's another issue at play here, about rules being adhered to without people grasping the reason why they are adhering to them, and thus keeping the rule in force after the need for it has evaporated. Rules should be given along with reasons for them.

Addendum:

Watching the DailyWTF commentators (who have a fairly high opinion of their own coding skills, given that they are essentially gathered there to point and laugh) try and repeatedly fail to translate the following code into a single expression underscores that the multiple-exit points version can be in some cases easier to understand, read, and modify:


if (condition1(x)) 
  return true;
if (condition2(x)) 
  return false;
if (condition3(x)) 
  return true;
return false;

You'd think that this is the same as

return condition1(x) || !condition2(x) || condition3(x);,

However the one-liner version returns true if all conditions are false, unlike the original. Furthermore, it evaluates condition3 if condition2 is true, which the original does not.

You could also code it as:

bool result = false;

if (condition1(x))
result = true;
if (condition2(x))
result = false;
if (condition3(x))
result = true;

return result;

However this also evaluates condition3 if condition2 is true, which the original does not. I'm sure that the original logic can be replicated with only one return, but it's not going to be as elegant.

Update: Richard Tobin has sent me this comment by email:

This discussion is a common one that we continually face where I work. I took your example and reworked it into a single return as follows:

private bool LotsOfReturns(int x)
{
    if (Condition1(x))
        return true;
    if (Condition2(x))
        return false;
    if (Condition3(x))
        return true;
    return false;
}

private bool OneReturn(int x)
{
    bool flag = Condition1(x);

    if (!flag)
    {
        if (!Condition2(x))
        {
            flag = Condition3(x);
        }
    }

    return flag;
}

After compiling and looking at the IL with Reflector, the single return routine seemed more compact and efficient, and the C# source seems less tricky.

All I can really say is that he has suceeded in converting the code, and which one "seems less tricky" is going to be a matter of taste.

IDesign C# Coding standards

Anthony.Steele — Fri, 12 Oct 2007 15:35:00 GMT

De gustibus non est disputandum.

- A Latin proverb, roughly "don't argue about matters of taste"

As Scott Guthrie noted here:, Juval Lowry has published his C# code layout standards here: IDesign C# Coding standards This compliments the classic: www.tiobe.com/www.tiobe.com/www.tiobe.com/standards/gemrcsharpcs.pdf

Low-level Code standards are wonderful things for your emotions– you can agree fervently with 90% of one, and recoil in disgust at the last 10%.

I'm going through Juval Lowry's document, and comment on the bits that I disagree with or want to explicate, and try to give reasons why I disagree. I'll try to be mindful that arguing that "It looks bad to me" is not a valid reason – what looks right to you is just what you're used to.

Point 9a: "avoid single character variable names" as noted somewhere in Code Complete, the comprehensibility of a short variable name is dependant on how wide its scope is. If the variable is referenced throughout the application, give it a long descriptive name. If it is used only over 2-3 lines of private code, a short or general name will suffice, and anyone can read a variable called "i" over this scope.. However, "loopIndex" is a short enough, general enough name for me to write.

Point 11: use capital letters for types. I find class MyDictionary<Key, Value> more readable than class MyDictionary<K, V> , and it's more in keeping with point 9.

Point 14: Put using statments outside the namespace. Howard van Rooijen pointed out to me that putting the "using" inside the namespace means that it applies to the end of the namespace, not the end of the file, and this is generally what you intend, so there is a case to be made that the "using" goes inside the namespace. I'm not entirely convinced: if this distinction is meaningful to you because you have multiple namespaces in the file, one after the other, then you're doing it wrong. Very wrong.

Point 20: Preface private members with "m_". Is this looking Hungarian? Maybe, but it may be better than the alternatives. Various other conventions on this are in use. Starting with an underscore is not CLS compliant in cases where the var becomes protected and could be called from VB.NET code. I dislike using no prefix at all, just lower case, since it ends up in constructors like this:

class Foo

{

private int bar;

public Foo(int bar)

{

this.bar = bar;

}

Which is just an accident waiting to happen in the constructor – two identically named variables are in scope, and the use of "this." To distinguish between them is not optional.

Point 24: Open curly braces on new line: There is an exception for the simplest of properties,

I.e.

public int foo

{

get { return m_foo; }

set { m_foo = value; }

}

More complex properties should not do this.

Point 25-26 -27

Now we are getting into real matters of taste. See the way that the code on trailing lines lines up with the right-hand of the equals stuff? I don't like that. I prefer to always indent trailing lines by one indentation stop (typically four spaces).

E.g. Not

int someReturnValue = CallAFunction(param1, param2,

param3, param4);

But

int someReturnValue = CallAFunction(param1, param2,

param3, param4);

Despite taste, there are advantages of this layout:

Indentation of the trailing line is consistent with other indented lines.

Indentation does not depend of the contents of the line above.

Indentation does not need to change if the line above changes, e.g. renaming the var.

Indentation is less, and thus there is more room on the line – the code is more readable as it's not all squashed up on the right.

Section 2

Point 12-13 Bill Wagner explained the difference between readonly and const – (Effective C#, Bill Wagner, Item 2) – essentially const is folded in at compile-time, readonly values can be updated by updating the assembly that contains it.

Point 26 Avoid explicit values for enums. Why not give values to enums? I would, when they have to be explicitly the same as ints stored on a database. Also if you don't specify values, the first one will be zero. Zero is best reserved for an "unknown"/"not set" value. (Bill Wagner - item 8). By saying "except when they are integer powers of 2" does not encourage the reader to be mindful that enums in C# (and C) play two slightly different roles - select one from a named, restricted set of values, and to provide a set of flags over those values (this is when the numbers must correspond to the powers of 2).

Point 55 – Why prefer String.Empty over ""? I had to find out why this one is correct. Because it doesn't create a new string instance each time. Apparently it's a trivial difference, but a difference nonetheless.

http://blogs.msdn.com/brada/archive/2003/04/22/49997.aspx

Point 56 – When is StringBuilder faster than concatenation? If there's a loop over lots of strings it is faster, but not if just concatenating three or four strings.

http://www.heikniemi.net/hc/archives/000124.html

http://www.chinhdo.com/chinh/blog/20070224/stringbuilder-is-not-always-faster/

http://www.chinhdo.com/chinh/blog/20070929/stringbuilder-part-2/

Code Review

Anthony.Steele — Fri, 27 Jul 2007 12:43:00 GMT

"Code review" is one of those topics where it sometimes seems that everyone means something slightly different by the phrase, yet everyone assumes that others understand exactly the same thing as themselves.

Code review in the broadest sense just means reading code; inspecting code critical eye; usually code written by someone else at an earlier date.

The greatest benefit is that bugs are found early. Another benefit is that coders can learn from other's examples. And that common standards and idioms are propagated.

Wikipedia defines code review as "Code review is systematic examination of computer source code intended to find and fix mistakes overlooked in the initial development phase, improving overall quality of software and can also be used as a tool to better develop skills at the same time."

Some degree of code review is cost-effective, but there must be a limit to that somewhere. The Extreme stance is that continuous pairing is cost-effective; but nobody is suggesting that we'd get software quality cheaper and faster by having ten full-time reviewers for each person engaged in coding. So, there are natural limits, but some code review is good.

Code review deals with the basics of software. I seem to have an interest in getting the basics right - it's not SOA architecture design, but I think it's important to get the basics right even then.

Types of code review:

I'll try and list some types of code review that i can think of, to make discussion easier:

Self review:

Reading your own code prior to check in is in my opinion a good practice, especially if done with the source control system's diff tool in order to see what you have changed.

Informal peer review

e.g. "Hey Pete, can you have a look at this code before I check it in".

Pairing:

Reviewing the code as it is being written. This is slightly different to regular code review, since one of the points of "review" is to see if the finished product makes sense, without seeing how it got like that. Also note that wikipedia's definition of review includes "overlooked in the initial development phase" – if it's during the initial development phase, it's not review, it's just view. I'm not saying that pairing is bad, just that it does not quite fit the definition of code review.

Round-robin peer review:

Each person on the team reviews one other person's code.

Group peer review.

A piece of code is selected, and all members read it and comment.

Senior review:

A team lead looks at the code of all other people on the team.

Automated review:

Fxcop and the like can highlight lines of code worthy of more scrutiny.

Repository review:

Inspecting the source control repository to see what has been checked in. This is more common on open-source projects where many people can check code in.

Patch management:

A more extreme version of repository review where changes are submitted as a patch to someone who is willing to review it and who has permission to check into the repository. In extreme cases, e.g. on the Linux kernel, there are multiple layers of review. The person who first reviews and accept the patch does not have access to the core repository, but must then pass it up the chain after acting as a quality filter, either accepting, rejecting or reworking submitted code.

Other parameters

Feedback can be: verbally in a meeting, via email, wiki or comments in the code, via the source control system (e.g. TFS policy).

Formality can be low, with an informal eye cast on the code, or formal with a meeting scheduled and minutes taken.

Mandatory review: TFS can be set up to force all changes to be reviewed by another party before being approved.

Issues document:

This is a wiki page or the like where issues are recorded for later refactoring. I have seen it called a "code review document" but I don't see it as such.

Problems: Writing down an issue gives a feeling that "something is being done" but serves to delay actual action.

It is harder to separate bugs from todos, which are not within the scope of code reviews.

Keeping a list of todos and issues to large to tackle immediately may well be a good idea, but I don't think it fits the definition of code review.

Code review checklist:

A list of items, some possibly found in previous code reviews, to look out for in further code reviews. Items can be anything from general standard and policy down to specific errors to avoid.

My experience

Group peer review has worked well for me on a past project. The procedure was this: early in the week a file of code would be selected – selected because it was important to review – e.g. critical infrastructure, or it was the most complex example of it's kind; or a typical example of it's kind, or the first example in a new area of code.

We would print out the code with line numbers, and scribble on it with marker pens and pencils.

On Friday afternoon we went into a meeting room and worked from the top, commenting on everything. Any issues that repeatedly came up were fed into the code review checklist.

It's as much about learning from the code as it is about spotting errors in the code. Though a surprisingly large number of potential sources of error were spotted.

Also a common coding style rapidly came out, which aided readability. After a while we were rapidly moving past things that were old hat – either recorded in the review document or that we agreed to disagree on.

I suppose that this could be done at less cost by not being so formal – not booking a meeting room and all that, but the very formality and rhythm of it helped ensure that it kept going for quite a while.

Like many agile practices, it both supports and depends on the people - if your team is fractured, this won't fix it, but make it more visible.

So, what is your experience of code review?