Why Is This Bad Code?

[Scott Brady Drummonds]

Hi, everyeone,

I recently stumbled on some code that someone else wrote that I don't like.
However, I'm having trouble articulating what the bad quality of the
following code is. The unnecessary use of indentation and else statements
seems to be counter-intuitive to me. However, I'm hoping for an argument
that is more formal than my intuition.

<quote>
// Make a function call based on each parameter not meeting certain
conditions.
if (a == 3)
error_code = 1;
else
{
if (b == 10)
error_code = 2;
else {
if (c == 7)
error_code = 3;
else
error_code = call_function(a ,b,c);
}
}

return error_code;
</quote>

Personally, I find all of the else statements distracting. Is there
anything that you don't like about the organization of this simple code?

Thanks,
Scott

--
Remove .nospam from my e-mail address to mail me.

http://www.e-scott.net

[David Hilsee]

Re: Why Is This Bad Code?

"Scott Brady Drummonds" <scott.b.drummo nds.nospam@inte l.com> wrote in
message news:ckpl4k$c8e $1@news01.intel .com...[color=blue]
> Hi, everyeone,
>
> I recently stumbled on some code that someone else wrote that I don't[/color]
like.[color=blue]
> However, I'm having trouble articulating what the bad quality of the
> following code is. The unnecessary use of indentation and else statements
> seems to be counter-intuitive to me. However, I'm hoping for an argument
> that is more formal than my intuition.
>
> <quote>
> // Make a function call based on each parameter not meeting certain
> conditions.
> if (a == 3)
> error_code = 1;
> else
> {
> if (b == 10)
> error_code = 2;
> else {
> if (c == 7)
> error_code = 3;
> else
> error_code = call_function(a ,b,c);
> }
> }
>
> return error_code;
> </quote>
>
> Personally, I find all of the else statements distracting. Is there
> anything that you don't like about the organization of this simple code?[/color]

I think it's better to reduce the indentation and write

if (a == 3)
error_code = 1;
else if (b == 10)
error_code = 2;
else if (c == 7)
error_code = 3;
else
error_code = call_function(a ,b,c);

For readability/maintainability reasons, I'd also add some braces
(if(...){}), but they aren't required.

--
David Hilsee

[Rich Grise]

Re: Why Is This Bad Code?

On Fri, 15 Oct 2004 16:05:55 -0700, Scott Brady Drummonds wrote:
[color=blue]
> Hi, everyeone,
>
> I recently stumbled on some code that someone else wrote that I don't like.
> However, I'm having trouble articulating what the bad quality of the
> following code is. The unnecessary use of indentation and else statements
> seems to be counter-intuitive to me. However, I'm hoping for an argument
> that is more formal than my intuition.
>
> <quote>
> // Make a function call based on each parameter not meeting certain
> conditions.
> if (a == 3)
> error_code = 1;
> else
> {
> if (b == 10)
> error_code = 2;
> else {
> if (c == 7)
> error_code = 3;
> else
> error_code = call_function(a ,b,c);
> }
> }
>
> return error_code;
> </quote>
>
> Personally, I find all of the else statements distracting. Is there
> anything that you don't like about the organization of this simple code?[/color]

I don't see anything "wrong" with the code, but do we all understand the
programmer's intent here? If a is 3, then you don't even check any of the
others, and if a's not 3, then you check b, and so on.

I suppose he could been more concise:
[color=blue]
> if (a == 3)
> error_code = 1; }
> else if (b == 10)
> error_code = 2;
> else if (c == 7)
> error_code = 3;
> else
> error_code = call_function(a ,b,c);[/color]

Cheers!
Rich

[Phlip]

Re: Why Is This Bad Code?

Scott Brady Drummonds wrote:
[color=blue]
> I recently stumbled on some code that someone else wrote that I don't[/color]
like.[color=blue]
> However, I'm having trouble articulating what the bad quality of the
> following code is. The unnecessary use of indentation and else statements
> seems to be counter-intuitive to me. However, I'm hoping for an argument
> that is more formal than my intuition.[/color]

Thank you for displaying greater than average sensitivity to low-quality
code.
[color=blue]
> <quote>
> // Make a function call based on each parameter not meeting certain
> conditions.
> if (a == 3)
> error_code = 1;
> else
> {
> if (b == 10)
> error_code = 2;
> else {
> if (c == 7)
> error_code = 3;
> else
> error_code = call_function(a ,b,c);
> }
> }
>
> return error_code;
> </quote>
>
> Personally, I find all of the else statements distracting.[/color]

