# Cyclomatic complexity while protecting input

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;
}
``````
• These checks all look valid to me. The only way you're going to reduce the cyclomatic complexity is by eliminating some of the checks, or by reframing the problem you're trying to solve so that less checks are required. Commented Nov 12, 2016 at 18:42
• Indeed, don't see how to reduce cyclomatic complexity, but there are ways to improve readability. Rather than doing a direct numerical comparison, you could call a function `IsValidMonth`, `IsValidYear`, `IsValidDay`, `ValidateLeapYear` etc. Would also remove some of the copy/paste and make your intent clearer. Commented Nov 12, 2016 at 18:48
• Cyclomatic complexity directly equates to the number of branches in your flow control graph. You only have 1 such branch (ignoring short-circuit evaluation), so it would be pretty hard to reduce it further. Commented Nov 12, 2016 at 18:48
• I'd look for a date library which has a `validDate(d,m,y)` operation.
– user188153
Commented Nov 12, 2016 at 19:30
• If you are insistent on rolling your own, it might also be cleaner to change or wrap `DaysInMonth` so that it takes in the year and handles leap years internally. Then you would not need the separate leap year check. Commented Nov 12, 2016 at 21:29

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

• +1 Using calendar implementation. Anyways, related to the given code I would add the IsLeap validation into the DaysInMonth resolution, `DaysInMonth(m,y)`
– Laiv
Commented Nov 12, 2016 at 19:03
• @Laiv True. That would simplify things somewhat, but again using an existing calendar implementation that catches invalid dates is a much better option. I can't emphasize that enough. This is a wheel that has been reinvented way too many times, and we already have a lot of very good implementations. Why write a new half-baked one? Commented Nov 12, 2016 at 19:07
• Totally agreed with you.
– Laiv
Commented Nov 12, 2016 at 19:09
• I didn't do that at first, because I thought it wouldn't help with the complexity and that was, what I was focused at the moment. Although now I have done it, it helped me to resolve the problem. Commented Nov 13, 2016 at 13:35
• Also regretfully, I can't use any external libraries. It's an assignment, where I basically have to reinvent the wheel. Commented Nov 13, 2016 at 13:37

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.

• @CandiedOrange, I meant it as overloaded, but as this is C, I guess we'll have to name it something different... Commented Nov 12, 2016 at 21:56
• Ugg let me try. Damn names are the hardest part of programming. Commented Nov 12, 2016 at 21:57
• I do like the lack of negation Commented Nov 12, 2016 at 21:59
• You are right that using disjunction is much more readable. I will tweak that. Commented Nov 13, 2016 at 13:39

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);
}
``````
• Ah, no negations at all. And I love seeing vars between their min and max values. I don't even care if it's more complex. It's readable! Commented Nov 13, 2016 at 4:42

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.

• The thing is that my program is doing what I need him to do. The only thing I need now is to pass test for the complexity, as I am being tested on large data and I have to make my program as fast as possible. Commented Nov 13, 2016 at 13:42

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.