Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I was just taking a look at CodeIgniters encryption class and found the following function:

function sha1($str)
{
    if ( ! function_exists('sha1'))
    {
        if ( ! function_exists('mhash'))
        {
            // use CI implementation
            require_once(BASEPATH.'libraries/Sha1'.EXT);
            $SH = new CI_SHA;
            return $SH->generate($str);
        }
        else
        {
            // use alternate built in function
            return bin2hex(mhash(MHASH_SHA1, $str));
        }
    }
    else
    {
        // use built in function
        return sha1($str);
    }
}

If I was writing this function I would do so like this:

function sha1($str)
{
    // use built in function
    if(function_exists('sha1'))
        return sha1($str);

    // use alternate built in function
    if(function_exists('mhash'))
        return bin2hex(mhash(MHASH_SHA1, $str));

    // use CI implementation
    require_once(BASEPATH.'libraries/Sha1'.EXT);
    $SH = new CI_SHA;
    return $SH->generate($str);
}

Which is better and why?

share|improve this question

closed as off topic by Anna Lear Jan 8 '12 at 17:03

Questions on Programmers Stack Exchange are expected to relate to software development within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.

    
Evaluating code snippets is off-topic here, but you might want to check out our sister site Code Review where you can get feedback on your own working code. –  Anna Lear Jan 8 '12 at 17:03
    
Any code that says if (!x) {} else {} drives me nuts. –  nicodemus13 Dec 20 '12 at 13:45

5 Answers 5

The second one is much better - reads like what it means.

The fact that it is shorter and without nested ifs makes it a better read.

share|improve this answer

Early return (the second version) is always better in my opinion. In your very simple example where the function has no local state it is not obvious, but it is very obvious in more complicated situations.

When you read an early-return function you can easily make sense of it piece by piece without having to examine what is going to happen with the local state further down. When the function does not use early-return, then as you consider each possible flow-of-control case you are not sure what the outcome will be unless you read the entire function all the way to the end to make sure that no more operations are performed with the local state of the function.

To put it in different terms, the complexity of your program is equal to the amount of state multiplied by the amount of code to which the state is accessible for read and write (*1). The more code you have to take into consideration, the larger the multiplication factor.

(*1) Incidentally, that's why immutability is awesome: it greatly reduces program complexity because immutable data are only available to the code for read, not for read-and-write.

share|improve this answer
    
This is what I thought (and hoped). I just wanted to confirm there was no brilliance in the approach taken by the CI team in this case. –  Matt Jan 8 '12 at 13:33
1  
Brilliance? Are you kidding me? It is as dumb as a doorknob and it derives from the days of early languages like Pascal which had no return statement, so the only way to exit from a function was to keep going until you fell off the end of it. –  Mike Nakis Jan 8 '12 at 13:46

Given a choice between the two, the second one is better because it is more readable.

In my opinion, however, best practices would be to recognize that the sha1 function has been built-in to php for 10 years now, and take out the dead code that supports ancient, deprecated, unsupported php versions that no one in their right mind still uses.

share|improve this answer

I would tend to write it like your second example:

// Compute a sha1 hash using some existing routine
// Prefer sha1 to mhash to CI implementation
function sha1($str) {
  if (function_exists('sha1')) return sha1($str);
  if (function_exists('mhash')) return bin2hex(mhash(MHASH_SHA1,$str));
  require_once(BASEPATH.'libraries/Sha1'.EXT);
  return (new CI_SHA)->generate($str);
}

in order to avoid naming any intermediates, and to get the mostly redundant comments out of the code which would otherwise be compact and simple.

It's really a very simple piece of code; spreading it out over 20 lines with braces and comments and so on hardly helps speed understanding.

share|improve this answer
    
I think adding the ifs back in only serves to confuse! –  Matt Jan 8 '12 at 13:32
    
@Matt - Given the compactness of the alternative, you have a point. –  Rex Kerr Jan 8 '12 at 15:12
    
It infuriates me no end when I see if (something) return something; else if (...). The else on the else if is completely redundant and makes me think that the person who wrote the code doesn't really get the point of returning in the if. –  Stu Jan 8 '12 at 15:13
    
@Stu - I guess it depends on your syntax highlighting and the size of the method. If the return statement is obvious, I agree. If it's not obvious (because of nesting, for example), then the point that "this is an alternative code path" is clear, even if the mechanism is redundant. In this case, though, on reflection, I agree; I've changed the answer. –  Rex Kerr Jan 8 '12 at 15:16
    
If you are returning out in complex nesting then you need to refactor. Finding a return statement buried 3+ levels deep is equally annoying! Refactor it and you'll quickly find that the else isn't required. –  Stu Jan 8 '12 at 15:20

The second one is easier to read and understand due to inverting the if logic making it easier to see what the function actually returns.

The problem is that each time this method is called it should, by design, return the same implementation each time which means you're going through the same if logic each time with no change in result given the same string.

An alternative approach could be using the Strategy pattern :

function sha1_factory() {
    $hash_func = "";
    // Only decide which implementation once
    if(function_exists('sha1')) {
        $hash_func = sha1;
    }
    else if(function_exists('mhash')) {
        $hash_func = function($str) {
            return bin2hex(mhash(MHASH_SHA1, $str));
        };
    }
    else {
        $hash_func = function($str) {
            require_once(BASEPATH.'libraries/Sha1'.EXT);
            $SH = new CI_SHA;
            return $SH->generate($str);
        };
    }
    // Can also use anonymous functions to wrap again to 
    // avoid passing the function in over and over again
    sha1_hasher = function($str) {
        $hash_func($str);
    };
    return sha1_hasher;
}

With this approach you're not going through that if logic every single time even though the result will not change and it is easy to add new implementations.

P.S. Full disclosure, I am not a PHP programmer by trade, if someone can think of a cleaner way to do this other than using Anonymous functions please comment.

share|improve this answer
4  
^Paterns, they make simple things difficult... (^)-quite often. –  Coder Jan 8 '12 at 13:48
1  
Depending on how and when patterns are applied they can make simple things difficult or difficult things simple. Perhaps this was not the correct place to use this pattern given the simplicity of the sample. –  ahjmorton Jan 8 '12 at 13:51

Not the answer you're looking for? Browse other questions tagged or ask your own question.