Posts tagged ‘return statement’

Set the Returns Free

2013-07-24 19:02

Few pastimes are more pointless and unproductive than arguing about coding style. As long as the basic requirement of consistency is present, there is really next to nothing that wouldn’t fly. You may hear stories of certain… unusual approaches, like using 3-space indentation to “offend all equally”, but it still doesn’t give you the license to criticize – or worse yet, disobey. If you chose to contribute to a particular codebase, you follow the standard, period.

With this out of the way, I will go ahead to pick on my personal pet peeve by pointing out that the rule of single return from function is just dumb!… To my excuse, however, it is not really a stylistic rule, which is also one of the reasons I consider it inane. Since it changes the algorithmic structure of a function, it’s actually about semantics and should be treated as architectural guideline.

Even then, what’s wrong with avoiding more than one return statement? A couple of things come to mind.

It’s unnecessarily verbose

There is a particular “idiom” that you may encounter from time to time, especially in beginner’s code, or just code written by someone who had a particularly bad day. Here’s how it goes:

  1. bool isFooed() {
  2.     if (some != condition || thatMay(involve(foo))) {
  3.         return true;
  4.     } else {
  5.         return false;
  6.     }
  7. }

While it looks completely silly, its only felony is carrying the result through one more place than it’s absolutely necessary. But we would never write such a thing with clear mind. We would point it out immediately in code review. We would fix it whenever encountered.

And yet, it’s essentially the same pattern as in the code below:

  1. bool performSomeObscureAction() {
  2.     bool success;
  3.     if (startsAlignProperly()) {
  4.         // .. do stuff, followed by more stuff ...
  5.         success = true;
  6.     } else {
  7.         // ... report error, maybe do some cleanup ...
  8.         success = false;
  9.     }
  10.     return success;
  11. }

It has just one redundant layer of complexity: the success variable. We could remove it and replace the assignments to it by direct return true; and return false;. But that, of course, would violate the rule of single return point, and therefore code meant to follow it is stuck with flaws equivalent to having no idea how basic control statements and boolean logic works.

It reeks of Pascal stench

If we are happily condemning the use of more than one return statement, we could as well stay completely true to our beliefs. Why not prohibit all of such statements, and in fact use a language that just plain doesn’t support them?…

  1. function PerformAction : Boolean;
  2. begin
  3.     if canDo; then
  4.         { ... do stuff ... }
  5.         PerformAction := True;
  6.     else
  7.         PerformAction := False;
  8. end;

I’m totally baffled by any claims purporting that this technique makes code more readable. Not only the return value is now semi-hidden, disguised as one of the ordinary variable assignments, but also the return point itself is completely lost. An execution path may end inside an if inside while inside a switch, somewhere deep in the function innards, and you wouldn’t even notice without tracing all the closing braces end;s, or following the changes in indentation levels.
I’m all for disincentivizing long functions but really, there are much better ways to do that.

It gives the wrong impression

Although by far the most braindead, the rule of single return is unfortunately not the only one of its kind. There is a whole class of misguided principles that apply to certain flow control instructions: return, break and continue. What they share in common is the great potential of obfuscating the real meaning of some parts of the algorithms, simply because of forcing the code to be structured in a specific, rigid way.

Why it’s bad? Because one size does not fit all. Abusing early returns is hardly commendable:

  1. @Override
  2. public void onReceive(Context context, Intent intent) {
  3.     if (intent.getBooleanExtra(BatteryManager.EXTRA_PLUGGED, false)) {
  4.         resumeBackgroundTasks();
  5.         return;
  6.     }
  7.     stopBackgroundTasks();
  8.     // XXX: why "charger plugged" is more special state than "unplugged"?...
  9. }

but avoiding them at all costs won’t make the code better by any stretch:

  1. void read_config_file(const std::string& filename) {
  2.     std::ifstream ifs(filename):
  3.     if (ifs) {
  4.         // read the file ...
  5.         // ... parse the file ...
  6.         // ... remember config values ...
  7.         // ... and probably some other things too!
  8.     } else {  // (200 lines later)
  9.         log("cannot read config file: %s", filename);
  10.     }
  11. }

Since every function is different, trying to mold them into some over-specified template will just end awry.

Tags: ,
Author: Xion, posted under Computer Science & IT » Comments Off on Set the Returns Free
 


© 2023 Karol Kuczmarski "Xion". Layout by Urszulka. Powered by WordPress with QuickLaTeX.com.