Ayende @ Rahien

My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:


+972 52-548-6969

, @ Q c

Posts: 6,128 | Comments: 45,551

filter by tags archive

Common issues found in code review

time to read 2 min | 314 words

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.


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


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


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:


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:


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


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


I much rather have this:


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:


Which we can rewrite as:


I think that this is enough for now...



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

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.


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

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

if (myBoolValue == false)


rather than:

if (!myBoolValue)


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

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


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

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.


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


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)




if (! some other condition)




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

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.


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)





more code

to be more readable than



 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

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.


@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


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);


@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

Good call, Daniel

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

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.

Comment preview

Comments have been closed on this topic.


  1. The worker pattern - 10 hours from now

There are posts all the way to May 30, 2016


  1. The design of RavenDB 4.0 (14):
    26 May 2016 - The client side
  2. RavenDB 3.5 whirl wind tour (14):
    25 May 2016 - Got anything to declare, ya smuggler?
  3. Tasks for the new comer (2):
    15 Apr 2016 - Quartz.NET with RavenDB
  4. Code through the looking glass (5):
    18 Mar 2016 - And a linear search to rule them
  5. Find the bug (8):
    29 Feb 2016 - When you can't rely on your own identity
View all series


Main feed Feed Stats
Comments feed   Comments Feed Stats