Refusing to continue

I’ve seen this so many times, and thought about writing about it. Today I found some code that is too good an example to let it go. I’m talking about the refusal some people have to using the continue statement. Here is an example in some pseudo-code:

for value in list {
  if value.condition == True {
      do stuff
      do more stuff
  }
}

It looks like fine code, but personally I’d rather use:

for value in list {
  if value.condition == False: continue

  do stuff
  do more stuff
}

The philosophy behind my preferred way is to bail out as soon as possible. It makes your code easier to understand, because you know there is not going to be an else somewhere down the line. If you see continue you can mentally let go of that condition, because you know that the rest of the code in the loop will be skipped.

This simple principle of using continue as soon as possible to mentally free you of keeping track of conditions also applies to other flow control statements such as break and return. Here is something I see all too often:

function(object *values) {
   if(values != null) {
       do something with the values;
   } else {
       handle errors;
   }
}

Again, very simple code, nothing really wrong with it. Until “doing something with the values” involves checking for more errors and doing a loop. Things get nested very deeply very rapidly. Instead I propose to do it like this:

function(object *values) {
   if(values == null) {
       handle errors;
       return;
   }

   do something with the values;
}

When you have code like this, it is very easy to add more conditions and error checks without the main body of the code getting any more indents. The level of indentation is directly proportional to the mental effort required to understand the code. Being able to add conditions and checks without the code becoming more complex is a very good thing.

These were all very simple examples. I have been able to remove several bugs just by applying the bail out as soon as possible strategy to actually professionally written production code.

A function should have one return point

I have heard many times that people were taught that “A function should have one return point”. It sounds like a silly thing to me - why would you not return, when you already know the work that had to be done is done? Until now people were only able to explain my “Why?” question with “it was taught to me that way”. Of course learning something only to teach it again is very nice for folklore but not for programming styles.

In many modern languages (well, more like non-ancient languages) there are many more return points to a function than just the return statement: exceptions. Any function call can cause an exception, and if you don’t handle it that means that every call can be a potential point of return.

And now here’s a bit of code that triggered me to finally write this down:

while((item = map_rect_get_item(mr))) {
    first=1;
    while (item_coord_get(item, &c, 1)) {
        if (first)
            first=0;
        else {
            heightline=heightlines;
            rbbox.lu=last;
            rbbox.rl=last;
            coord_rect_extend(&rbbox, &c);
            while (heightline) {
                if (coord_rect_overlap(&rbbox, &heightline->bbox)) {
                    for (i = 0 ; i < heightline->count - 1; i++) {
                        if (heightline->c[i].x != heightline->c[i+1].x || heightline->c[i].y != heightline->c[i+1].y) {
                            if (line_intersection(heightline->c+i, heightline->c+i+1, &last, &c, &res)) {
                                diagram_point=g_new(struct diagram_point, 1);
                                diagram_point->c.x=dist+transform_distance(projection_mg, &last, &res);
                                diagram_point->c.y=heightline->height;
                                diagram_point->next=diagram_points;
                                diagram_points=diagram_point;
                                dbg(0,"%d %d\n", diagram_point->c.x, diagram_point->c.y);
                            }
                        }
                    }
                }
                heightline=heightline->next;
            }
            dist+=transform_distance(projection_mg, &last, &c);
        }
        last=c;
    }

}

With a few modifications I changed the code into this:

while((item = map_rect_get_item(mr))) {
    first=1;
    for(; item_coord_get(item, &c, 1); last=c) {
        if (first) {
            first=0;
            continue;
        }

        heightline=heightlines;
        rbbox.lu=last;
        rbbox.rl=last;
        coord_rect_extend(&rbbox, &c);
        for (; heightline; heightline=heightline->next) {
            if (!coord_rect_overlap(&rbbox, &heightline->bbox))
                continue

            for (i = 0 ; i < heightline->count - 1; i++) {
                if (heightline->c[i].x == heightline->c[i+1].x && heightline->c[i].y == heightline->c[i+1].y)
                    continue;

                if (!line_intersection(heightline->c+i, heightline->c+i+1, &last, &c, &res))
                    continue;

                diagram_point=g_new(struct diagram_point, 1);
                diagram_point->c.x=dist+transform_distance(projection_mg, &last, &res);
                diagram_point->c.y=heightline->height;
                diagram_point->next=diagram_points;
                diagram_points=diagram_point;
                dbg(0,"%d %d\n", diagram_point->c.x, diagram_point->c.y);
            }
        }
        dist+=transform_distance(projection_mg, &last, &c);
    }

}

Granted, it is still a big beast and should be split up into a few small functions, but still I was able to reduce the nesting level of the main piece of the code (the diagram_point lines) by four levels.

NOTE: The C code above was taken from Navit, an Open Source navigation application. Big kudos to the team that wrote the application, as it works well. By writing this article I do not want to imply anything about the quality of the Navit application in general, just about the small bit of code I copied.