Ayende @ Rahien

Marcel Popescu commented on Common issues found in code review

Wed, 29 Oct 2008 08:45:45 GMT

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

Benny Thomas commented on Common issues found in code review

Thu, 02 Oct 2008 11:19:49 GMT

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

Dag commented on Common issues found in code review

Wed, 01 Oct 2008 09:32:04 GMT

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

Daniel Crabtree commented on Common issues found in code review

Tue, 30 Sep 2008 17:29:37 GMT

@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 commented on Common issues found in code review

Tue, 30 Sep 2008 16:39:31 GMT

@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

J.P. Hamilton commented on Common issues found in code review

Tue, 30 Sep 2008 14:30:46 GMT

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.

Bryan commented on Common issues found in code review

Tue, 30 Sep 2008 12:36:13 GMT

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.

Markus Zywitza commented on Common issues found in code review

Tue, 30 Sep 2008 09:51:12 GMT

I'm not keen on replacing List

Rik Hemsley commented on Common issues found in code review

Tue, 30 Sep 2008 08:06:47 GMT

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)

Arild commented on Common issues found in code review

Tue, 30 Sep 2008 07:30:30 GMT

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

Daniel Crabtree commented on Common issues found in code review

Tue, 30 Sep 2008 06:30:02 GMT

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.

Ayende Rahien commented on Common issues found in code review

Tue, 30 Sep 2008 06:09:45 GMT

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

Hadi Hariri commented on Common issues found in code review

Tue, 30 Sep 2008 04:13:32 GMT

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 H commented on Common issues found in code review

Tue, 30 Sep 2008 03:54:38 GMT

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?

Skim commented on Common issues found in code review

Tue, 30 Sep 2008 03:03:42 GMT

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.

Agus Suhanto commented on Common issues found in code review

Tue, 30 Sep 2008 01:33:28 GMT

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.

PLamb commented on Common issues found in code review

Tue, 30 Sep 2008 00:29:52 GMT

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.