Ayende @ Rahien

Mohammad Tayseer commented on More code review errors

Thu, 02 Oct 2008 01:40:28 GMT

@Manav The error case should be handled first, to free your mind from remembering if the error was handled or not. In C++, you shouldn't use goto Cleanup. Use destructors & smart pointers instead. You don't want your resource cleanup to be all over the place

Manav commented on More code review errors

Wed, 01 Oct 2008 16:42:36 GMT

Disagree whole-heartedly with multiple return value point. There is another easy way to refactor the code i..e using goto Cleanup. That is the standard way we use in C/C++ but I have not used C# so don't know if goto exists or not.

JonR commented on More code review errors

Wed, 01 Oct 2008 09:30:01 GMT

__Just because something compiles, doesn't mean it won't error. :rolleyes: i'll give you that, but i'm sure you see what i'm getting at.

Bruno commented on More code review errors

Tue, 30 Sep 2008 15:15:22 GMT

@meisinger String.Format is too much for simple string concatenations following the pattern 1 literal + 1 variable. It uses StringBuilder. You can check the stack trace causing an error due to bad formating.

meisinger commented on More code review errors

Tue, 30 Sep 2008 14:45:53 GMT

interesting... i would have tested "store" and "fieldBridge" first so that i wouldn't waste a call to "GetFieldPosition" lord only knows what that method is doing (i guess i have been bitten by too many "bad" calls in the past) the other thing that interests me is the fact that there is no check to see if "matchingPosition" is within the bounds of the "result" array lastly... (this is my personal "downfall") i would use a string.Format for the exception messages just my 2 cents

Ayende Rahien commented on More code review errors

Tue, 30 Sep 2008 14:29:49 GMT

Paul, I should have been more clear. I am showing the C# code because it is what most of my readers will understand. The fault lies in the Java code, I agree. It is bad in either language.

jonnii commented on More code review errors

Tue, 30 Sep 2008 13:50:26 GMT

I tend to put error conditions at the top of my methods, as they're the first thing I write tests for so my code ends up looking like this by default. I find it far easier to read.

Robert Taylor commented on More code review errors

Tue, 30 Sep 2008 12:07:31 GMT

The difference between the 1-line If-statements with vs. without braces is that for the ones without braces no code after those statements could ever possibly be executed. If someone needed to come along and add some additional statements it should be clear that it can't go *after* the throw statement in the same if-block.

Michael McCurrey commented on More code review errors

Tue, 30 Sep 2008 11:31:18 GMT

" __i *strongly* agree with all these points (and the ones in the previous post), but isn't it a little OTT to call them "errors"? i mean, if they really were errors, they wouldn't compile. " Just because something compiles, doesn't mean it won't error.

Paul Hatcher commented on More code review errors

Tue, 30 Sep 2008 10:47:33 GMT

Oren The reason I wrote it that way was that it is a direct port of the Java code and I'd prefer to keep the code bases the same until the port it complete. Once it's all there, then refactoring makes sense, but until it is, trying to figure out if someone's refactored code does the same job as the original Java can be really problematic.

Ayende Rahien commented on More code review errors

Tue, 30 Sep 2008 09:55:26 GMT

Frans, This method is called in one code path where the IFieldBridge impl should also be ITwoWayFieldBridge. If you don't enter this code path, you don't have to be IFieldBridge. That is why the check is made dynamically.

Frans Bouma commented on More code review errors

Tue, 30 Sep 2008 09:40:24 GMT

fieldBridge is casted twice to ITwoWayFieldBridge, one should use 'as' and test for null. It might be my C background, but I would test for < 0 instead of == -1. The most weird thing still is that the method header accepts IFieldBridge, but effectively crashes (exception) if the parameter isn't an ITwoWayFieldBridge, i.o.w.: the method should have its fieldBridge parameter be typed as ITwoWayFieldBridge and let the compiler deal with calls to the method. Also, 1 line if clauses should IMHO also be inside {}.

Markus Zywitza commented on More code review errors

Tue, 30 Sep 2008 09:38:29 GMT

This type of code usually exists because code isn't refactored. I don't know who wrote this code, but this was only red/green, not red/green/refactor. The human mind thinks like the first code snippet: Do the main options first and wrap them with the required conditionals and handle exceptional conditions after that. The latter code snippet is not how we think (well, at least not as I do): I don't think up all invariants and restrictions up front, but add them as if clauses as I go by. As a result, I refactor every method directly after I have finished it.

Lars Hundertwasser commented on More code review errors

Tue, 30 Sep 2008 09:17:39 GMT

I agree with Romain, put one liners in braces. At least you have to be consistent, i.e. you have 3 one liners w/o braces and 1 with...

Romain Verdier commented on More code review errors

Tue, 30 Sep 2008 09:02:28 GMT

I prefer using "!" and putting one liners in braces every time, but yes, guard clauses make code more easy to read/maintain.

JonR commented on More code review errors

Tue, 30 Sep 2008 08:50:45 GMT

i *strongly* agree with all these points (and the ones in the previous post), but isn't it a little OTT to call them "errors"? i mean, if they really were errors, they wouldn't compile.

Laurent Kempé commented on More code review errors

Tue, 30 Sep 2008 08:08:20 GMT

Thanks to Resharper those code review are easiness!

Ayende Rahien commented on More code review errors

Tue, 30 Sep 2008 07:56:00 GMT

comparing to false is the way I like to do it. As for the last, it just looks better to me that way

Frank Quednau commented on More code review errors

Tue, 30 Sep 2008 07:51:35 GMT

It's funny you're coming with those, yesterday I talked with 2 colleagues about just that kind of stuff you see. Quite often it is paired with the weird obsession to have only 1 exit point. People are out there who actually TALK like that. While being at it, any reason for comparing to false and not using '!' as well as any reason why the last if'd one-liner is in braces, while the other 1-liners aren't ? Cheers...:)

meowth commented on More code review errors

Tue, 30 Sep 2008 07:49:43 GMT

Simply detect and explicitly express preconditions instead of trying to keep one return point, yep?