Style: Are these two blocks of code equivalent?

  • Thread starter Thread starter Michael McCarthy
  • Start date Start date
M

Michael McCarthy

This code is supposed to create a streamreader object from a stream
object and I suppose use encoding if requested (enc isn't null)... The
second bit is someone elses code...

/// code block #1
if (enc != null)
{
reader = new StreamReader(s, enc);
}
else
{
reader = new StreamReader(s);
}

/// code block #2
reader = (enc != null) ? new StreamReader(s, enc) : new StreamReader(s);

Okay, if they are the same, why would you use #2? To be cute? What is
this from, an old C++ hacking coder ethic?

thanks for any info.

M~
 
Why not simply calling

reader = new StreamReader(s, enc) ?

if encoding is null, the ctor behaves as if you would have called "new
StreamReader(s)".

As for your original question: style #2 is only used by newbies who are
trying to proof that they are able to understand complex syntax rules but no
professional programmer would write code like that because such code is
unmaintainable.
 
There is really no difference between the two.. The second is just a shorter
version of the first.

Michael Cierkowski
 
Michael said:
Okay, if they are the same, why would you use #2? To be cute? What is
this from, an old C++ hacking coder ethic?
Click to expand...

It's a matter of preference. The bits are exactly equivalent.

*Warning*, higly personal views below, they are likely to start vast
flame-wars :)

I personally dislike version 1, because of all the blank lines with
"{"'s on wasting my screen-space, making it harder to view more than a
few lines of code at a time.

I dicourage version 2, since it's really a bit cryptoc for many people.
It's a bit shorter, since it allows you do declare and initialize on the
same line and compresses the branch into an expression. Whether you can
(in a specific setting) use it depends largely on the audience. Some
people read this as well as "if...else", others don't:

StreamReader reader =
(enc == null) ? new StreamReader(s) : new StreamReader(s,enc);

In any case, I tend to prefer:

StreamReader reader;
if ( enc == null )
reader = new StreamReader(s);
else
reader = new StreamReader(s, enc);

Which is readable to most people, and has a nice structured layout,
while not taking up the whole screen for a simple choice.

I also like to avoid testing != null (if it doesn't disturb the
program-flow). I read "!= null" as "not undefined", which is a double
negation and there is no reason for complicating branches unnessesarily
with those ;)

But the *real* question here is, why are you accepting a pair of
(Stream, Reader)? could you not just accept a StreamReader instance
instead? The caller of you functionality probably *knows* whether he has
an encoding or not and can just create the proper StreamReader himself.
 
As for your original question: style #2 is only used by newbies who are
trying to proof that they are able to understand complex syntax rules but
no professional programmer would write code like that because such code is
unmaintainable.
Click to expand...

I'd have to respectfully disagree with your characterization of the
conditional operator in C# as both complex and unmaintainable. There's
nothing particularly complex about it. It merely returns one of two values
depending on the evaluation of a Boolean expression. It has been in the C
language family for at least twenty-five years - first in C, then in C++,
and now in C#. Professional programmers have been using it that long, and I
think that few if any professional programmers in this language family would
shy away from it because of any notion that it's unmaintainable. Unless
taken to extremes with large and complex Boolean expressions, it's an
elegantly simple way to evaluate a Boolean test and perform the appropriate
one of two expressions.

Here's a simple example of where it might save you several lines of code,
without loss of transparency of programmer intent: suppose you're preparing
some text for a status bar to report how many email messages your
application has processed, and you want to be grammatical and distinguish
between one message and multiple messages. This simple statement returns
either "1 message" or "N messages", depending on the value of a variable
named count:

