Code Craft
The art, science and craft of writing quality software

Aug. 3, 2005 - Evil Code

Posted in Articles

To paraphrase Supreme Court Justice Potter Stewarts, "I shall not today attempt further to define {evil code} ... but I know it when I see it!" 

 

At one time or another each of us has encountered code that is unreadable, hacked-to-hell, bloated-beyond-belief, or otherwise just plain evil.  Depending on our time, inclination and skill we may be forced to run away in fear, hide it away with a wrapping layer, or unravel a piece or two and make a change.  On a good day, if we are feeling appropriately "lazy", we may even bite the bullet and rewrite the monster so we won't be forced to gaze upon it again.  Whatever we do when we find it, when it comes to spotting really bad code almost anyone can do it.

 

If spotting bad code is so easy why is "bad code" so hard to define?  The short answer is that programming appears to be more like a craft than a strict engineering discipline.  Like other crafts, writing code requires familiarity with an enormous variety of tools and skills each of which may be appropriate in some cases and not appropriate in others.  It requires judgment.

 

The formost reference on style in programming is still Kernighan and Plauger's classic book The Elements of Programming Style.  Many of its ideas like avoiding unnecessary branches and using variable names that have meaning are basic assumptions people don't even question any more.  At the same time, programming has evolved since 1974 and much of it feels dated.  Concepts like avoiding the Fortran arithmetic IF are hardly useful to most people (those who do find it useful might consider re-reading the section about rewriting bad code rather than fixing it).  Notions about class structure and encapsulation are naturally non-existent since object oriented notions had not yet fully emerged.  Despite its many virtues it's far from covering many of the symptoms of "bad-ness" found in day to day code.  It does not attempt to.  It treats programming like writing and offers guidelines that can be understood and followed differently by different people.

 

Other modern books on patterns or refactoring or countless other topics are also far from providing a clear step-by-step guide to code nirvana.  For every pattern you will find three alternatives and for many the pattern is its own antipattern.  The Singleton Pattern is also frequently known as the Singleton Antipattern.  Likewise, the Value Object Pattern is sometimes the Value Object Antipattern.  This is not to say that such books are useless.  In fact, they are invaluable; it's just that there does not appear to be a simple formula that can be applied in a cookie-cutter fashion to produce code that is good.

 

Naturally if we can't clearly and consistently define what good code is (and there is no reason to believe we can) it's hardly surprising that many people find it difficult to produce.  One good example should suffice to make it clear why this is so.  Consider the following code snippet in some arbitrary language:

// determine the productivity of a farm

int function getProductivity( Farm a)

{

   int t;

   if (a != null && (1 < (a.eggs / a.hens) || 4 > (a.eggs / a.hens)))

      t = 2;

   else if (a != null && (a.eggs / a.hens) >= 4)

      t = 3;

   else if (a != null)

      t = 1;

   return (a == null) ? 0 : t;

}

This is clearly awful code.  No doubt about it, a real stinker.  Anyone at all can find several immediate improvements.  Let's try to clean it up.  How about this version?

// determine the productivity of a farm

int function getProductivity( Farm a)

{

   if (a == null) return 0;

   return Math.min((a.eggs / a.hens) / 2, 3);

}

