0
\$\begingroup\$

code climate tells me this block of code has a Cognitive Complexity of 7 (exceeds 5 allowed). Any ideas how I would refactor this to reduce complexity?

const validateDays = (param, days, dateValues) => {
  if (!days || !param) {
    return;
  } else if (param === 'from' && dateValues) {
    const disabledDays = {
      before: days.before,
      after: parse(dateValues['to'])
    };
    return { disabledDays };
  } else if (param === 'to' && dateValues) {
    const disabledDays = {
      before: parse(dateValues['from']),
      after: days.after
    };
    return { disabledDays };
  } else {
    return { days };
  }
};
\$\endgroup\$
5
  • \$\begingroup\$ sometimes code just is as complex as it is. Do you think your code is too complex? Or do you just want to get CodeClimate to shut up? \$\endgroup\$
    – Vogel612
    Jun 4, 2021 at 14:04
  • 1
    \$\begingroup\$ @Vogel612 I agree sometimes code is just complex. But improving readability will certainly help us understand it better. \$\endgroup\$ Jun 4, 2021 at 14:57
  • 2
    \$\begingroup\$ Please follow out guidelines WRT the title: codereview.stackexchange.com/help/how-to-ask \$\endgroup\$
    – BCdotWEB
    Jun 4, 2021 at 15:19
  • 1
    \$\begingroup\$ As @BCdotWEB the title should tell us what the code does rather than your concerns about the code. \$\endgroup\$
    – pacmaninbw
    Jun 4, 2021 at 16:06
  • 1
    \$\begingroup\$ @MalcolmMaima Welcome to CR! In addition to the suggestions for improvement so far, it's worth explaining the specification for this code in English. What is it supposed to validate? What do some sample calls look like? This context might lead to a fundamentally different approach that totally avoids the complexity problem. I also notice the post is tagged react.js but I don't see any obvious connection to React per se, so you might want to justify why the tag is present by describing the larger app context as an edit to your post \$\endgroup\$
    – ggorlen
    Jun 4, 2021 at 17:56

2 Answers 2

2
\$\begingroup\$

Complexity

Any code metric associated with readability is a subjective quantity. Cognitive complexity is one such metric and should be viewed with a good dose of skepticism.

Personally (to make up a metric) I would give your code a value of 1 where values over 0 are bad.

Why does it get 1? Because you use the same clause 2 times, which is 1 too many.

The value dateValues is checked twice

  } else if (param === 'from' && dateValues) {
      ...
  } else if (param === 'to' && dateValues) {
      ...

Checking the value only once means no redundancy

  } else if (dateValues) {
      if (param === 'from') {
          ...
      } else if (param === 'to') {
          ...
      }
  }

Cognitive Complexity

Which in my book is a dumber version of cyclic complexity.

Both metrics (cyclic and cognitive) can be reduced not by reducing complexity, but by tricking the parser into thinking the code is less complex.

Thus the following will likely get a lesser score than your original.

const validateDays = (param, days, dateValues) => {
    const dirs = {
        get from: () => ({before: days.before, after: parse(dateValues.to)}),
        get to: () => ({before: parse(dateValues.from, after: days.after)}), 
    };
    return days && param ? (dateValues ? dirs[param] ?? {days} : {days}) : undefined;  
}

However there are many here that consider nested ternaries as extremely complex (not my view).

Code quality

Personally I consider the bugs per line multiplied by line count the best metric for code quality.

Bugs per lines is a quantity defined by each coder. New coders have high values and this value goes down as you gain experience. The actual value is very hard to know, hence we can revert to the scalar line count as a good approximation. On average coders have a bug per line of ~ 1/100

Reducing line count is the best thing new coders can do to increase code quality.

Thus using the if else style your code can be

const validateDays = (param, days, dateValues) => {
  if (!(days && param)) { return }
  var res = {days};
  if (dateValues) {
    if (param === "from") {
      res = {disabledDays: {before: days.before, after: parse(dateValues.to)}};
    } else if (param === "to") {
      res = {disabledDays: {before: parse(dateValues.from), after: days.after}};
    }
  }
  return res;
}

Has a line count of 12 as opposed to your 19 lines, and may also get a lower cognitive complexity score.

or

const validateDays = (param, days, dateValues) => {
  if (!(days && param)) { return }
  var res = {days};
  if (dateValues) {
    param === "from" &&
      (res = {disabledDays: {before: days.before, after: parse(dateValues.to)}});
    param === "to" &&
      (res = {disabledDays: {before: parse(dateValues.from), after: days.after}});
  }
  return res;
}
\$\endgroup\$
1
  • \$\begingroup\$ Love your approach, definitely super helpful. Do get a complexity of 6 still above the threshold but I guess I can work with that for now \$\endgroup\$ Jun 5, 2021 at 19:14
1
\$\begingroup\$

Since every if statement also has a return, you don't need the else, you can just start the next if without typing else.

This reduces the "indentation" and keeps the code "flat" (for lack of better words) and I think this would reduce the complexity, although I'm not familiar with "Code climate".

I think the nested if-else-if-else structure actually goes one level deeper each time, causing the warning that you have.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.