Programming and
Design by
 
Jens Schöbel
Facebook Twitter LinkedIn

Self-Explanatory Code

Here I show you how to avoid comments in code. Some might think comments are needed to understand the according source. Unfortunately some problems will arise if you change your code but you forget to change the comment. Comments are good if they mean what they say. Hence the question come up, how to write code that avoids this issue?

The Example

Lets start with the following piece of code. Thats a snipped of code I had to deal with as I was a scientific member of the University of Heidelberg. Everything is written in plain C without classes. Why nobody was using object orientation I don't know. I assume its just the lack of experience since most programmers are beginners at the university. I started changing this section as I run into trouble with this:

void DetermineAreas (void) { ...   // Determines the area of a rectangular   Area1 = DetermineArea(0, side1); // Determines the area of a rectangular   Area2 = DetermineArea(0, side2, side3); ... }

You might think whats wrong with this. I tell you. Look at the first comment. Defined was DetermineArea(...) as:

float DetermineArea( int t, float l1, float l2 = -1.f); float DetermineArea( int t, float l1, float l2) {   float Area = 0;   // if no second argument given its a square   if (l2 < 0)   {     l2 = l1;     t = 1;   }   switch (t)   {     case 0:       // Area of a rectangle       Area  = l1 * l2;       break;     case 1:       // Area of a square       Area  = l1 * l1;       break;     default:       // error       printf ( "Error" );       break;   }   return Area ; }

DetermineArea(...) is very convoluted. Before you start wondering; this code isn't too complex. Its still understandable and thats why I've choosen this piece. Now lets have a closer look.

Problems:

  1. The mentioned comment will tell you that you are calculating the area of a rectangular ... or a square ... Who knows? You allways can say a square is a special rectangular but thats not what I mean.
  2. DetermineArea(...) uses an optional Parameter. I don't like that since you allways have to look into the definition of a mehtod to find out what its doing.
  3. I don't like the type parameter t. Since you need to look into the source to find out what's going on here.

Lets do some clean ups. But before this a warning. Please don't do clean ups without test. Sometimes you need to deal with the fact that there are no auto tests. Thats bad. And now guess how much code was covered with tests.

Refactor steps:

  1. Create an enum EAreaType and use this instead of t.
  2. This way the first comment becomes obsolete. EAreaType will tell you the type.
  3. If you have a closer look at the optional parameter you see that if l2 is missing its the same as l1. According to this you make l2 mandatory and change all the function calls - thats easy the compiler will tell you if something went wrong.
  4. l1 and l2 aren't good names so lets change this to something more meaningful.

enum EAreaType {   SQUARE = 0, RECTANGLE }; void DetermineAreas(void) { ...   Area1 = DetermineArea(RECTANGLE, side1, side1);   Area2 = DetermineArea(RECTANGLE, side2, side3); ... } DetermineArea( EAreaType type, float sideLength1, float sideLength2); float DetermineArea( EAreaType type, float sideLength1, float sideLength2) {   float Area = 0;   switch (type)   {     case SQUARE:       // Area of a rectangle       Area  = l1 * l2;       break;     case RECTANGLE:       // Area of a square       Area  = l1 * l1;       break;     default:       // error       printf ( "Error" );       break;   }   return Area ; }

Thats better but there is more you can do.

Refactor steps:

  1. In the first line of DetermineAreas(...) is a square calculated, hence change the type here.
  2. A switch with code is a code smell. Therefore extract two methods for it.
  3. To make code more readable you define an ERROR. Defining it will blow up the code but will make it much easier to maintain.

#define ERROR -1 void DetermineAreas (void) { ...   Area1 = DetermineArea(SQUARE, side1, side1);   Area2 = DetermineArea(RECTANGLE, side2, side3); ... } float DetermineArea ( EAreaType type, float sideLength1, float sideLength2) {   switch (type)   {     case SQUARE :       return GetAreaOfSqure(sideLength1);     case RECTANGLE:       return GetAreaOfRectangle(sideLength1, sideLength2);     default:       DrawError();   }   return ERROR; } float GetAreaOfSqure (float sideLength1); {   return sideLength1 * sideLength1; } float GetAreaOfRectangle( float sideLength1, float sideLength2) {   return sideLength1 * sideLength2; }

Thats much better but still there is more you can do.

Refactor steps:

  1. If you have a closer look at the switch statement its easy to get rid of it. Instead of calling GetAreaOfSqure(...) through DetermineArea(...) you can call it derectly. The same holds for GetAreaOfRectangle(...).
  2. By getting rid of the switch also EAreaType and ERROR become obsolete so delete it. This means they were just placeholders on the way to your final code. Now you might understand why I don't care if I blow up my code. Mostly you will delete lot of code later.

Thats it. Finaly you got a much cleaner code. With this little steps of refactoring you reduced your ammount of code and made it more readable by completely avoiding to write a single line of comment.

The code becomes self-explanatory now.

After Refactoring:

void DetermineAreas (void) { ...   Area1 = GetAreaOfSqure(side1);   Area2 = GetAreaOfRectangle(side2, side3); ... } float GetAreaOfSqure(float sideLength1); {   return sideLength1 * sideLength1; } float GetAreaOfRectangle( float sideLength1, float sideLength2) {   return  sideLength1 * sideLength2; }