The new version is shorter and it superficially appears to have done the same thing [at least if we accept the notion that negative eggs and negative hens can't exist], but is it a bit too clever?  This whole division by 2 piece and then the Math.min seems to hide the meaning.  How about this version?

// determine the productivity of a farm

int function getProductivity( Farm a)

{

   if (a == null) return 0;

   int eggsPerHen = a.eggs / a.hens;

   if (eggsPerHen < 2) return 1;

   if (eggsPerHen > 3) return 3;

   return 2;

}

This is easier to read and a more faithful replacement for the behavior of the first function (since it allows for negative hens and eggs).  There are, however, a great many things wrong with it.  For example, some people would argue on principle that there should be only one return statement and would rewrite it as follows:

// determine the productivity of a farm

int function getProductivity( Farm a)

{

   int rtn = 0;

   if (a != null)

   {

      int eggsPerHen = a.eggs / a.hens;

      if (eggsPerHen < 2) rtn = 1;

      else if (eggsPerHen > 3) rtn = 3;

      else rtn = 2;

   }

   return rtn;

}

Actually I prefer the previous version with multiple returns.  It is simple enough that the reader will not be confused.  The issue of multiple returns does not make much difference here and I view it as largely an issue for more substantial code chunks (to be avoided on principle anyway).  Aside from the tremondously lage number of potential nits about style, use of block statements, single-line-if-statments and the like, there are still a number of very substantial issues with this snippet (small though it is).  Let me rework it a bit and see if it can be improved further:

// class that describes the workings of a farm

class Farm ..original syntax..

{

   ..original Farm source..

 

   Enumeration HenProductivityLevel

   {

      Low = 1,

      Medium = 2,

      High = 3

   }

 

   HenProductivityLevel getHenProductivity()

   {

      int eggsPerHen = eggs / hens;

      if (eggsPerHen < 2) return HenProductivityLevel.Low;

      if (eggsPerHen > 3) return HenProductivityLevel.High;

      return HenProductivityLevel.Medium;

   }

}

Many people reading this probably already realized that it is generally a bad idea to have a function operate against the inner data of a passed object like the earlier versions did.  Also, why should we be returning these clearly artificial "productivity level" integers?  All languages have some better way to represent enumerations.  At very least it will have some kind of #define or static final int, or whatever.  By now most of you have also probably spotted the obvious possibility that the number of hens could be 0 (causing some kind of run time error).  If this language supports exceptions we could throw one, but maybe it would be better to just return Low productivity or perhaps a new value of 'Unknown' that can be interpreted by the caller.  I'd generally advocate this latter approach for such a simple method since having 0 hens seems like a likely possibility.  All of this would be highly context sensitive (would it break a lot of existing code, if so perhaps we should be forced to examine all the uses?).  Oh, and should we use the hens and eggs member variables or should we be calling getEggs() and getHens() [or perhaps this language provides a clean automated syntax that makes the use of such obtainers unnecessary] and shouldn't the variables be named numberOfHens and numberOfEggs, and what about comments?

 

Of course once we get to context another litany of possibilities opens up including replacing the "magic" threshold values of two and three with definable values or properties.  And why just three levels, what if people want a five valued differentiator, maybe we should code for that too?  Those of you with an agile programming background are by now thinking "You Aren't  Going to Need It" and others are thinking "yes, why not make that a property in case our initial guesses about what a 'High' level of productivity are turn out to be wrong?"  The answer to which of these perspectives is right probably depends on much greater application context.

 

The point is this: even with such a simple code snippet the definition of bad code and good code has a lot of variability based on application context, the tools in use, and even stylistic preference.  At the same time there is little doubt that the last piece of code is a lot better than the first piece.  Some of the reasons the first piece of code is bad are generalizable, but others are much harder to pin down.  We can't be Nihilistic about code quality just because we can't get 100% consensus or a mechanistic definition.  In the end, dealing with this kind of complexity is what craft is all about.  You just need to know the options and use your judgment about which options make the most sense.  Frankly, that's what makes it so much fun!

 

 

Share |

Aug. 30, 2005 - the first problem with all the code

Posted by Anonymous
The first problem with all the code is

the throwback TERRIBLE placement of the braces.



if (condition==met) {

do this;

and this;

return that;

}



Makes more sense.

Permanent Link

Nov. 6, 2006 - Braces

Posted by Anonymous
As you say, terrible placement of braces.
Most Java developers use the inline brace placement; so much so that I'd say it was the unofficial standard.

Thanks god for code formatters!
Permanent Link

Share and enjoy
  • Digg
  • del.icio.us
  • DZone
  • Netvouz
  • NewsVine
  • Reddit
  • Slashdot
  • StumbleUpon
  • Technorati
  • YahooMyWeb
<- Last Page Next Page ->

Kevin Barnes

Code Craft is the place for my thoughts, rants, ideas and occassional jokes on what it means to write code, why some people are better at it than others, and how we think about software in general.

Copyright (C) 2005, Kevin Barnes. All rights reserved.