|
Aaron,
I teach C# at a local JC, and one thing I always stress is readability. White space and well defined blocks of
code are good because it allows your mind to process each item individually line by line, whereas this
Insets insets = obj as Insets;
if (ReferenceEquals(insets, null)) return false;
return ((top == insets.top) && (left == insets.left) && (bottom == insets.bottom) && (right == insets.right));
forces me to stop and pick apart these lines to digest all that is happening.
I find this to me much clearer
if (obj is Insets)
{
Insets insets = obj as Insets;
return ((top == insets.top) && (left == insets.left) && (bottom == insets.bottom) && (right == insets.right));
}
return false;
Also, one other thing we teach is having only one return in a method. It prevents "spaghetti code" where you
have to really hunt to see where the method is actually ending. So condider this:
bool bReturn = false;
if (obj is Insets)
{
Insets insets = obj as Insets;
bReturn = ((top == insets.top) && (left == insets.left) && (bottom == insets.bottom) && (right == insets.right));
}
return bReturn;
So now if I wanted to put more code in the method that I know will always run, all I have to do is find
the return & put the code above it.
Just my humble opinion.
Everything makes sense in someone's mind
|
|
|
|
|
Kevin Marois wrote: Also, one other thing we teach is having only one return in a method
IMO, this makes the code hard to read. Multiple return is the preferred approach. Martin Fowler has a topic on this in his Refactoring book. I don't have it right now to quote.
|
|
|
|
|
N a v a n e e t h wrote: Martin Fowler
But is he the only one who says that? I prefer to make my own decisions rather than follow the word of one author.
|
|
|
|
|
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));
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
