3
\$\begingroup\$

The following method works fine but it is rather verbose:

private function performComparison($element, $operator, $value_allowed, $value_entered)
{
    switch($operator):
        case '>':
            $result = ($value_entered >  $value_allowed) ? true : false;
            break;
        case '>=':
            $result = ($value_entered >= $value_allowed) ? true : false;
            break;
        case '<':
            $result = ($value_entered <  $value_allowed) ? true : false;
            break;
        case '<=':
            $result = ($value_entered <= $value_allowed) ? true : false;
            break;
        case '=':
            $result = ($value_entered == $value_allowed) ? true : false;
            break;
    endswitch;
        $dateformat = 'n/j/Y';
        $debug = ['element' => $element, 'operator' => $operator, 'value allowed' => is_a($value_allowed, 'DateTime') ? $value_allowed->format($dateformat) : $value_allowed, 'value entered' => is_a($value_entered, 'DateTime') ? $value_entered->format($dateformat) : $value_entered, 'result' => ($result === false) ? 'false' : 'true'];
        echo '<pre>'.print_r($debug,1).'</pre>';
    return $result;
}

Note that the 3 lines of code just before the return $result; line are very helpful for debugging in the Development phase so I'd like to keep those.

In an attempt to make a more concise version of this method I came up with the following:

private function performComparison($element, $operator, $value_allowed, $value_entered)
{
    $comparisonFunctions = [
        '>' => function($value_allowed, $value_entered) { return ($value_entered >  $value_allowed) ? true : false; },
        '>='    => function($value_allowed, $value_entered) { return ($value_entered >= $value_allowed) ? true : false; },
        '<' => function($value_allowed, $value_entered) { return ($value_entered <  $value_allowed) ? true : false; },
        '<='    => function($value_allowed, $value_entered) { return ($value_entered <= $value_allowed) ? true : false; },
        '=' => function($value_allowed, $value_entered) { return ($value_entered == $value_allowed) ? true : false; }
    ];

    $result = array_key_exists($operator, $comparisonFunctions) ? $comparisonFunctions[$operator]($value_allowed, $value_entered) : null;
        $dateformat = 'n/j/Y';
        $debug = ['element' => $element, 'operator' => $operator, 'value allowed' => is_a($value_allowed, 'DateTime') ? $value_allowed->format($dateformat) : $value_allowed, 'value entered' => is_a($value_entered, 'DateTime') ? $value_entered->format($dateformat) : $value_entered, 'result' => ($result === false) ? 'false' : 'true'];
        echo '<pre>'.print_r($debug,1).'</pre>';
    return $result;
}

This version also works fine but I'm wondering if there are any ways it could be condensed further.

\$\endgroup\$
2
  • 3
    \$\begingroup\$ At the very least, ... ? true : false is always too verbose and can be omitted. \$\endgroup\$ – deceze Jul 10 '18 at 12:10
  • \$\begingroup\$ Could you explain what this function is actually for? I'm finding it hard to understand why you'd do something like performComparison($element, $operator, $value_allowed, $value_entered) instead of just doing the comparison. If the purpose of the function is to assist debugging then you should have a logComparison function that you call after making a comparison not performComparsion where it just has those 3 lines after the switch. I know if I came across a code base that made use of a performComparison function I would forget to use it in favor of making the actual comparison. \$\endgroup\$ – Shelby115 Jul 10 '18 at 12:26
2
\$\begingroup\$

given your function's purpose is to return the calculation result, I would return the expression result right away, especially given return makes break unnecessary as well

private function performComparison($operator, $value_allowed, $value_entered)
{
    switch($operator) {
        case '>':
            return ($value_entered >  $value_allowed);
        case '>=':
            return ($value_entered >= $value_allowed);
        case '<':
            return ($value_entered <  $value_allowed);
        case '<=':
            return ($value_entered <= $value_allowed);
        case '=':
            return ($value_entered == $value_allowed);
        default:
            throw new InvalidArgumentException("Invalid operator");
    }
}

The code above is both concise and readable, it takes one no problem to gist the function's meaning and purpose.

Whereas any debugging should be done elsewhere. If you need any debugging for this function, create another one encapsulating the current function's call.

    private function debugPerformComparison($element, $operator, $value_allowed, $value_entered)
    {
        $result = $this->performComparison($operator, $value_allowed, $value_entered);
        $dateformat = 'n/j/Y';
        $debug = [
            'element' => $element, 
            'operator' => $operator, 
            'value allowed' => is_a($value_allowed, 'DateTime') ? $value_allowed->format($dateformat) : $value_allowed, 
            'value entered' => is_a($value_entered, 'DateTime') ? $value_entered->format($dateformat) : $value_entered, 
            'result' => $result,
        ];
        echo '<pre>'.var_export($debug,1).'</pre>';
        return $result;
    }

Regarding anonymous function-based version, a concise version shouldn't be less readable in the first place. Any code improvements should improve the readability, by means of removing unnecessary parts, not worsen it. And the said version is extremely hard to read.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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