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]

"Scott Brady Drummonds" <sc************ **********@inte l.com> wrote in
message news:ck******** **@news01.intel .com...

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?

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]

On Fri, 15 Oct 2004 16:05:55 -0700, Scott Brady Drummonds wrote:

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

Cheers!
Rich

[Phlip]

Scott Brady Drummonds wrote:

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.
Thank you for displaying greater than average sensitivity to low-quality
code.
<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.
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.
Is there
anything that you don't like about the organization of this simple code?

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/bi...UserInterfaces

[Phlip]

Phlip wrote:

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.

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

--
Phlip
http://industrialxp.org/community/bi...UserInterfaces

[E. Robert Tisdale]

Scott Brady Drummonds wrote:

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? cat f.cc

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]

E. Robert Tisdale wrote:

> cat f.cc

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

}

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/bi...UserInterfaces

[Daniel T.]

In article <ck**********@n ews01.intel.com >,
"Scott Brady Drummonds" <sc************ **********@inte l.com> wrote:

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?

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]

On Fri, 15 Oct 2004 16:45:24 -0700, "E. Robert Tisdale"
<E.************ **@jpl.nasa.gov > wrote:

> cat f.cc

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

}

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]

On Sat, 16 Oct 2004 00:15:07 GMT, JXStern <JX************ **@gte.net>
wrote:

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);
Maybe without the "then" will compile better.

J.

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

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

J.