If C++ supported an std::map that was slightly easier to initialize, we
could write something like this (represented in a language resembling Ruby):

map = { 3=>1, 10=>2, 7=>3 }
error_code = map.fetch(b, nil)
error_code = call_function(a ,b,c) if not error_code

However, in C++ the cost of setting up such a map may exceed the cost of the
chain of else statements.
[color=blue]
> Is there
> anything that you don't like about the organization of this simple code?[/color]

Why does c only get checked if a and b fail? What happens if the programmer
tries to change c's behavior? Could a, b, & c live inside a polymorphic
object? If there were some other reason to abstract them, then eventually
you might get (in C++):

error_code = abc.convertErro rCode();

Then the results vary based on the derived type of abc. If the program
containing that code depends on many similar if statements, scattered here
and there, then maybe many of them would go away with a careful use of
polymorphism.

If not, take out some excess delimiters, to help the code look like a table:

if (a == 3) error_code = 1;
else if (b == 10) error_code = 2;
else if (c == 7) error_code = 3;
else
error_code = call_function(a ,b,c);

The cruft is still there, but (in a monospaced font) you can at least scan
the columns and instantly see what's going on.

--
Phlip

http://industrialxp.org/community/bin/view/Main/TestFirstUserInterfaces

[Phlip]

Re: Why Is This Bad Code?

Phlip wrote:
[color=blue]
> If C++ supported an std::map that was slightly easier to initialize, we
> could write something like this (represented in a language resembling[/color]
Ruby):[color=blue]
>
> map = { 3=>1, 10=>2, 7=>3 }
> error_code = map.fetch(b, nil)
> error_code = call_function(a ,b,c) if not error_code
>
> However, in C++ the cost of setting up such a map may exceed the cost of[/color]
the[color=blue]
> chain of else statements.[/color]

We would also introduce some extra stuff to fetch with a, b, or c, too!

--
Phlip

http://industrialxp.org/community/bin/view/Main/TestFirstUserInterfaces

[E. Robert Tisdale]

Re: Why Is This Bad Code?

Scott Brady Drummonds wrote:
[color=blue]
> I recently stumbled on some code that someone else wrote that I don't like.
> However, I'm having trouble articulating
> what the bad quality of the following code is.
> The unnecessary use of indentation and else statements
> seems to be counter-intuitive to me.
> However, I'm hoping for an argument that is more formal than my intuition.
>
> <quote>
> // Make a function call based on each parameter not meeting certain
> conditions.
> if (a == 3)
> error_code = 1;
> else
> {
> if (b == 10)
> error_code = 2;
> else {
> if (c == 7)
> error_code = 3;
> else
> error_code = call_function(a ,b,c);
> }
> }
>
> return error_code;
> </quote>
>
> Personally, I find all of the else statements distracting.
> Is there anything that you don't like
> about the organization of this simple code?[/color]
[color=blue]
> cat f.cc[/color]
int call_function(i nt, int, int);
int f(int a, int b, int c) {

return ( 3 == a)? 1:
(10 == b)? 2:
( 7 == c)? 3: call_function(a , b, c);

}

[Phlip]

Re: Why Is This Bad Code?

E. Robert Tisdale wrote:
[color=blue][color=green]
> > cat f.cc[/color]
> int call_function(i nt, int, int);
> int f(int a, int b, int c) {
>
> return ( 3 == a)? 1:
> (10 == b)? 2:
> ( 7 == c)? 3: call_function(a , b, c);
>
> }[/color]

Another quality of good code is how easily one can change it. Even if the
stack of else statements that we recommended were just as complex as this
chain of ternary operators, changing that statement seems higher risk.

OTOH proper respects for putting the constants on the left of the ==
operator - I forgot that one.

--
Phlip

http://industrialxp.org/community/bin/view/Main/TestFirstUserInterfaces

[Daniel T.]

Re: Why Is This Bad Code?

