+972 52-548-6969
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.
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...
Comments
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.
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.
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?
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)
Phil,
Yes, I do find it more readable, and that is why I am doing it
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 (queue.Any(w => w.IsBatch))
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)
{
}
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)
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)
if(!someCondition)
{
}
more code
to be more readable than
if(someCondition)
{
}
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.
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(list).ToArray();
@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.
Good call, Daniel
I didn't see the workspace.IsBatch problem, but this may be a preemptive strike and not needed.
{} 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