<< Emulating Java Enums | Home | More code review errors >>

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...

Print | posted on Tuesday, September 30, 2008 2:54 AM

Feedback

Gravatar

# re: Common issues found in code review 9/30/2008 3:29 AM 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.

Gravatar

# re: Common issues found in code review 9/30/2008 4:33 AM 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.

Gravatar

# re: Common issues found in code review 9/30/2008 6:03 AM 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.

Gravatar

# re: Common issues found in code review 9/30/2008 6:54 AM 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?

Gravatar

# re: Common issues found in code review 9/30/2008 7:13 AM 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)

Gravatar

# re: Common issues found in code review 9/30/2008 9:09 AM Ayende Rahien

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

Gravatar

# re: Common issues found in code review 9/30/2008 9:30 AM 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.

Gravatar

# re: Common issues found in code review 9/30/2008 10:30 AM 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))

Gravatar

# Golf and reducing nesting 9/30/2008 11:06 AM 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)

Gravatar

# re: Common issues found in code review 9/30/2008 12:51 PM Markus Zywitza

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

Gravatar

# re: Common issues found in code review 9/30/2008 3:36 PM 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.

Gravatar

# re: Common issues found in code review 9/30/2008 5:30 PM 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.

Gravatar

# re: Common issues found in code review 9/30/2008 7:39 PM 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 list)
{
string[] array = new List(list).ToArray();
}

Gravatar

# re: Common issues found in code review 9/30/2008 8:29 PM 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);

Gravatar

# re: Common issues found in code review 10/1/2008 12:32 PM 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.

Gravatar

# re: Common issues found in code review 10/2/2008 2:19 PM Benny Thomas

Good call, Daniel

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

Gravatar

# re: Common issues found in code review 10/29/2008 10:45 AM 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.