In article <ckpl4k$c8e$1@n ews01.intel.com >,
"Scott Brady Drummonds" <scott.b.drummo nds.nospam@inte l.com> wrote:
[color=blue]
> Hi, everyeone,
>
> I recently stumbled on some code that someone else wrote that I don't like.
> However, I'm having trouble articulating what the bad quality of the
> following code is. The unnecessary use of indentation and else statements
> seems to be counter-intuitive to me. However, I'm hoping for an argument
> that is more formal than my intuition.
>
> <quote>
> // Make a function call based on each parameter not meeting certain
> conditions.
> if (a == 3)
> error_code = 1;
> else
> {
> if (b == 10)
> error_code = 2;
> else {
> if (c == 7)
> error_code = 3;
> else
> error_code = call_function(a ,b,c);
> }
> }
>
> return error_code;
> </quote>
>
> Personally, I find all of the else statements distracting. Is there
> anything that you don't like about the organization of this simple code?[/color]

It implies that the 'if (a==3)' is a more important comparison that any
of the others, when in fact they are all on the same level...

if ( a == 3 )
error_code = 1;
else if ( b == 10 )
error_code = 2;
else if ( c == 7 )
error_code = 3;
else
error_code = call_function( a, b, c );

even this implies that the comparisons are more important than assigning
to error_code, which is also (probably) incorrect...

error_code = (a==3) ? 1: (b==10) ? 2: (c==7) ? 3: call_function(a , b, c);

Of course the use of the ?: operator may upset some people, they may
even consider this obfuscation; but it succeeds in making the most
important part of the code, the most obvious. Try reading each one out
loud and you will see that the last snippet is the most coherent.

[JXStern]

Re: Why Is This Bad Code?

On Fri, 15 Oct 2004 16:45:24 -0700, "E. Robert Tisdale"
<E.Robert.Tisda le@jpl.nasa.gov > wrote:[color=blue][color=green]
> > cat f.cc[/color]
> int call_function(i nt, int, int);
> int f(int a, int b, int c) {
>
> return ( 3 == a)? 1:
> (10 == b)? 2:
> ( 7 == c)? 3: call_function(a , b, c);
>
> }[/color]

If indeed we do simply return afterwards, you can say:

if (a == 3) then return 1;
if (b == 10) then return 2;
if (c == 7) then return 3;

return call_function(a , b, c);

Though I usually set up an error system and write stuff like:

my_runtime_asse rt(a <> 3, 1);
etc.

J.

[JXStern]

Re: Why Is This Bad Code?

On Sat, 16 Oct 2004 00:15:07 GMT, JXStern <JXSternChangeX 2R@gte.net>
wrote:[color=blue]
>If indeed we do simply return afterwards, you can say:
>
> if (a == 3) then return 1;
> if (b == 10) then return 2;
> if (c == 7) then return 3;
>
> return call_function(a , b, c);[/color]

Maybe without the "then" will compile better.

J.
[color=blue]
>
>Though I usually set up an error system and write stuff like:
>
> my_runtime_asse rt(a <> 3, 1);
> etc.
>
>J.[/color]

[David Hilsee]

Re: Why Is This Bad Code?

"David Hilsee" <davidhilseenew s@yahoo.com> wrote in message
news:NLqdnVsvqe 8Tx-3cRVn-qA@comcast.com. ..[color=blue]
> "Scott Brady Drummonds" <scott.b.drummo nds.nospam@inte l.com> wrote in
> message news:ckpl4k$c8e $1@news01.intel .com...[color=green]
> > Hi, everyeone,
> >
> > I recently stumbled on some code that someone else wrote that I don't[/color]
> like.[color=green]
> > However, I'm having trouble articulating what the bad quality of the
> > following code is. The unnecessary use of indentation and else[/color][/color]
statements[color=blue][color=green]
> > seems to be counter-intuitive to me. However, I'm hoping for an[/color][/color]
argument[color=blue][color=green]
> > that is more formal than my intuition.
> >
> > <quote>
> > // Make a function call based on each parameter not meeting certain
> > conditions.
> > if (a == 3)
> > error_code = 1;
> > else
> > {
> > if (b == 10)
> > error_code = 2;
> > else {
> > if (c == 7)
> > error_code = 3;
> > else
> > error_code = call_function(a ,b,c);
> > }
> > }
> >
> > return error_code;
> > </quote>
> >
> > Personally, I find all of the else statements distracting. Is there
> > anything that you don't like about the organization of this simple code?[/color]
>
> I think it's better to reduce the indentation and write
>
> if (a == 3)
> error_code = 1;
> else if (b == 10)
> error_code = 2;
> else if (c == 7)
> error_code = 3;
> else
> error_code = call_function(a ,b,c);
>
> For readability/maintainability reasons, I'd also add some braces
> (if(...){}), but they aren't required.[/color]