String howMany = new String(count.ToString() + " message" + ( (count > 1) ?
"s" : "' );

Elegant, no? As opposed to, for example:

String howMany = count.ToString();
if (count > 1)
howMany += " messages";
else
howMany += " message";

Just consider it a regular part of your syntactic toolkit as a C#
programmer. Use it when it helps you do your job, and always strive for
readable and maintainable code.

Some programmers seem to delight in constructing the most complex and
difficult-to-understand code that they can manage, just to show off. Someone
like that could easily abuse the conditional operator, but they're hardly
behaving professionally when they do.

Tom Dacon
Dacon Software Consulting
a C/C++/C# programmer for twenty-three years, last time I checked
 
Tom Dacon said:
I'd have to respectfully disagree with your characterization of the
conditional operator in C# as both complex and unmaintainable. There's
nothing particularly complex about it. It merely returns one of two values
depending on the evaluation of a Boolean expression. It has been in the C
language family for at least twenty-five years - first in C, then in C++,
and now in C#. Professional programmers have been using it that long, and
I think that few if any professional programmers in this language family
would shy away from it because of any notion that it's unmaintainable.
Unless taken to extremes with large and complex Boolean expressions, it's
an elegantly simple way to evaluate a Boolean test and perform the
appropriate one of two expressions.

Here's a simple example of where it might save you several lines of code,
without loss of transparency of programmer intent: suppose you're
preparing some text for a status bar to report how many email messages
your application has processed, and you want to be grammatical and
distinguish between one message and multiple messages. This simple
statement returns either "1 message" or "N messages", depending on the
value of a variable named count:

String howMany = new String(count.ToString() + " message" + ( (count > 1)
? "s" : "' );

Elegant, no? As opposed to, for example:
Click to expand...

Not really, there is way too much in a single line in that example. Unless
you are *really* paying attention it isn't hard to miss that you are using a
condition. The operator can be an aide to clarity when used properly, but I
do not think it is a good thing nested within complex expressions or when
the expressions within it are too complex.
 
cody said:
Why not simply calling

reader = new StreamReader(s, enc) ?

if encoding is null, the ctor behaves as if you would have called "new
StreamReader(s)".
Click to expand...

No, if the encoding is null, you get an ArgumentNullException.
As for your original question: style #2 is only used by newbies who are
trying to proof that they are able to understand complex syntax rules but no
professional programmer would write code like that because such code is
unmaintainable.
Click to expand...

I disagree - I wouldn't *mind* using that code, although I'd prefer:

reader = new StreamReader (s, enc==null ? Encoding.UTF8 : enc);

I don't generally like having a lot of text in any of the ternary
operands.
 
Professional coders would and do use it. In fact as a hiring manager, I
certainly wouldn't hire anyone who made a statement like yours in an
interview or didn't know about it.

--
Bob Powell [MVP]
Visual C#, System.Drawing

Find great Windows Forms articles in Windows Forms Tips and Tricks
http://www.bobpowell.net/tipstricks.htm

Answer those GDI+ questions with the GDI+ FAQ
http://www.bobpowell.net/faqmain.htm

All new articles provide code in C# and VB.NET.
Subscribe to the RSS feeds provided and never miss a new article.
 
Bob said:
Professional coders would and do use it. In fact as a hiring manager, I
certainly wouldn't hire anyone who made a statement like yours in an
interview or didn't know about it.
Click to expand...

The fact that 'professional coders', (I'm professional, I'm self
employeed, that doesn't mean much), would and do use it doesn't make it
very good stylistically... It seems overtly terse and cryptic. There's
no real point in "pretty code", unless you are just trying to show off
how clever you are, and generally speaking that's when things break.
I'm all for screen readibility, you don't have to be superfluous, but
this is the exact opposite, and if you read any style guides almost any
will tell you to avoid things like this, along with something like

while((suchandsuch = action() ) != null)....

....but people still do it.

If you are the only one who is going to use your code, do what you like,
but I would think a person in your position would want to have a
codebase that was easily readable by anyone just perusing it... it's not
a question of if they are familiar with the logic, the question was
more, why would you use a cryptic method over a fairly readable one, and
I myself, chose to hire out programmers who make code that can be
legible if say, they get hit by a bus the next day.

~m.
 
I'd have to respectfully disagree with your characterization of the
conditional operator in C# as both complex and unmaintainable. There's
nothing particularly complex about it.
Click to expand...

I wasn't talking about the ternary operator itself just about the given
example.
 
Professional coders would and do use it. In fact as a hiring manager, I
certainly wouldn't hire anyone who made a statement like yours in an
interview
Click to expand...

Imho the ?: should only be used in short expressions, but maybe its just a
matter of taste. You will certainly agree that method #1 or jons suggestion
"new StreamReader (s, enc==null ? Encoding.UTF8 : enc)" are far more
readable, at least for me they are.
or didn't know about it.
Click to expand...

Not knowing about the ternary operator? Thats for sure :)
 
Not really, there is way too much in a single line in that example. Unless
you are *really* paying attention it isn't hard to miss that you are using a
condition. The operator can be an aide to clarity when used properly, but I
do not think it is a good thing nested within complex expressions or when
the expressions within it are too complex.
Click to expand...

Well, everyone has their own tolerance for complexity.

One thing I personally dislike is code that runs off the right side of the
editing window. I tend to break up code into multi-line blocks in that
situation just so that I can see it all.

When I see one of my peeps doing a good job of using something like the
conditional operator, I take it as a sign that they're comfortable with more
than just the basics of the language. On the other hand, if I catch one of
them writing complex expressions, or even worse nesting them within a single
statement, for example, I take the opportunity to do a little education on
code readability and maintainability.

In Einstein's words: "Make things as simple as possible...but no simpler."

Tom Dacon
Dacon Software Consulting
 
Michael McCarthy said:
The fact that 'professional coders', (I'm professional, I'm self
employeed, that doesn't mean much), would and do use it doesn't make it
very good stylistically... It seems overtly terse and cryptic. There's
no real point in "pretty code", unless you are just trying to show off
how clever you are, and generally speaking that's when things break.
I'm all for screen readibility, you don't have to be superfluous, but
this is the exact opposite, and if you read any style guides almost any
will tell you to avoid things like this, along with something like

while((suchandsuch = action() ) != null)....

...but people still do it.

If you are the only one who is going to use your code, do what you like,
but I would think a person in your position would want to have a
codebase that was easily readable by anyone just perusing it... it's not
a question of if they are familiar with the logic, the question was
more, why would you use a cryptic method over a fairly readable one, and
I myself, chose to hire out programmers who make code that can be
legible if say, they get hit by a bus the next day.
Click to expand...

But in the case of your example, it tends to be the idiomatic, *most
commonly* used way of iterating in some cases. For instance, when
reading lines from a StreamReader, I would certainly use:

string line;
while ( (line=reader.ReadLine()) != null)
{
// do something with line
}

over the alternatives. What would you use as a more readable form,
given that the above is well-known?
 
Jon Skeet [C# MVP] wrote:

over the alternatives. What would you use as a more readable form,
given that the above is well-known?
Click to expand...

I don't mind the "while-assign" solotion that much, but there are
alternatives.

For example, I don't particularly mind the for-variation, although it
duplicates the action for progressing:

for ( string line = reader.ReadLine();
line != null;
line = reader.ReadLine() ) {
...
}

Specificly, the read-by-line is so common that you could defend writing
a helper for it:

public class Read {
public class ReadByLine: IEnumerable {
readonly TextReader Reader;
public ReadByLine(TextReader reader) { this.Reader = reader; }
class Enumerator: IEnumerator() {
readonly TextReader Reader;
string Line;
public Enumerator(TextReader reader) { this.Reader = reader; }
public object Current { get { return Line; } }
public bool MoveNext() {
Line = Reader.ReadLine();
return Line != null;
}
}
public IEnumerator GetEnumerator() { return new Enumerator(); }
}
public static ReadByLine ByLine(TextReader reader)
{ return new ReadByLine(reader); }
}

or with yield:

class Read {
public static IEnumerable<string> ByLine(TextReader reader) {
// your choice of loop here, atleast it's only one place
// if it's ugly
for ( string line = reader.ReadLine();
line != null;
line = reader.ReadLine() )
yield return line;
}
}

Both helpers fits the faboulously readable "foreach":

foreach ( string line in Read.ByLine(reader) ) {
...
}
 
Helge Jensen said:
I don't mind the "while-assign" solotion that much, but there are
alternatives.

For example, I don't particularly mind the for-variation, although it
duplicates the action for progressing:

for ( string line = reader.ReadLine();
line != null;
line = reader.ReadLine() ) {
...
}
Click to expand...

Yes, the duplication is nasty here - although it does have the
advantage of declaring the variable within the scope of the loop.
Specificly, the read-by-line is so common that you could defend writing
a helper for it:
Click to expand...

<snip>

True - I've considered doing this before, in fact. I think I was
considering it in Java though, where there are problems in terms of the
exceptions an enumerator can throw...
 
Back
Top