Re: Why Is This Bad Code?

From: David (FlyLikeAnEagle_at_United.Com)
Date: 10/17/04 

Date: Sun, 17 Oct 2004 04:49:33 GMT

On Fri, 15 Oct 2004 23:28:58 UTC, "Phlip" <phlip_cpp@yahoo.com> wrote:

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

  When all I need are a few similar statements that aren't multi-line
that would be very close to my code as well. I'd also have the last
line line up as best as possible...

...
else if (c == 7) error_code = 3;
else error_code = call_function (a, b, c);
...

  I prefer this kind of style since it is very easy to eye-ball
it and see what should and shouldn't be there. It can also be
easy to add a line and be pretty sure you got it right. For
example, a misspelling of "error_xode" would stick out like a
sore thumb (whatever that looks like).

  As the code gets more complex I rewrite it as necessary
back in the traditional style. My emphasis is on speed
reading, understandability, and maintainability.

  The original code fragment is rather difficult to read
and comprehend quickly when scrolling or paging through the
code. It also has inconsistant use of several constructs.

  Our team has a good deal of legacy code and we have a
lot of styles to deal with. So long as we can be reasonably
sure we're making equivalent changes when correcting style
we normalize code in close proximity to other changes as
time permits. This allows the code to become more consistant
and for coding standards to evolve over time.

  David (and his $0.02 USD worth)

Relevant Pages

  • Re: ALTER design (Was: Code problems with Perform Thru Exit causes fall through)
    ... Or losing one's job. ... decent, sane, well-educated, clean-fingernailed programmer can see - then ... have to be re-run and one can lose customers because of dissatisfaction ... certain there was a Very Good Reason for that. ...
    (comp.lang.cobol)
  • Re: ANN: Q language website (new)
    ... that is the single best reason for adopting higher-level features - ... be simpler than in any other language. ... > implementation of the program would either match or beat the Lisp ... You need to be careful when you use the word "polymorphism" as it means ...
    (comp.lang.misc)
  • Re: Stroustrups FAQ causes infinite loop when executed
    ... There may be ways in which inheritance could ... programmer could not have foreseen. ... >> complex calculations such as multiplying numerous transformation ... >> reason to cap the class. ...
    (comp.lang.cpp)
  • Re: Why Is This Bad Code?
    ... The unnecessary use of indentation and else statements ... in C++ the cost of setting up such a map may exceed the cost of the ... What happens if the programmer ... polymorphism. ...
    (comp.lang.cpp)
  • Re: Contructing a dir. tree
    ... Perl programmer to think that variables must be initialized, ... The reason I set it to zero is for the same reason I set ... Again, while *you* may know the initialization is not needed, a novice ... he could be printing the Gutenberg Bible for every directory level. ...
    (comp.lang.perl.misc)