I didn't answer your question. You said that you had a hard time
articulating what was wrong with the original code. After comparing the
improved code to the old code, it should be obvious that the flow and 4
potential outcomes are obfuscated by the excessive indentation in the old
code. Most programmers agree that indentation levels should be limited so
the function's execution paths are immediately obvious to the reader, and
many style guides contain rules that limit indentation levels for that
reason. For example, the Linux kernel style guide dictates that tabs are
eight character, and therefore code that exceeds three indentation levels
generally has to be "fixed" to make it readable. It's just harder to follow
code with many levels of indentation.

--
David Hilsee

[E. Robert Tisdale]

Re: Why Is This Bad Code?

JXStern wrote:
[color=blue]
> E. Robert Tisdale wrote:
>[color=green][color=darkred]
>> > cat f.cc[/color]
>> int call_function(i nt, int, int);
>> int f(int a, int b, int c) {
>>
>> return ( 3 == a)? 1:
>> (10 == b)? 2:
>> ( 7 == c)? 3: call_function(a , b, c);
>>
>> }[/color]
>
>
> If indeed we do simply return afterwards, you can say:
>
> if (a == 3) then return 1;
> if (b == 10) then return 2;
> if (c == 7) then return 3;
>
> return call_function(a , b, c);[/color]

No. They won't let you do that here.
The fatwa declares that
functions shall have a single point of return.
But that's a 'nother can of worms.

[E. Mark Ping]

Re: Why Is This Bad Code?

In article <ckppp9$mpi$1@n ntp1.jpl.nasa.g ov>,
E. Robert Tisdale <E.Robert.Tisda le@jpl.nasa.gov > wrote:
[color=blue]
>No. They won't let you do that here.
>The fatwa declares that
>functions shall have a single point of return.
>But that's a 'nother can of worms.[/color]

So exceptions aren't allowed?
--
Mark Ping
emarkp@soda.CSU A.Berkeley.EDU

[Phlip]

Re: Why Is This Bad Code?

E. Mark Ping wrote:
[color=blue]
> E. Robert Tisdale wrote:
>[color=green]
> >No. They won't let you do that here.
> >The fatwa declares that
> >functions shall have a single point of return.
> >But that's a 'nother can of worms.[/color]
>
> So exceptions aren't allowed?[/color]

The higher rule is "functions shall be short".

If you indulge in long ones, you need extra rules to help you mentally sort
out their concepts.

"Single exit" might have other reasons...

--
Phlip

http://industrialxp.org/community/bin/view/Main/TestFirstUserInterfaces

[Alf P. Steinbach]

Re: Why Is This Bad Code?

* Scott Brady Drummonds:[color=blue]
>
> I recently stumbled on some code that someone else wrote that I don't like.
> However, I'm having trouble articulating what the bad quality of the
> following code is. The unnecessary use of indentation and else statements
> seems to be counter-intuitive to me. However, I'm hoping for an argument
> that is more formal than my intuition.
>
> <quote>
> // Make a function call based on each parameter not meeting certain
> conditions.
> if (a == 3)
> error_code = 1;
> else
> {
> if (b == 10)
> error_code = 2;
> else {
> if (c == 7)
> error_code = 3;
> else
> error_code = call_function(a ,b,c);
> }
> }
>
> return error_code;
> </quote>
>
> Personally, I find all of the else statements distracting. Is there
> anything that you don't like about the organization of this simple code?[/color]

1) Indentation that shouldn't be there.
2) Inconsistent placement of braces.
3) Lacking braces around if-part and else-part.
4) Superflous variable (assuming error_code isn't global which would be
even worse).
5) Single-letter variable names.
6) "Magic numbers" (literal constants) sprinkled like pepper.
7) Error code instead of exceptions and asserts.

The 'else's are OK and should be there, as far as I'm concerned. They
prevent a common problem with maintainance where new statements are
inserted between if's. We want them to have to work harder to screw up,
to get compilation errors for the most common screw-up modes.

Regarding 1, 2, 3 and 4, try (adjust to your favorite formatting)

if ( a == 3 ) { return 1; }
else if( b == 10 ) { return 2; }
else if( c == 7 ) { return 3; }
else { return call_function( a, b, c ); }

or

return (
a == 3? 1 :
b == 10? 2 :
c == 7? 3 :
call_function( a, b, c )
);

but keep in mind that 7, exceptions, can make things simpler, e.g.

throwIf( a == 3 ); throwIf( b == 10 ); throwIf( c == 7 );
call_function( a, b, c );

--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?