Which code snippet would you rather come across? [closed]

Question

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?

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

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.

Answer 5

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.