Why Is This Bad Code?

  • Thread starter Scott Brady Drummonds
  • Start date
S

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
 
D

David Hilsee

Scott Brady Drummonds said:
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.
 
P

Phlip

Scott said:
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.convertErrorCode();

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.
 
R

Rich Grise

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
 
P

Phlip

Phlip said:
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!
 
E

E. Robert Tisdale

Scott said:
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?
int call_function(int, 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);

}
 
P

Phlip

E. Robert Tisdale said:
int call_function(int, 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.
 
D

Daniel T.

Scott Brady Drummonds said:
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.
 
J

JXStern

int call_function(int, 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_assert(a <> 3, 1);
etc.

J.
 
E

E. Robert Tisdale

JXStern said:
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);

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.
 
J

JXStern

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.
 
D

David Hilsee

David Hilsee said:
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.

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.
 
E

E. Mark Ping

E. Robert Tisdale said:
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.

So exceptions aren't allowed?
 
P

Phlip

E. Mark Ping said:
So exceptions aren't allowed?

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

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

No members online now.

Forum statistics

Threads
473,744
Messages
2,569,484
Members
44,903
Latest member
orderPeak8CBDGummies

Latest Threads

Top