|
I've not read enough books (or this one by Fowler) to know if other people say it, but I would personally prefer to use multiple return points. The reality is that in some cases, such as when you have cleanup code that must run and a try-finally isn't appropriate, you may need to have only one return point. Other than that, there is little to no mental benefit to doing it either way. When I read this,
bool bReturn;
if (condition)
{
bReturn = true;
}
else
{
if (other condition)
{
bReturn = false;
}
else
{
bReturn = true;
}
}
return bReturn;
I still have to trace through the same amount of code to know what the function will return each case, whether I use single or multiple returns. Allowing multiple returns makes it obvious when some condition will stop execution of the method immediately. If I must have a single return, these conditions end up causing if-else statements that I have to trace to the end of the method before realizing that, "Hey, this condition stops the method." This will also usually end up leading to deeper nesting, which is more difficult to read.
|
|
|
|
|
Yes, that example may well be a good place for multiple returns, but it may also simply be a bad piece of code too.
My concern about having multiple returns is that it's too close to:
// do stuff
if ( exp ) goto end ;
// more stuff
end:
return whatever ;
In my opinion, multiple-return should be as uncommon as goto (that goes for continue and break (in loops*) as well).
Multiple-return, continue, break, and goto are the right tool for some jobs, but they shouldn't be the developer's first choice.
* Break and goto are required in switch statements  .
|
|
|
|
|
Hi,
IMO readability is very important; it helps in getting correct code that is easily maintained.
Code generated and performance tend to be equal or better when you improve readability.
LimitedAtonement wrote: Insets insets = obj as Insets;
if (ReferenceEquals(insets, null)) return false;
return ((top == insets.top) && (left == insets.left) && (bottom == insets.bottom) && (right == insets.right));
I seldom use "is", often "as", hardly ever ReferenceEquals, hence:
Insets insets=obj as Insets;
return insets!=null && insets.top==top && insets.left=left && insets.bottom==bottom && insets.right==right;
LimitedAtonement wrote: foreach (Point a in _gr_pts)
{
if (prev == Point.Empty || prev == a)
{
prev = a;
continue;
}
g.DrawArc(_ap_pen, prev, a);
prev = a;
}
isn't that just:
foreach (Point a in _gr_pts) {
if (prev!=Point.Empty && prev!=a) g.DrawArc(_ap_pen, prev, a);
prev=a;
}
You may have noted:
- I skip most irrelevant spaces
- I use "East Coast brackets", i.e. they open without a new line
- I use parentheses even when not needed to make sure about operator precedence but only if you could be mistaken.
- I don't use negative if conditions, unless there is no else, or the negative block is much much shorter than
the positive block (as in if (!OK) {throw} else {lots of code})
Luc Pattyn
I only read code that is properly indented, and rendered in a non-proportional font; hint: use PRE tags in forum messages
Local announcement (Antwerp region): Lange Wapper? Neen!
|
|
|
|
|
East Coast Brackets, I like the term. I usually just call it Java-style but yours is more fun.
|
|
|
|
|
I never associated my bracket placement with Java, and I remember having had similar east/west discussions when being an active Java developer. I checked a few Java books and found both styles in use.
I've got the East Coast name long time ago, can't remember where from; and Google seems to have forgotten all about it too. But I continue to use and apply it.
Luc Pattyn
I only read code that is properly indented, and rendered in a non-proportional font; hint: use PRE tags in forum messages
Local announcement (Antwerp region): Lange Wapper? Neen!
|
|
|
|
|
They're why I left Boston for California. 
(Not really.)
|
|
|
|
|
LimitedAtonement wrote: I use ReferenceEquals, not `=='
You don't get any advantage of doing this. Objects are by default compared by reference. Your first sharpified code looks good. IMO, readability of code is subjective. I'd write something like
Insets insets = obj as Insets;
return insets == null ? false : ((top == insets.top) && (left == insets.left)
&& (bottom == insets.bottom) && (right == insets.right));
|
|
|
|
|
No need for the trinary operator. Otherwise, 5.
|
|
|
|
|
I too thought the same after posting. Probably more cleaner approach would be
Insets insets = obj as Insets;
if(insets == null)
return false;
return ((top == insets.top) && (left == insets.left) && (bottom == insets.bottom) && (right == insets.right));
|
|
|
|
|
The only use I've found for ReferenceEquals is null checking in classes that overload operators == and !=.
|
|
|
|
|
Dave,
But why would a class overload the == operator and not handle null itself? Do you see this often? I also thought this, but reconsidered recently. The reason I was using ReferenceEquals is because I don't want to have to check to see if it has an overload, then check to see if it returns properly with null.
In Christ,
Aaron Laws
|
|
|
|
|
I meant within the class itself. Consider this int wrapper class (I've just written it out quickly so is very rough!) and compare the two different == operator overloads...
public class MyIntWrapperClass : IEquatable<MyIntWrapperClass>
{
private int _Value;
public MyIntWrapperClass(int value)
{
_Value = value;
}
public static bool operator ==(MyIntWrapperClass a, MyIntWrapperClass b)
{
bool result = false;
if (!ReferenceEquals(a, null))
{
if (!ReferenceEquals(b, null))
result = a._Value == b._Value;
else
result = false;
}
else
result = ReferenceEquals(b, null);
return result;
}
public static bool operator !=(MyIntWrapperClass a, MyIntWrapperClass b)
{
return !(a == b);
}
public int Value
{
get { return _Value; }
}
public override bool Equals(object obj)
{
return Equals(obj as MyIntWrapperClass);
}
public bool Equals(MyIntWrapperClass other)
{
return this == other;
}
public override int GetHashCode()
{
return _Value; ;
}
public override string ToString()
{
return _Value.ToString();
}
}
modified on Wednesday, October 7, 2009 5:24 PM
|
|
|
|
|
LimitedAtonement wrote: I do this kind of thing ALL the time
So your job is to edit and refactor code?
From a performance and readability perspective I saw nothing wrong with the first snippet, except the casting the object twice (once for the is comparison and once for the as). If you had been working for me I'd question why you were wasting time like this.
All of the coding standards I have used call for curly braces with control statements and one statement per line. It is not only more readable but helps when debugging to see what statement is being executed.
Not checking an object after doing a cast, or as in this case, is just plain stupid.
only two letters away from being an asset
|
|
|
|
|
Don't hold back. Tell him what your really think.
Everything makes sense in someone's mind
|
|
|
|
|
Mark Nischalke wrote: except the casting the object twice (once for the is comparison and once for the as)
There is no casting happens when is keyword is used. It just emits isinst IL instruction. Here is what MSDN[^] says:
Tests whether an object reference (type O) is an instance of a particular class.
In the code:
if (obj is Insets)
{
Insets insets = obj as Insets;
}
return false; two times isinst instruction will be emitted, one for obj is Insets and the other for obj as Insets because as checks the types before converting. The real conversion happens only when executing obj as Insets. This can be avoided by doing only obj as Insets and check for NULL.
This is micro optimization and in my opinion, no one should worry about such things.
|
|
|
|
|
N a v a n e e t h wrote: There is no casting
You're right, poor choice of words on my part.  You explained what I was meant though
only two letters away from being an asset
|
|
|
|
|
N a v a n e e t h wrote: This is micro optimization
No it isn't; the overall effect on performance may not be huge, but using both is and as on the same object is a sign that the developer doesn't understand what as does, and that's worse than the minimal hit to performance.
|
|
|
|
|
Dear Mr. Nischalke,
When I implement code from somewhere, that's when I wind up editing it. For instance, using the RectangleF structure, I noticed to get 'Bottom' to be a smaller number than 'Top' (exemplar gratia in an Euclidean space), I would have to rewrite the code. When copying and pasting code from the internet, when looking over my old code, that's what I mean by all the time.
What do you mean, <quote class="FQ">Not checking an object after doing a cast? I think I checked it with ReferenceEquals .
Do you mean that revisiting code that works is a waste of time? Or refactoring Java to C# is a waste? Or casting the object twice (in is and in as) is the waste?
In Christ,
Aaron Laws
|
|
|
|
|
IMO, this is more readable
if (insets == null)
than this
if (ReferenceEquals(insets, null))
LimitedAtonement wrote: I cast the object first without asking about the cast.
Not checking an object is unforgivable for a professional developer.
Refactoring must be balanced with need. Just because you can dosen't mean you should. The code may be working perfectly fine and the refactor introduces defects downstream. I've also had developers waste time refactoring when they had other tasks to perform. If you follow coding standards this sort of readbility refactoring won't be necessary.
only two letters away from being an asset
|
|
|
|
|
|
Mark Nischalke wrote: casting the object twice
Whether it casts twice or not, I agree there shouldn't be any need to use both is and as on the same object.
|
|
|
|
|
If it ain't broke don't fix it.
An organization should have coding standards and everyone should follow them. If you have a standard and you find a non-complying file you may fix it (and alert a superior who should endeavor to enlighten the wayward developer). If you have no standard, then don't reformat the code just to "make it more readable".
Any edits you make to the code should otherwise fit the existing format.
LimitedAtonement wrote: I do this kind of thing ALL the time
Then you're wasting time.
LimitedAtonement wrote: ReferenceEquals, not `=='.
For what purpose? == is likely to be overridden to perform what the developer of the class thinks is best, which may simply be ReferenceEquals anyway.
LimitedAtonement wrote: I'm crazy about conserving lines
For what purpose? Other than with compiler directives; you can write C/C++/C# with no line breaks at all. I don't see you doing that; obviously you recognize that whitespace enhances readability (I simply take it to a much higher level ). Back in high school when I was learning BASIC I endeavored to put the most statements on a line as I could -- the result is unreadable by humans. I've matured since then.
LimitedAtonement wrote: some people swear by ALWAYS using braces
I wouldn't say that I swear by it; I recognize that they're not required in some cases. The primary reason is if I need to add an additional statement later, perhaps just during debugging, I don't need to constantly add and remove BRACEs as I work on the code. As an added bonus, by always using them, (I feel) my code has a more cohesive look and feel. Another concern is line length; I've worked in shops where the coding standard specified a maximum line length of eighty characters, BRACEs yield an obvious place to break a line.
<Aside>In my opinion, the BRACEs should be required and the parentheses should not be. Currently, when writing an if statement, you are required to type at least two and at most six delimiters. By requiring the BRACEs rather than the parentheses, the maximim is reduced to four while retaining the minimum of two. Any of you who want to "save keystrokes" should see the logic in this.</Aside>
As to multiple <codelang="text">returns in a method:
They are rarely needed and should only be used when the alternatives are much less readable. In my opinion, single-return methods show that the developer has a more organized concept of program flow and doesn't take shortcuts.
As for the posted code; why not:
Insets insets = obj as Insets;
return ( (insets != null) && (top == insets.top) && (left == insets.left) && (bottom == insets.bottom) && (right == insets.right));
|
|
|
|
|
foreach (Point a in _gr_pts)
{
if (prev != Point.Empty && prev != a)
{
g.DrawLine(_ap_pen, prev, a);
}
prev = a;
}
|
|
|
|
|
I agree with the previous responses: readability is the important bit, and line spacing can seriously improve this - that is one reason why the "how to get an answer" bit at the top of the page stresses to use "code block" or <pre> tags to preserve formating.
Three points:
1) Whatever the company style is, that is the one you use. Making it all look the same is the easiest way to make it all readable - because if the company style makes it all harder to read, then everyone will complain, and the company style will get changed. If everyone codes to a personal style, then everything gets muddled and harder to cope with.
2) My personal preference is always to use braces for a loop of conditional statement. That way, it is much harder to get it wrong when adding a second staement to a single line conditional. This is a lot easier nowadays, with VS etc auto indenting etc., but it is still easier to see what is going on. (It is similar to the reason why C++ accepts "if(A = B)" while C# wont - avoidance of potential problems.)
3) I also indent braces to match the statements in the functional block - I consider them to be a single indented statement in effect. I really hate the open brace at the end of an if statement, with it's matching brace indented. It looks clumsy and makes it difficult yo find matching braces by eye. But, if that is the company style, then I use it. They write the cheques...
No trees were harmed in the sending of this message; however, a significant number of electrons were slightly inconvenienced.
This message is made of fully recyclable Zeros and Ones
|
|
|
|
|
Hear hear!
OriginalGriff wrote: I consider them to be a single indented statement in effect
They are one statement -- a compound statement. At least that's how it is in C/C++, it seems the C# guys didn't like that concept for some reason.
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
