Fix code with high NPath complexity

Question

I was analyzing my code with PHP Mess Detector when PHPMD reported some of my codes has high NPath Complexity. One example would be:

function compareDates($date1, $date2){
    if($date->year < $date2->year){
        return -1;
    }
    if($date->year > $date2->year){
        return 1;
    }
    if($date->month < $date2->month){
        return -1;
    }
    if($date->month > $date2->month){
        return 1;
    }
    if($date->day < $date2->day){
        return -1;
    }
    if($date->day > $date2->day){
        return 1;
    }
    // etc.. same for hour, minute, second.
    return 0;
}

The result would be that this function has very high NPath complexity. Is there a generic way of coding to reduce such control structures and NPath complexity?

Source Code: http://code.google.com/p/phpraise/source/browse/trunk/phpraise/core/datetime/RaiseDateTime.php#546

Using `else if` instead of just `if` for all but the first `if` statement will reduce the npath complexity tremendously! However, it won't make your code better of faster. — Jan Wy, Oct 27 '13 at 09:07

score 4 · Accepted Answer · answered Dec 05 '11 at 16:36

Your code actually is relatively simple, just poorly structured. I would recommend creating a subfunction that takes two parameters and handles the return of -1/1 and then iterating through an array of fields to check on, as this would be a bit easier, but a few things of note:

Your way is OK. It's not clean, but it's clear and if it works there's no pressing need to change it - any programmer who looks at it will be able to understand what you're doing, even if they scoff at your implementation.
Complexity isn't a holy grail. It's important, and as a programmer who does a lot of maintenance programming I think it's really important that the people writing the code I maintain know about complexity, but you can't entirely avoid complexity and sometimes the complex solution (using McCabe's Complexity) is the easiest to read.

The only change I would really recommend you making is having a single return call. Do something like:

$compare_val = 0;

At the top of your file, and then change your subsequent if calls to elseifs and instead of returning the value, just update $compare_val and return it at the end of your function.

just out of curiousity: is there any valid reason for using a single return call instead of how the OP does it or is just a matter of taste? I find that the way mauris wrote it, more cleaner — Vlad Balmos, Jan 09 '13 at 15:17
Absolutely. Multiple return points can make the code very confusing, especially when nested in control structures. One return statement should be all that's used if possible and it's always possible. Having a return return multiple values in an array is a good way to do this in PHP (vs Python or Go that have multiple return values in one return statement ... and I believe Go can only have one return statement). Return an empty array upon failure. Never return null when you can return an empty type. That makes every call have to do erro checking rather than it being done inside the function. — lucian303, May 31 '13 at 19:11
Multiple returns are discussed [here](http://stackoverflow.com/questions/36707/should-a-function-have-only-one-return-statement). — Alexander Palamarchuk, Oct 24 '13 at 15:30

Gordon · Answer 2 · 2011-12-05T16:53:35.780

It's a common misconception that sort functions have to return -1,0,1. You can do

function compareDates($date1, $date2)
{
    return strtotime("{$date1->year}-{$date1->month}-{$date1->day}")
         - strtotime("{$date2->year}-{$date2->month}-{$date2->day}");
}

Note that if the integer limit is an issue, you can use DateTime, which doesnt have that limitation, e.g.

function compareDates($date1, $date2)
{
    return new DateTime("{$date1->year}-{$date1->month}-{$date1->day}")
         < new DateTime("{$date2->year}-{$date2->month}-{$date2->day}");
}

As for reducing NPath Complexity in general: you have to reduce the number of possible execution paths. Have a look at the chapter about Simplifying Conditional Expressions from Fowler's Refactoring book for a start.

On a sidenote, I wonder what are the benefits of that RaiseDateTime thing? Can it do anything that the native DateTime API cannot do? If not, why would I want to use it?

The framework is designed for cross-compatibility. As far as development is concerned, we are interfacing the PHP API so that if the PHP API changes, users don't have to worry about it. — mauris, Dec 05 '11 at 23:20

score -1 · Answer 3 · answered Dec 05 '11 at 16:34

-1

Im new to PHP doesnt this code do the same but just simple?

function compareDates($date1, $date2){
if(($date->year < $date2->year) || ($date->month < $date2->month) || ($date->day < $date2->day) {
    return -1;
}
 if($date->year > $date2->year) || ($date->month > $date2->month) || ($date->day > $date2->day) {
    return 1;
}
// etc.. same for hour, minute, second.
return 0;
}

answered Dec 05 '11 at 16:34

dansasu11

875
1
9
17

1

No, it doesn't. If the years are different then he doesn't care about the months so he has to do the comparisons in the order he has defined them above. – Jonathan Rich Dec 05 '11 at 16:37

Fix code with high NPath complexity

3 Answers3