Page 2 of 2

Posted: Thu Jul 28, 2005 9:10 pm
by timvw
Consider the function below, try to make it as readable with a single return statement ;)

Code: Select all

function isLeapYear($year)
{
  // if year isn't a multitude of 4, it isn't a leapyear
  if (($year % 4) != 0) 
  {
    return false;
  }

  // centuries that can't be divided by 400 aren't leapyears
  if (($year % 100) == 0)
  { 
    if (($year % 400) != 0)
    {
      return false;
    }
  }

  // it must be a leapyear
  return true;
}

Posted: Thu Jul 28, 2005 9:48 pm
by nielsene
Hey no-fair! I'm a multi-return guy and I'm still trying to do it :)

Code: Select all

function isLeapYear($year)
{
  $leap = TRUE;
  if (($year % 4) != 0) $leap=FALSE;
  if ($leap && (($year % 100)==0) && (($year % 400)!=0)) {
      $leap=FALSE;
    }
  return $leap;
}

Posted: Thu Jul 28, 2005 11:10 pm
by harrisonad

Code: Select all

function isLeapYear($year){  
    $is_leap_year = true;
    if ((($year % 4) != 0)||(($year % 100) == 0) &&(($year % 400) != 0)){  
        $is_leap_year = false;
    }
    return $is_leap_year;
}
No problem, but again I vote for multiple returns.

Posted: Sat Jul 30, 2005 6:07 pm
by stukov
harrisonad wrote:

Code: Select all

function isLeapYear($year){  
    $is_leap_year = true;
    if ((($year % 4) != 0)||(($year % 100) == 0) &&(($year % 400) != 0)){  
        $is_leap_year = false;
    }
    return $is_leap_year;
}
No problem, but again I vote for multiple returns.
Hehe... You still did it in 7 lines of code...

I think there is always a way to do it with only one return. You see, if the code needs to use multiple returns (like the code timvw submitted), it is because the code can be re-designed in a better way (like the code harrisonad submitted.
McGruff wrote:I don't think you can sustain that.
Really? :wink:

Posted: Sat Jul 30, 2005 6:11 pm
by McGruff
OK the second function I gave could have a single return (would it be as readable though?). However, in the first - parse() - I really want to exit the loop immediately I find what I'm looking for rather than waste time churning through the full list. That means I need two returns :)

Posted: Sat Jul 30, 2005 6:14 pm
by nielsene
stukov wrote:
I think there is always a way to do it with only one return. You see, if the code needs to use multiple returns (like the code timvw submitted), it is because the code can be re-designed in a better way (like the code harrisonad submitted.
McGruff wrote:I don't think you can sustain that.
Really? :wink:
I disagree. In order to implement the version that harrisonad or myself showed we had to implement an extra local variable or suffer with extra nesting. If we needed a second flag variable, then it quickly gets very messy, each if starts to have lots of terms and more logical branches to worry about, or you end up with the deaply nesting problem. Additionally the large if is now less easily readablee, in my mind. Its harder to pick out the three leap year rules.

You actually could even reduce it further:

Code: Select all

function isLeapYear($year) {
  return !((($year % 4) != 0)||(($year % 100) == 0) &&(($year % 400) != 0)));
}
but I wouldn't really recommend that from a readable perspective.

Posted: Sat Jul 30, 2005 6:15 pm
by stukov
McGruff wrote:OK the second function I gave could have a single return (would it be as readable though?). However, in the first - parse() - I really want to exit the loop immediately I find what I'm looking for rather than waste time churning through the full list. That means I need two returns :)
No, you could have used a while like this one :

Code: Select all

while (list(, $value) = each($arr))
instead of the foreach. In this case, you could have add another condition which would "break" the loop when the condition is met. Using a var which contains the value you will return, you could have only one return. :wink:

Posted: Sat Jul 30, 2005 6:26 pm
by stukov
nielsene wrote:I disagree. In order to implement the version that harrisonad or myself showed we had to implement an extra local variable or suffer with extra nesting. If we needed a second flag variable, then it quickly gets very messy, each if starts to have lots of terms and more logical branches to worry about, or you end up with the deaply nesting problem. Additionally the large if is now less easily readablee, in my mind. Its harder to pick out the three leap year rules.

You actually could even reduce it further:

Code: Select all

function isLeapYear($year) {
  return !((($year % 4) != 0)||(($year % 100) == 0) &&(($year % 400) != 0)));
}
but I wouldn't really recommend that from a readable perspective.
Reduced doesn't mean clearer. Actually, I find harrisonad's version easier to read. But, what you are saying makes sense. Now again, which one should I trust? Dijkstra or nielsene?

Posted: Sat Jul 30, 2005 6:35 pm
by nielsene
OK, I wasn't sure if you were thinking shorter = better.

Obviosuly I'm less of an authority than Dijkstra. But most of his writings are also based aaround much earlier languange and pre-date a lot of OOP inspired theory, so I wouldn't use him as an absolute authority these days.

I'll still say that if you see a function that has deeply nested ifs and a single return you have a few options:
1. Switch some of the encasing if's to guard clauses that return early
2. See if Extract Method will help lower the nested-ness and increase readability
3. See if introducing extra, local flag variables lets you remove some nesting
and the expense of "simple" logic conditions.

I'm not saying you should always use one paticular method to fix the code smell, but don't throw away a method either.


[edit, fixed a confusing typo... easiler==>earlier]

Posted: Sat Jul 30, 2005 9:11 pm
by timvw
I think "Structured Programming" was a logic consequence for the spaghetti code around those days, but i think it was more a case against goto than a case for one in and one out.

I'm aware that Dijkstra has proven that each (sub)program can be written with one in and one out. But i don't agree that it's always the most readable version.

I (subjective) sometimes prefer code this way:

Code: Select all

if (...) return 1;
if (...) return 2;
if (...) return 3;
return 4;
Oppossed to

Code: Select all

if (...) result = 1;
else if (...) result = 2;
else if (...) result = 3;
else result = 4;
return result;

Posted: Sun Jul 31, 2005 8:58 pm
by harrisonad
high-level languages are meant to be readable to human. Yes, we can reduce our code to smallest as possible, but I am not sure if you can work with your team efficiently on practicing it.