3

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. – Robert Harvey Nov 12 '16 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. – RomanK Nov 12 '16 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. – Jules Nov 12 '16 at 18:48
  • I'd look for a date library which has a validDate(d,m,y) operation. – qwerty_so Nov 12 '16 at 19:30
  • 1
    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. – cbojar Nov 12 '16 at 21:29
12

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

  • 2
    +1 Using calendar implementation. Anyways, related to the given code I would add the IsLeap validation into the DaysInMonth resolution, DaysInMonth(m,y) – Laiv Nov 12 '16 at 19:03
  • 2
    @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? – Jules Nov 12 '16 at 19:07
  • Totally agreed with you. – Laiv Nov 12 '16 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. – Petr D. Svoboda Nov 13 '16 at 13:35
  • Also regretfully, I can't use any external libraries. It's an assignment, where I basically have to reinvent the wheel. – Petr D. Svoboda Nov 13 '16 at 13:37
5

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... – Erik Eidt Nov 12 '16 at 21:56
  • 1
    Ugg let me try. Damn names are the hardest part of programming. – candied_orange Nov 12 '16 at 21:57
  • I do like the lack of negation – candied_orange Nov 12 '16 at 21:59
  • You are right that using disjunction is much more readable. I will tweak that. – Petr D. Svoboda Nov 13 '16 at 13:39
2

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);
}
  • 2
    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! – candied_orange Nov 13 '16 at 4:42
1

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. – Petr D. Svoboda Nov 13 '16 at 13:42
0

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.

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.