|
Dear Sirs,
I just came across some code and changed it. I'm implementing the java Gridbag layout manager to a panel in C#, and in the Insets.Equals(object) code, the following could be found:
if (obj instanceof Insets) {
Insets insets = (Insets)obj;
return ((top == insets.top) && (left == insets.left) &&
(bottom == insets.bottom) && (right == insets.right));
}
return false;
So I sharpified it:
if (obj is Insets)
{
Insets insets = obj as Insets;
return ((top == insets.top) && (left == insets.left) && (bottom == insets.bottom) && (right == insets.right));
}
return false;
Then, almost compulsively, I modified it thus:
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 do this kind of thing ALL the time. I would like some comments on readability and efficiency.
I guess to characterize the modification, I would say that I make it read more sequentially, without having to skip code (casting your eyes over code because in certain cases it would not be executed). Also, I'm crazy about conserving lines. I suppose that's because I'm not paid per line  . I always use the same-line `if' if I can, and if not, I resort to the no-brace 'if'. Which sometimes means...oh yeah, let me show a line I wrote the other day. Here's what I first wrote:
foreach (Point a in _gr_pts)
{
if (prev == Point.Empty || prev == a)
{
prev = a;
continue;
}
g.DrawArc(_ap_pen, prev, a);
prev = a;
}
Then, I changed it to:
foreach (Point a in _gr_pts)
{
if (prev == Point.Empty || prev == a)
{
prev =a;
continue;
}
g.DrawLine(_ap_pen, prev, prev = a);
}
I tried for a while to figure out how to get the braces out of the if statement...oh yeah, I just realized that this is possible:
(I'm writing free-hand, so there may be typos...)
foreach (Point a in _gr_pts)
if (prev == Point.Empty || prev == a)
{
prev = a;
continue;
}
else g.DrawLine(_ap_pen, prev, prev = a);
I know some people swear by ALWAYS using braces in if, else, do, while, for, foreach statements, (and if I remember correctly, Visual studio has some option about including those compulsively) and I am a little curious if any of you are that way, or how many would do what I do.
To sum up, some items on which to comment:
I cast the object first without asking about the cast. I use ReferenceEquals, not `=='. I return false inline with the if-statement; I don't use else.
When I say things like `I don't use else,' I don't mean all the time, but I don't use it if I don't need it. Does anyone know if this affects effeciency in any way? I suppose I'm too lazy to check out the disassembly (because it's always SOO confusing  ).
I look forward to hearing back from you, my esteemed colleagues.
In Christ,
Aaron Laws
|
|
|
|
|
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.
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
