Cyclomatic complexity while protecting input

Question

I need to reduce the cyclomatic complexity of my input tests. I have no control over the inputs, so I have to go through all of these checks.

if(y1 < 2000 || y2 < 2000 ||
    !(m1 >= 1 && m1 <= 12) ||
    !(m2 >= 1 && m2 <= 12) ||
    !(d1 >= 1 && d1 <= DaysInMonth(m1)) ||
    !(d2 >= 1 && d2 <= DaysInMonth(m2)) ||
    (m1 == 2 && IsLeap(y1) && !(d1 >= 1 && d1 <= DaysInMonth(m1) + 1)) ||
    (m2 == 2 && IsLeap(y2) && !(d2 >= 1 && d2 <= DaysInMonth(m2) + 1)))
{
    return 0;
}

Is there any way, to make it less complex. Should I approach this in different way?

EDIT

Thanks everybody. I managed to lower the complexity. Basically the main issue was with the function DaysInMonth, as it wasn't handling leap years. It was just a small helper function at first and I didn't need to manage leap years. But that changed in the end, so I plastered the leap year logic outside of it wherever I needed it.

I tweaked DaysInMonth a bit with the help of your examples and cyclomatic complexity went down.

I also made a function for the invalid date checking and switched to disjunctions.

So this is my result.

int IsInvalidDate(int year, int month, int day) {
return year < 2000 ||
    month < 1 || month > 12 ||
    day < 1 || day > DaysInMonth(year, month);
}

...

if(IsInvalidDate(y1, m1, d1) || IsInvalidDate(y2, m2, d2)) {
    return 0;
}

Answer 1

You're performing identical tests on two independent sets of values. Factor out a function that performs those tests on a single set of values, and then call it twice:

int DateInvalid (int d, int m, int y)
{
     return y < 2000 ||
         !(m >= 1 && m <= 12) ||
         !(d >= 1 && d <= DaysInMonth(m)) ||
         (m == 2 && IsLeap(y) && !(d >= 1 && d <= DaysInMonth(m) + 1));
}

...
if (DateInvalid (d1, m1, y1) || DateInvalid (d2, m2, y2)) return 0;

Or, better yet, use an existing calendar implementation to perform the check for you, thus removing your need to have date manipulation code at all (which is notoriously easy to get wrong).

Answer 2

Let's back out the excessive negation, which makes the logic much harder to read.

The same tests can be written using only disjunction instead of as disjunction of negated conjunctions.

return y < 2000 ||
     m < 1 || m > 12 ||
     d < 1 || d > DaysInMonth(m,y);

where

int DaysInMonth(m,y) {
    return DaysInNonLeapYearMonth(m) + (m == 2 && IsLeap(y) ? 0 : 1);
}

The disjunctive version is almost readable by comparison with the negated conjunctions.

Answer 3

As already said, using existing date utilities is better. Let's assume they're unavailable so I can present the following tips.

Don't use negated conditions. IsDateValid is easier to understand than the opposite, especially when combined with additional negations.

Consider using test of the form a <= b && b <=c as they often read easier just like a ≤ b ≤ c does.

Try to find a form needing no parentheses and no additional negations like

boolean isDateValid(int d, int m, int y) {
     return 2000 <= y 
         && 1 <= m && m <= 12
         && 1 <= d && d <= daysInMonth(m, y);
}

Answer 4

Reducing cyclomatic complexity should not be your primary goal in this scenario. Your validation seems to be a sort of "if anything bad is detected, return false, otherwise return true". Those types of validations are usually pretty easy to understand regardless of what the analysis tool reports.

In these scenarios, unless pieces of the validation tests are used in other areas, it is ok to have a longish/complex function.

Especially in this scenario, as Date and time handling is one of those obnoxiously nuanced and difficult to get right areas. Your goal should be correctness, and using established, reliable date/time libraries is the way to accomplish that.

Like virtually all 'rules' in software development, there are exceptions to every rule. While cyclomatic complexity can usually indicate a function is growing too large to be easily understood, sometimes that's not the case. This may be one of them.

Answer 5

Your cyclomatic complexity is so high that your code is broken. It will reject Feb 29th in leap years, because you first check that the date is within the 28th normal days and it fails.

The immediately obvious problem is that you have a function called DaysInMonth with only one parameter. It's a lie. It doesn't return the number of days in the month, it returns a number that is sometimes correct and sometimes isn't. A function named DaysInMonth must have two parameters, one for the month and one for the year.

From experience, once you see an obvious red flag like this, you just know that there is a bug nearby, and it was right. Your code is unnecessary complex because you don't pass the year to DaysInMonth, and as a result it is wrong.

You would also make your code much more readable (remove the brain-twisting !s) by checking for a valid date.