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

Answer 1

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:

1. 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.

2. 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.

Answer 2

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?

Answer 3

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;
}