Code Readability Poll

LimitedAtonement

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;
}
//Crazy, I know.
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 littl

Kevin Marois

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

Luc Pattyn

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!

N a v a n e e t h

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

Navaneeth How to use google | Ask smart questions

Not Active

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

Kevin Marois

Don't hold back. Tell him what your really think.

Everything makes sense in someone's mind

N a v a n e e t h

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

Navaneeth How to use google | Ask smart questions

PIEBALDconsult

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 :cool: ). 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 returns in a method:

LimitedAtonement

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

PIEBALDconsult

No need for the trinary operator. Otherwise, 5.

N a v a n e e t h

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

Navaneeth How to use google | Ask smart questions

PIEBALDconsult

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.

N a v a n e e t h

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

:)

Navaneeth How to use google | Ask smart questions

Not Active

N a v a n e e t h wrote:

There is no casting

You're right, poor choice of words on my part. :-O You explained what I was meant though

only two letters away from being an asset

PIEBALDconsult

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.

Not Active

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

PIEBALDconsult

Hear hear!

PIEBALDconsult

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.

PIEBALDconsult

foreach (Point a in _gr_pts)
{
if (prev != Point.Empty && prev != a)
{
g.DrawLine(_ap_pen, prev, a);
}

prev = a;

}

Ennis Ray Lynch Jr

East Coast Brackets, I like the term. I usually just call it Java-style but yours is more fun.

Need custom software developed? I do custom programming based primarily on MS tools with an emphasis on C# development and consulting. A man said to the universe: "Sir I exist!" "However," replied the universe, "The fact has not created in me A sense of obligation." --Stephen Crane