Zrjydtj

I have a function that checks user level and limits the data before saving it to mongoDB database (pre 'save' middleware for mongoose).
It have been getting complexity warnings and tried to rewrite it, and came up with the second version. I think now it's really not human readable! Anyone has any suggestions?

Before:

providerSchema.pre('save', function(next) {

 if(this.level === 4) this.certifications.length > 5)
 next(new Error('your current plan does not have this feature'));
 else 
 next()
 
 else if(this.level === 3)
 if(this.description.length >= 50 else if(this.level === 2) else if(this.level === 1) else this.locationLong );

After:

providerSchema.pre('save', function(next) this.locationLong );
```

According to www.lizard.ws the original's function cyclomatic complexity is 29 and for the second version is 22. Both numbers are usually considered high, and teams aim for much lower values (debatable what the good range is though and will see with this example why).

In order to reduce it, one way is to encapsulate the if statements, among with reducing the code duplication and responsibilities.

The next calls seem duplicate, so lets reduce them first.

providerSchema.pre('save', function (next) 

 var valid = true;
 if (this.level === 4) else if (this.level === 3) 
 if (this.description.length >= 50 else if (this.level === 2) else if (this.level === 1) this.teaser else this.teaser 

 if (valid) 
 next();
 else 
 next(new Error('your current plan does not have this feature'));
 
);

With this first refactoring we didn't gain much in terms of reducing CC, in fact it increased to 30, because of the added if statement. However this can let us to split the validation responsibility from the enabling the actual feature (as @Margon mentioned).

providerSchema.pre('save', function (next) 
 if (isValidFeatureRequest()) 
 next();
 else 
 next(new Error('your current plan does not have this feature'));
 

 function isValidFeatureRequest() 
 if (this.level === 4) 
 if (this.description.length >= 80 else if (this.level === 3) else if (this.level === 2) this.social.length > 0) 
 return false;
 
 else if (this.level === 1) else 
 return true;
 
);

The isValidFeatureRequest function is at 29 and providerSchema is at 2. We still need to work on.

Checking again the code duplication, I noticed the last two lines validate the same for Level 1 other levels than 2, 3 or 4. So let's remove that too.

providerSchema.pre('save', function (next) 
 if (isValidFeatureRequest()) 
 next();
 else 
 next(new Error('your current plan does not have this feature'));
 

 function isValidFeatureRequest() 
 if (this.level === 4) 
 if (this.description.length >= 80 else if (this.level === 3) else if (this.level === 2) this.social.length > 0) 
 return false;
 
 else 
 return true;
 
);

We gained the following figures

function | CC
-------------------------------------------
providerSchema | 2
isValidFeatureRequest | 20

The CC for isValidFeatureRequest is now at 20, which is an improvement.
The check for description and certifications also seem vary, so let's encapsulate this into a function

providerSchema.pre('save', function (next) 
 if (isValidFeatureRequest()) 
 next();
 else 
 next(new Error('your current plan does not have this feature'));
 

 function isValidFeatureRequest() 
 if (this.level === 4) 
 if (descriptionOrCertificationsOffLimits(80, 5)) 
 return false;
 
 else if (this.level === 3) 
 if (descriptionOrCertificationsOffLimits(40, 3)) 
 return false;
 
 else if (this.level === 2) else 
 return true;
 

 function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) 
);

function | CC
-------------------------------------------
providerSchema | 2
isValidFeatureRequest | 17
descriptionOrCertificationsOffLimits | 2

CC is now at 17, slightly better.

There is lot to check in the last branch, so let's extract this into his own function.

providerSchema.pre('save', function (next) 
 if (isValidFeatureRequest()) 
 next();
 else 
 next(new Error('your current plan does not have this feature'));
 

 function isValidFeatureRequest() 
 if (this.level === 4) 
 if (descriptionOrCertificationsOffLimits(80, 5)) 
 return false;
 
 else if (this.level === 3) 
 if (descriptionOrCertificationsOffLimits(40, 3)) 
 return false;
 
 else if (this.level === 2) else if (hasAny()) 
 return false;
 
 return true;
 

 function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) 

 function hasAny() this.workingHourEnd 
);

Which results into

function | CC
-------------------------------------------
providerSchema | 2
isValidFeatureRequest | 10
descriptionOrCertificationsOffLimits | 2
hasAny | 8

We have now 4 functions with manageable complexities.
The hasAny function seems to have a large CC, compared to what it does. What we can do here is to improve its readability. This is also the moment when I think we can't do anything about this function, and is the time not to look at an arbitrary CC limit in order to squize the code just to pass analyzer.

 function hasAny() 
 this.workingHourEnd 

Let's extract more, to check if it has a teaser or social data

providerSchema.pre('save', function (next) 
 if (isValidFeatureRequest()) 
 next();
 else 
 next(new Error('your current plan does not have this feature'));
 

 function isValidFeatureRequest() 
 if (this.level === 4) 
 if (descriptionOrCertificationsOffLimits(80, 5)) 
 return false;
 
 else if (this.level === 3) 
 if (descriptionOrCertificationsOffLimits(40, 3)) 
 return false;
 
 else if (this.level === 2) hasTeaserOrSocial()) 
 return false;
 
 else if (hasAny()) 
 return false;
 
 return true;
 

 function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) 

 function hasTeaserOrSocial() this.social.length > 0;
 

 function hasAny() 
 this.workingHourEnd 
);

Which results into

function | CC
-------------------------------------------
providerSchema | 2
isValidFeatureRequest | 9
descriptionOrCertificationsOffLimits | 2
hasTeaserOrSocial | 2
hasAny | 8

The if followed by an inner if can be combined into and and operation so we can have this

 function isValidFeatureRequest() hasTeaserOrSocial()) 
 return false;
 else if (hasAny()) 
 return false;
 
 return true;
 

The CC doesn't change, but I can extract validation for every level, so I gain smaller functions, with smaller complexity.

function isValidFeatureRequest() 
 if (isLevel4AndNotValid()) 
 return false;
 else if (isLevel3AndNotValid()) 
 return false;
 else if (isLevel2AndNotValid()) 
 return false;
 else if (hasAny()) 
 return false;
 
 return true;
 

 function isLevel4AndNotValid() 
 return this.level === 4 && descriptionOrCertificationsOffLimits(80, 5);
 

 function isLevel3AndNotValid() 
 return this.level === 3 && descriptionOrCertificationsOffLimits(40, 3);
 

 function isLevel2AndNotValid() 

Which are

function | CC
-------------------------------------------
providerSchema | 2
isValidFeatureRequest | 5
isLevel4AndNotValid | 2
isLevel3AndNotValid | 2
isLevel2AndNotValid | 3
descriptionOrCertificationsOffLimits | 2
hasTeaserOrSocial | 2
hasAny | 8

The isValidFeatureRequest still looks odd, I can remove the else statements and I can convert the last call into a return statement, which decrease the complexity with one point.

 function isValidFeatureRequest() 
 if (isLevel4AndNotValid()) 
 return false;
 

 if (isLevel3AndNotValid()) 
 return false;
 

 if (isLevel2AndNotValid()) 
 return false;
 

 return !hasAny();
 

My final attempt is this:

providerSchema.pre('save', function (next) 
 if (isValidFeatureRequest()) 
 next();
 else 
 next(new Error('your current plan does not have this feature'));
 

 function isValidFeatureRequest() 
 if (isLevel4AndNotValid()) 
 return false;
 

 if (isLevel3AndNotValid()) 
 return false;
 

 if (isLevel2AndNotValid()) 
 return false;
 

 return !hasAny();
 

 function isLevel4AndNotValid() 
 return this.level === 4 && descriptionOrCertificationsOffLimits(80, 5);
 

 function isLevel3AndNotValid() 
 return this.level === 3 && descriptionOrCertificationsOffLimits(40, 3);
 

 function isLevel2AndNotValid() 

 function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) 

 function hasTeaserOrSocial() this.social.length > 0;
 

 function hasAny() 
 this.workingHourEnd 
);

With the following resuts:

function | CC
-------------------------------------------
providerSchema | 2
isValidFeatureRequest | 4
isLevel4AndNotValid | 2
isLevel3AndNotValid | 2
isLevel2AndNotValid | 3
descriptionOrCertificationsOffLimits | 2
hasTeaserOrSocial | 2
hasAny | 8

I am not a mongoDB user but is there not some type of validation API, not sure if it can be used on schemes. If it can then maybe that is the better option for your code.

The Question

It have been getting complexity warnings and tried to rewrite it.

Cyclomatic complexity (CC) is a count of the paths through a section of code. Note some code linters use a different metric than described in this answer.

Calculating Cyclomatic Complexity.

To get an estimate of the CC you can count the paths in the section of code.

For example the following functions have 2 paths and thus have a CC of 2

function bar(foo) 
 if (foo === 2) foo = 3 // path 1
 else foo = 4 // path 2
 return foo;



function bar(foo) 
 if (foo === 2) foo = 3 // path 1
 // hidden else path 2
 return foo;

If we add another if statement we get another path. The next function has a CC of 3

function bar(foo) 
 if (foo === 2) foo = 3 // path 1
 else if (foo === 4) foo = 5 // path 2
 else foo = 4 // path 3
 return foo;

It is not just the if statement that creates a path, each clause in a statement creates a path. Thus the following function also has a CC of 3

function bar(foo) foo === 4) foo = 3 // path 1 and 2
 else foo = 4 // path 3
 return foo;

Things get a little involved when you are using functions. CC is calculated per function so the next example will have a median CC of 3 and a max CC of 3. The CC of (bar + poo) / number of functions

function poo(foo) foo === 4) foo = 3 
 else foo = 4 
 return foo;

function bar(foo) foo === 4) foo = 3 
 else foo = poo(foo) 
 return foo;

Your function

Counting the clauses in your function (below) I estimate the CC to be near 20, which is in the high range. Counting the first version in your question has a lot of nested branches so that may have a value near 30.

providerSchema.pre('save', function(next) this.locationLong );

The answer by Margon has separated the code into two functions. This will reduce the median CC. However it has failed to spread the complexity across the two functions, this will drive the max CC up. The first functions CC is 2 and validateData is about 17 giving a median CC of (2 + 17) / 2 ~= 10 and a max CC of 17.

Reducing the CC

As you can see moving code into functions can go a long way to reduce the complexity.

Another way is to remove branching paths altogether. Consider the following function

function foo(a) 
 if(a === 1) a = 2 
 else if (a === 2) a = 3 
 else if (a === 3) a = 4 
 else a = undefined 
 return a;

it has a CC of 4. Now we can do the same with only one path by using a lookup to take the place of the if statements.

function foo(a) 
 return ("1": 2, "2": 3, "3": 4)[a];

The function above has a CC of 1. There is one path yet 4 outcomes.

Applying to your code

Using a combination of functions and lookups we can reduce the CC of you code considerably. However I will point out that CC is but a metric and is only part of what makes good or bad code. Paying too much attention on the CC can be to the detriment of the source code quality. Good code is a balance of many metrics.

Example

There are 8 functions one lookup (object levels). The CC are about (in order top to bottom) 2 (outer function), 3, 4, 1, 1, 2, 2, and 5 so the median CC is (2 + 3 + 4 + 1 + 1 + 2 + 2 + 5) / 8 = 20 / 8 ~= 2 and the max CC is 5.

providerSchema.pre('save', function(next) );

Summing up.

From a high CC of around 20 down to 2. Now the questions that remain are.

• Is it more readable? That is debatable, it is hard for me to tell as I am good at reading my own style.


• Is it more manageable? Yes making changes or adding conditions is simpler as a lot of repeated clauses have been removed or grouped in functions.


• Is it worth the effort? That is up to the coder!

I would suggest to refactor the code to make it cleaner using a function that checks user level and limits

function validateData(data) 

 switch(data.level) data.teaser 


providerSchema.pre('save', function(next) 

 if(validateData(this))
 next(new Error('your current plan does not have this feature'));
 else 
 next()
 
);

I think this could be improved again, but that's a starting point