Ayende @ Rahien

Refunds available at head office

Common issues found in code review

I am going over a code base that I haven't seen in a while, and I am familiarizing myself with it by doing a code review to see that I understand what the code is doing now.

I am going to post code samples of changes that I made, along with some explanations.

image

This code can be improved by introducing a guard clause, like this:

image

This reduce nesting and make the code easier to read in the long run (no nesting).

image

I hope you recognize the issue. The code is using reflection to do an operation that is already built into the CLR.

This is much better:

image

Of course, there is another issue here, why the hell do we have those if statement on type instead of pushing this into polymorphic behavior. No answer yet, I am current just doing blind code review.

Here is another issue, using List explicitly:

image

It is generally better to rely on the most abstract type that you can use:

image

This is a matter of style more than anything else, but it drives me crazy:

image

I much rather have this:

image

Note that I added braces for both clauses, because it also bother me if one has it and the other doesn't.

Another issue is hanging ifs:

image

Which we can rewrite as:

image

I think that this is enough for now...

Comments

PLamb
09/30/2008 12:29 AM by
PLamb

Not so sure about the first one. Changing to a negative condition and adding a continue and all you got rid of is some braces. If I'd come across your version, I would have changed it back like the first one. I'm pretty much of the same opinion for the last one too. It's "normal" practice to have a code flow for != null; introducing the continue doesn't improve readability for me. I agree on the others though. Maybe I've been doing too much ruby lately.

Agus Suhanto
09/30/2008 01:33 AM by
Agus Suhanto

Nice post Ayende! I found some of the works have been marked by ReSharper. Yes, with ReSharper we are warned of something that might be wrong.

Skim
09/30/2008 03:03 AM by
Skim

I don't see the point of the first refactoring.

It looks less readable - It'd have worked better if LuceneWork had a method called "IsNotABatch" instead of "IsBatch"

Moreover, I consider that guard clauses should be usually used near start of method. When you use guard clause within for loop, maybe the collection itself need to be populated differently instead of having to check and add guard clause within for loop.

Phil H
09/30/2008 03:54 AM by
Phil H

On a semi-related topic, I noticed during one of your screencasts code like this:

if (myBoolValue == false)

  DoSomeThing();

rather than:

if (!myBoolValue)

  DoSomeThing();

Any particular reason for that? Do you find it more readable?

Hadi Hariri
09/30/2008 04:13 AM by
Hadi Hariri

I just generally like "if" statements to have braces, even if they are only one line, because it's a source of bugs appearing otherwise (obviously "else" plays an important part here)

Ayende Rahien
09/30/2008 06:09 AM by
Ayende Rahien

Phil,

Yes, I do find it more readable, and that is why I am doing it

Daniel Crabtree
09/30/2008 06:30 AM by
Daniel Crabtree

I agree with all these changes, particularly the last guard clause example.

However, the improvement made in the first case is marginal at best. Assuming you've implemented a Has extension method, there is a far better alternative.

if(queue.Has(w => w.IsBatch))

workspace.IsBatch = true;

If you haven't added and refuse to add the extremely useful Has extension method, then you could use the less clear alternative:

if(queue.FirstOrDefault(w => w.IsBatch) != null)

workspace.IsBatch = true;

Both replace the entire foreach loop and improve readability dramatically.

Arild
09/30/2008 07:30 AM by
Arild

If you haven't added and refuse to add the extremely useful Has extension method, then you could use the less clear alternative:

if(queue.FirstOrDefault(w => w.IsBatch) != null)

workspace.IsBatch = true;

if (queue.Any(w => w.IsBatch))

Rik Hemsley
09/30/2008 08:06 AM by
Rik Hemsley

Golf...

workspace.IsBatch = queue.Has(w => w.IsBatch);

On the subject of reducing nesting: I'm also a fan of the 'get me out of here' checks before the actual logic, but I think it's a shame that C# doesn't help us make these clearer.

In C#, I write code like this:

if (some condition)

{

return;

}

if (! some other condition)

{

return;

}

This can be 'tidied' to:

if (some condition) return;

if (! some other condition) return;

But this puts the returns at different levels, which isn't particularly readable and would probably cause me pain later.

Ruby allows this:

return if (some condition)

return unless (some other condition)

Markus Zywitza
09/30/2008 09:51 AM by
Markus Zywitza

I'm not keen on replacing List <t with IList <t. If it's a list, I can do some nice things like l.ToArray() or l.AsReadOnly(), which is not possible with IList <t.

Bryan
09/30/2008 12:36 PM by
Bryan

I like having ILists as parameters to methods and return Lists from methods.

A lot of the changes posted take < 10 seconds with Resharper. Also, I have never found (but Resharper always suggests)

if(!someCondition)

{

 continue;

}

more code

to be more readable than

if(someCondition)

{

 more code

}

I dont find nesting to be an issue unless we are talking about 3 or 4 nests inside eachother. The logic one has to run through when considering the first option is going to slow down someone skimming code when looking over the 'continue' example as opposed to the other statement.

J.P. Hamilton
09/30/2008 02:30 PM by
J.P. Hamilton

The world would be a much better place if people would just read Refactoring. I have recommended the book over and over to people on my team. More than a year has passed and not one of them has read it.

sheraz
09/30/2008 04:39 PM by
sheraz

@Markus Zywitza: IList gives more flexibility as you are working against the abstraction. If you need the functionality of list then simply do this

public void Test(IList <string list)

    {

        string[] array = new List

<string(list).ToArray();

    }
Daniel Crabtree
09/30/2008 05:29 PM by
Daniel Crabtree

@Arild, quite true. I have no idea how I overlooked the Any function for this purpose.

@Rik Hemsley, your simplification is not consistent with the original code.

workspace.IsBatch = queue.Has(w => w.IsBatch);

This will change the value of workspace.IsBatch to false if none of the items has this property, whereas the original code would not. This is inconsistent as workspace.IsBatch may have already been true due to some previous code, so making this modification introduces a bug. If you want to make this further simplification, the following is needed:

workspace.IsBatch = workspace.IsBatch || queue.Has(w => w.IsBatch);

Dag
10/01/2008 09:32 AM by
Dag

@sheraz: with Linq: list.ToArray() (or even list.ToList().ToArray() if you prefer)

I prefer to use string.IsNullOrEmpty(), not sure if your StringHelper.IsEmpty contains any extra magic though.

Benny Thomas
10/02/2008 11:19 AM by
Benny Thomas

Good call, Daniel

I didn't see the workspace.IsBatch problem, but this may be a preemptive strike and not needed.

Marcel Popescu
10/29/2008 08:45 AM by
Marcel Popescu

{} are noise, only there because the compiler can't figure out what statements are part of a branch. I can - the ones that are indented. That's why I remove the {} wherever I can :) But yea, I've seen that most people prefer it the other way around.

Comments have been closed on this topic.