Checks user level and limit the data before saving it to mongoDBMonkey patching extra events in Node.js HTTP/ExpressThoughts on this conversion of code from Step.js to Q promise library?Recursion into git diff tree using nodegitGetting data from service and storing it in MongoDBSaving parsed CSV data to MongoDBCRUDL actions on MongoDB collection using Mongoose and NodeJSPromises in mongooseLoading information about a competition every time the page is loadedOpen/Closed Principle with calculating the total cost of items in a shopping cartSend Response To Browser without DB save confirmation
Can an Area of Effect spell cast outside a Prismatic Wall extend inside it?
What does ゆーか mean?
Is Diceware more secure than a long passphrase?
Don’t seats that recline flat defeat the purpose of having seatbelts?
Can someone publish a story that happened to you?
Rivers without rain
Pre-plastic human skin alternative
Can SQL Server create collisions in system generated constraint names?
A Note on N!
How come there are so many candidates for the 2020 Democratic party presidential nomination?
What are the steps to solving this definite integral?
Why does Mind Blank stop the Feeblemind spell?
How do I deal with a coworker that keeps asking to make small superficial changes to a report, and it is seriously triggering my anxiety?
Is there really no use for MD5 anymore?
Two field separators (colon and space) in awk
How do I check if a string is entirely made of the same substring?
What happened to Captain America in Endgame?
Elements other than carbon that can form many different compounds by bonding to themselves?
How exactly does Hawking radiation decrease the mass of black holes?
How to denote matrix elements succinctly?
How to display Aura JS Errors Lightning Out
can anyone help me with this awful query plan?
What happens to Mjolnir (Thor's hammer) at the end of Endgame?
Do I have an "anti-research" personality?
Checks user level and limit the data before saving it to mongoDB
Monkey patching extra events in Node.js HTTP/ExpressThoughts on this conversion of code from Step.js to Q promise library?Recursion into git diff tree using nodegitGetting data from service and storing it in MongoDBSaving parsed CSV data to MongoDBCRUDL actions on MongoDB collection using Mongoose and NodeJSPromises in mongooseLoading information about a competition every time the page is loadedOpen/Closed Principle with calculating the total cost of items in a shopping cartSend Response To Browser without DB save confirmation
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty margin-bottom:0;
$begingroup$
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)
if(this.description.length >= 80 else if(this.level === 3) else if(this.level === 2) else if(this.level === 1) this.certifications.length > 0 else this.workingHourStart)
next(new Error('your current plan does not have this feature'));
else
next()
);
After:
providerSchema.pre('save', function(next) this.locationLat );
```
javascript node.js comparative-review mongoose cyclomatic-complexity
New contributor
$endgroup$
add a comment |
$begingroup$
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)
if(this.description.length >= 80 else if(this.level === 3) else if(this.level === 2) else if(this.level === 1) this.certifications.length > 0 else this.workingHourStart)
next(new Error('your current plan does not have this feature'));
else
next()
);
After:
providerSchema.pre('save', function(next) this.locationLat );
```
javascript node.js comparative-review mongoose cyclomatic-complexity
New contributor
$endgroup$
$begingroup$
Welcome to Code Review! The standard for this site is for titles to state the task accomplished by the code; otherwise, too many questions would have the same title. See How to Ask.
$endgroup$
– 200_success
7 hours ago
$begingroup$
@200_success oh alright :) thanks for the correction.
$endgroup$
– Arootin Aghazaryan
6 hours ago
add a comment |
$begingroup$
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)
if(this.description.length >= 80 else if(this.level === 3) else if(this.level === 2) else if(this.level === 1) this.certifications.length > 0 else this.workingHourStart)
next(new Error('your current plan does not have this feature'));
else
next()
);
After:
providerSchema.pre('save', function(next) this.locationLat );
```
javascript node.js comparative-review mongoose cyclomatic-complexity
New contributor
$endgroup$
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)
if(this.description.length >= 80 else if(this.level === 3) else if(this.level === 2) else if(this.level === 1) this.certifications.length > 0 else this.workingHourStart)
next(new Error('your current plan does not have this feature'));
else
next()
);
After:
providerSchema.pre('save', function(next) this.locationLat );
```
javascript node.js comparative-review mongoose cyclomatic-complexity
javascript node.js comparative-review mongoose cyclomatic-complexity
New contributor
New contributor
edited 7 hours ago
200_success
132k20158423
132k20158423
New contributor
asked 17 hours ago
Arootin AghazaryanArootin Aghazaryan
363
363
New contributor
New contributor
$begingroup$
Welcome to Code Review! The standard for this site is for titles to state the task accomplished by the code; otherwise, too many questions would have the same title. See How to Ask.
$endgroup$
– 200_success
7 hours ago
$begingroup$
@200_success oh alright :) thanks for the correction.
$endgroup$
– Arootin Aghazaryan
6 hours ago
add a comment |
$begingroup$
Welcome to Code Review! The standard for this site is for titles to state the task accomplished by the code; otherwise, too many questions would have the same title. See How to Ask.
$endgroup$
– 200_success
7 hours ago
$begingroup$
@200_success oh alright :) thanks for the correction.
$endgroup$
– Arootin Aghazaryan
6 hours ago
$begingroup$
Welcome to Code Review! The standard for this site is for titles to state the task accomplished by the code; otherwise, too many questions would have the same title. See How to Ask.
$endgroup$
– 200_success
7 hours ago
$begingroup$
Welcome to Code Review! The standard for this site is for titles to state the task accomplished by the code; otherwise, too many questions would have the same title. See How to Ask.
$endgroup$
– 200_success
7 hours ago
$begingroup$
@200_success oh alright :) thanks for the correction.
$endgroup$
– Arootin Aghazaryan
6 hours ago
$begingroup$
@200_success oh alright :) thanks for the correction.
$endgroup$
– Arootin Aghazaryan
6 hours ago
add a comment |
5 Answers
5
active
oldest
votes
$begingroup$
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) this.certifications.length > 5)
valid = false;
else if (this.level === 3) this.certifications.length > 3)
valid = false;
else if (this.level === 2) this.certifications.length > 0 else if (this.level === 1) this.social.length > 0 else this.social.length > 0
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) else if (this.level === 3) else if (this.level === 2) this.teaser 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) else if (this.level === 3) else if (this.level === 2) this.teaser 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) this.social.length > 0)
return false;
else
return true;
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > 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) this.social.length > 0)
return false;
else if (hasAny())
return false;
return true;
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > 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.certifications.length > 0
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)
if (descriptionOrCertificationsOffLimits(30, 0) else if (hasAny())
return false;
return true;
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > certificationsLimit;
function hasTeaserOrSocial() this.social.length > 0;
function hasAny()
this.certifications.length > 0
);
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()
if (this.level === 4 && descriptionOrCertificationsOffLimits(80, 5))
return false;
else if (this.level === 3 && descriptionOrCertificationsOffLimits(40, 3))
return false;
else if (this.level === 2 && descriptionOrCertificationsOffLimits(30, 0)
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()
this.level === 2 && (descriptionOrCertificationsOffLimits(30, 0)
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()
this.level === 2 && (descriptionOrCertificationsOffLimits(30, 0)
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > certificationsLimit;
function hasTeaserOrSocial() this.social.length > 0;
function hasAny()
this.certifications.length > 0
);
With the following resuts:
function | CC
-------------------------------------------
providerSchema | 2
isValidFeatureRequest | 4
isLevel4AndNotValid | 2
isLevel3AndNotValid | 2
isLevel2AndNotValid | 3
descriptionOrCertificationsOffLimits | 2
hasTeaserOrSocial | 2
hasAny | 8
$endgroup$
$begingroup$
"The if followed by an inner if can be combined into and and operation" — this step changes the behavior since nowhasAny()
is called even when in level 4.
$endgroup$
– Roland Illig
4 hours ago
add a comment |
$begingroup$
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)
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)
if (foo === 2
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.locationLat );
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)
const checkSocial = () => this.description );
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!
$endgroup$
add a comment |
$begingroup$
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)
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
$endgroup$
add a comment |
$begingroup$
Paradigm shift: Table-driven methods
Once logic becomes complex enough, you may find it easier to manage the rules from a data structure than from code. Here's how I picture that working for you here.
Disclaimer: I made some assumptions about your business processes that may not be correct. Definitely review this code for correctness, and maybe rewrite it so that it makes more sense to you.
// For each data field we care about, at what level do various
// conditions on that field become available?
const LEVEL_REQUIREMENTS = [
(description) =>
if (description.length >= 80) return 5; // or maybe Infinity?
if (description.length >= 50) return 4;
if (description.length >= 30) return 3;
if (description) return 2;
return 0;
,
(certifications) =>
if (certifications.length > 5) return 5;
if (certifications.length > 3) return 4;
if (certifications.length > 0) return 3;
return 0;
,
(teaser) => teaser ? 3 : 0,
(social) => social.length > 0 ? 3 : 0,
(locationLat) => locationLat ? 2 : 0,
(locationLong) => locationLong ? 2 : 0,
(workingHourEnd) => workingHourEnd ? 2 : 0,
(workingHourStart) => workingHourStart ? 2 : 0,
];
function validate(data)
return LEVEL_REQUIREMENTS.every(levelRequirement => data.level >= levelRequirement(data));
...
providerSchema.pre('save', function(next)
if (validate(this))
next();
else
next(new Error('your current plan does not have this feature'));
);
Here, LEVEL_REQUIREMENTS
is an array of functions (ES6 arrow functions with parameter destructuring, because I think they look nice - feel free to refactor if you disagree, or if you are restricted to ES5). All of the logic for whether a given data blob is allowed at a given plan level is contained within this array.
That reduces the validation function to a simple "Is your level at or above the plan level required to use each feature?"
You may wish to structure the data differently, to make it easier to tell why a given save request was rejected.
Hopefully this looks similar to how the rest of the business thinks about this feature; the more closely your code matches their conceptions, the easier it will be for them to communicate requirements to you, for you to respond to their requirements, for those requirements to change, and so on.
$endgroup$
add a comment |
$begingroup$
After looking at your code for a while, I think I understood your requirements. They can be summarized in this table:
Level Descrs Certs TSoc Other
1 0 0 no no
2 29 0 no yes
3 49 3 yes yes
4 79 5 yes yes
That's the essence of your feature matrix, and that's probably how it looks in the requirements document. The code should have this data in table form so that later requirements can be adjusted easily, without having to dive deep into the code.
You should have a function that tests if such a plan is satisfied:
const plans =
1: maxDescriptions: 0, maxCertifications: 0, teaserAndSocial: false, other: false,
2: maxDescriptions: 29, maxCertifications: 0, teaserAndSocial: false, other: true,
3: maxDescriptions: 49, maxCertifications: 3, teaserAndSocial: true, other: true,
4: maxDescriptions: 79, maxCertifications: 5, teaserAndSocial: true, other: true
;
function planSatisfied(plan, obj) obj.workingHourEnd
providerSchema.pre('save', function(next)
const plan = plans[this.level] );
Using this code structure, it is easy to:
- see what the actual requirements for the plans are, by only looking at the
plans
table. - change the features of a plan, you just have to edit the
plans
table. - add a new feature to all plans, you just have to add it to the table and then once to the
planSatisfied
function. - understand the structure of the code, since it still uses only functions, if clauses and comparisons.
This should cover the typical changes that you will face. Anything else will need a code rewrite anyway.
$endgroup$
add a comment |
Your Answer
StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");
StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);
else
createEditor();
);
function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader:
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
,
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);
);
Arootin Aghazaryan is a new contributor. Be nice, and check out our Code of Conduct.
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f219167%2fchecks-user-level-and-limit-the-data-before-saving-it-to-mongodb%23new-answer', 'question_page');
);
Post as a guest
Required, but never shown
5 Answers
5
active
oldest
votes
5 Answers
5
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
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) this.certifications.length > 5)
valid = false;
else if (this.level === 3) this.certifications.length > 3)
valid = false;
else if (this.level === 2) this.certifications.length > 0 else if (this.level === 1) this.social.length > 0 else this.social.length > 0
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) else if (this.level === 3) else if (this.level === 2) this.teaser 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) else if (this.level === 3) else if (this.level === 2) this.teaser 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) this.social.length > 0)
return false;
else
return true;
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > 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) this.social.length > 0)
return false;
else if (hasAny())
return false;
return true;
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > 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.certifications.length > 0
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)
if (descriptionOrCertificationsOffLimits(30, 0) else if (hasAny())
return false;
return true;
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > certificationsLimit;
function hasTeaserOrSocial() this.social.length > 0;
function hasAny()
this.certifications.length > 0
);
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()
if (this.level === 4 && descriptionOrCertificationsOffLimits(80, 5))
return false;
else if (this.level === 3 && descriptionOrCertificationsOffLimits(40, 3))
return false;
else if (this.level === 2 && descriptionOrCertificationsOffLimits(30, 0)
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()
this.level === 2 && (descriptionOrCertificationsOffLimits(30, 0)
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()
this.level === 2 && (descriptionOrCertificationsOffLimits(30, 0)
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > certificationsLimit;
function hasTeaserOrSocial() this.social.length > 0;
function hasAny()
this.certifications.length > 0
);
With the following resuts:
function | CC
-------------------------------------------
providerSchema | 2
isValidFeatureRequest | 4
isLevel4AndNotValid | 2
isLevel3AndNotValid | 2
isLevel2AndNotValid | 3
descriptionOrCertificationsOffLimits | 2
hasTeaserOrSocial | 2
hasAny | 8
$endgroup$
$begingroup$
"The if followed by an inner if can be combined into and and operation" — this step changes the behavior since nowhasAny()
is called even when in level 4.
$endgroup$
– Roland Illig
4 hours ago
add a comment |
$begingroup$
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) this.certifications.length > 5)
valid = false;
else if (this.level === 3) this.certifications.length > 3)
valid = false;
else if (this.level === 2) this.certifications.length > 0 else if (this.level === 1) this.social.length > 0 else this.social.length > 0
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) else if (this.level === 3) else if (this.level === 2) this.teaser 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) else if (this.level === 3) else if (this.level === 2) this.teaser 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) this.social.length > 0)
return false;
else
return true;
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > 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) this.social.length > 0)
return false;
else if (hasAny())
return false;
return true;
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > 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.certifications.length > 0
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)
if (descriptionOrCertificationsOffLimits(30, 0) else if (hasAny())
return false;
return true;
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > certificationsLimit;
function hasTeaserOrSocial() this.social.length > 0;
function hasAny()
this.certifications.length > 0
);
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()
if (this.level === 4 && descriptionOrCertificationsOffLimits(80, 5))
return false;
else if (this.level === 3 && descriptionOrCertificationsOffLimits(40, 3))
return false;
else if (this.level === 2 && descriptionOrCertificationsOffLimits(30, 0)
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()
this.level === 2 && (descriptionOrCertificationsOffLimits(30, 0)
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()
this.level === 2 && (descriptionOrCertificationsOffLimits(30, 0)
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > certificationsLimit;
function hasTeaserOrSocial() this.social.length > 0;
function hasAny()
this.certifications.length > 0
);
With the following resuts:
function | CC
-------------------------------------------
providerSchema | 2
isValidFeatureRequest | 4
isLevel4AndNotValid | 2
isLevel3AndNotValid | 2
isLevel2AndNotValid | 3
descriptionOrCertificationsOffLimits | 2
hasTeaserOrSocial | 2
hasAny | 8
$endgroup$
$begingroup$
"The if followed by an inner if can be combined into and and operation" — this step changes the behavior since nowhasAny()
is called even when in level 4.
$endgroup$
– Roland Illig
4 hours ago
add a comment |
$begingroup$
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) this.certifications.length > 5)
valid = false;
else if (this.level === 3) this.certifications.length > 3)
valid = false;
else if (this.level === 2) this.certifications.length > 0 else if (this.level === 1) this.social.length > 0 else this.social.length > 0
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) else if (this.level === 3) else if (this.level === 2) this.teaser 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) else if (this.level === 3) else if (this.level === 2) this.teaser 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) this.social.length > 0)
return false;
else
return true;
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > 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) this.social.length > 0)
return false;
else if (hasAny())
return false;
return true;
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > 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.certifications.length > 0
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)
if (descriptionOrCertificationsOffLimits(30, 0) else if (hasAny())
return false;
return true;
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > certificationsLimit;
function hasTeaserOrSocial() this.social.length > 0;
function hasAny()
this.certifications.length > 0
);
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()
if (this.level === 4 && descriptionOrCertificationsOffLimits(80, 5))
return false;
else if (this.level === 3 && descriptionOrCertificationsOffLimits(40, 3))
return false;
else if (this.level === 2 && descriptionOrCertificationsOffLimits(30, 0)
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()
this.level === 2 && (descriptionOrCertificationsOffLimits(30, 0)
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()
this.level === 2 && (descriptionOrCertificationsOffLimits(30, 0)
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > certificationsLimit;
function hasTeaserOrSocial() this.social.length > 0;
function hasAny()
this.certifications.length > 0
);
With the following resuts:
function | CC
-------------------------------------------
providerSchema | 2
isValidFeatureRequest | 4
isLevel4AndNotValid | 2
isLevel3AndNotValid | 2
isLevel2AndNotValid | 3
descriptionOrCertificationsOffLimits | 2
hasTeaserOrSocial | 2
hasAny | 8
$endgroup$
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) this.certifications.length > 5)
valid = false;
else if (this.level === 3) this.certifications.length > 3)
valid = false;
else if (this.level === 2) this.certifications.length > 0 else if (this.level === 1) this.social.length > 0 else this.social.length > 0
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) else if (this.level === 3) else if (this.level === 2) this.teaser 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) else if (this.level === 3) else if (this.level === 2) this.teaser 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) this.social.length > 0)
return false;
else
return true;
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > 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) this.social.length > 0)
return false;
else if (hasAny())
return false;
return true;
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > 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.certifications.length > 0
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)
if (descriptionOrCertificationsOffLimits(30, 0) else if (hasAny())
return false;
return true;
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > certificationsLimit;
function hasTeaserOrSocial() this.social.length > 0;
function hasAny()
this.certifications.length > 0
);
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()
if (this.level === 4 && descriptionOrCertificationsOffLimits(80, 5))
return false;
else if (this.level === 3 && descriptionOrCertificationsOffLimits(40, 3))
return false;
else if (this.level === 2 && descriptionOrCertificationsOffLimits(30, 0)
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()
this.level === 2 && (descriptionOrCertificationsOffLimits(30, 0)
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()
this.level === 2 && (descriptionOrCertificationsOffLimits(30, 0)
function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) this.certifications.length > certificationsLimit;
function hasTeaserOrSocial() this.social.length > 0;
function hasAny()
this.certifications.length > 0
);
With the following resuts:
function | CC
-------------------------------------------
providerSchema | 2
isValidFeatureRequest | 4
isLevel4AndNotValid | 2
isLevel3AndNotValid | 2
isLevel2AndNotValid | 3
descriptionOrCertificationsOffLimits | 2
hasTeaserOrSocial | 2
hasAny | 8
answered 13 hours ago
Adrian IftodeAdrian Iftode
387111
387111
$begingroup$
"The if followed by an inner if can be combined into and and operation" — this step changes the behavior since nowhasAny()
is called even when in level 4.
$endgroup$
– Roland Illig
4 hours ago
add a comment |
$begingroup$
"The if followed by an inner if can be combined into and and operation" — this step changes the behavior since nowhasAny()
is called even when in level 4.
$endgroup$
– Roland Illig
4 hours ago
$begingroup$
"The if followed by an inner if can be combined into and and operation" — this step changes the behavior since now
hasAny()
is called even when in level 4.$endgroup$
– Roland Illig
4 hours ago
$begingroup$
"The if followed by an inner if can be combined into and and operation" — this step changes the behavior since now
hasAny()
is called even when in level 4.$endgroup$
– Roland Illig
4 hours ago
add a comment |
$begingroup$
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)
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)
if (foo === 2
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.locationLat );
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)
const checkSocial = () => this.description );
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!
$endgroup$
add a comment |
$begingroup$
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)
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)
if (foo === 2
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.locationLat );
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)
const checkSocial = () => this.description );
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!
$endgroup$
add a comment |
$begingroup$
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)
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)
if (foo === 2
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.locationLat );
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)
const checkSocial = () => this.description );
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!
$endgroup$
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)
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)
if (foo === 2
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.locationLat );
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)
const checkSocial = () => this.description );
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!
edited 12 hours ago
answered 13 hours ago
Blindman67Blindman67
10.1k1622
10.1k1622
add a comment |
add a comment |
$begingroup$
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)
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
$endgroup$
add a comment |
$begingroup$
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)
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
$endgroup$
add a comment |
$begingroup$
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)
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
$endgroup$
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)
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
answered 16 hours ago
MargonMargon
1794
1794
add a comment |
add a comment |
$begingroup$
Paradigm shift: Table-driven methods
Once logic becomes complex enough, you may find it easier to manage the rules from a data structure than from code. Here's how I picture that working for you here.
Disclaimer: I made some assumptions about your business processes that may not be correct. Definitely review this code for correctness, and maybe rewrite it so that it makes more sense to you.
// For each data field we care about, at what level do various
// conditions on that field become available?
const LEVEL_REQUIREMENTS = [
(description) =>
if (description.length >= 80) return 5; // or maybe Infinity?
if (description.length >= 50) return 4;
if (description.length >= 30) return 3;
if (description) return 2;
return 0;
,
(certifications) =>
if (certifications.length > 5) return 5;
if (certifications.length > 3) return 4;
if (certifications.length > 0) return 3;
return 0;
,
(teaser) => teaser ? 3 : 0,
(social) => social.length > 0 ? 3 : 0,
(locationLat) => locationLat ? 2 : 0,
(locationLong) => locationLong ? 2 : 0,
(workingHourEnd) => workingHourEnd ? 2 : 0,
(workingHourStart) => workingHourStart ? 2 : 0,
];
function validate(data)
return LEVEL_REQUIREMENTS.every(levelRequirement => data.level >= levelRequirement(data));
...
providerSchema.pre('save', function(next)
if (validate(this))
next();
else
next(new Error('your current plan does not have this feature'));
);
Here, LEVEL_REQUIREMENTS
is an array of functions (ES6 arrow functions with parameter destructuring, because I think they look nice - feel free to refactor if you disagree, or if you are restricted to ES5). All of the logic for whether a given data blob is allowed at a given plan level is contained within this array.
That reduces the validation function to a simple "Is your level at or above the plan level required to use each feature?"
You may wish to structure the data differently, to make it easier to tell why a given save request was rejected.
Hopefully this looks similar to how the rest of the business thinks about this feature; the more closely your code matches their conceptions, the easier it will be for them to communicate requirements to you, for you to respond to their requirements, for those requirements to change, and so on.
$endgroup$
add a comment |
$begingroup$
Paradigm shift: Table-driven methods
Once logic becomes complex enough, you may find it easier to manage the rules from a data structure than from code. Here's how I picture that working for you here.
Disclaimer: I made some assumptions about your business processes that may not be correct. Definitely review this code for correctness, and maybe rewrite it so that it makes more sense to you.
// For each data field we care about, at what level do various
// conditions on that field become available?
const LEVEL_REQUIREMENTS = [
(description) =>
if (description.length >= 80) return 5; // or maybe Infinity?
if (description.length >= 50) return 4;
if (description.length >= 30) return 3;
if (description) return 2;
return 0;
,
(certifications) =>
if (certifications.length > 5) return 5;
if (certifications.length > 3) return 4;
if (certifications.length > 0) return 3;
return 0;
,
(teaser) => teaser ? 3 : 0,
(social) => social.length > 0 ? 3 : 0,
(locationLat) => locationLat ? 2 : 0,
(locationLong) => locationLong ? 2 : 0,
(workingHourEnd) => workingHourEnd ? 2 : 0,
(workingHourStart) => workingHourStart ? 2 : 0,
];
function validate(data)
return LEVEL_REQUIREMENTS.every(levelRequirement => data.level >= levelRequirement(data));
...
providerSchema.pre('save', function(next)
if (validate(this))
next();
else
next(new Error('your current plan does not have this feature'));
);
Here, LEVEL_REQUIREMENTS
is an array of functions (ES6 arrow functions with parameter destructuring, because I think they look nice - feel free to refactor if you disagree, or if you are restricted to ES5). All of the logic for whether a given data blob is allowed at a given plan level is contained within this array.
That reduces the validation function to a simple "Is your level at or above the plan level required to use each feature?"
You may wish to structure the data differently, to make it easier to tell why a given save request was rejected.
Hopefully this looks similar to how the rest of the business thinks about this feature; the more closely your code matches their conceptions, the easier it will be for them to communicate requirements to you, for you to respond to their requirements, for those requirements to change, and so on.
$endgroup$
add a comment |
$begingroup$
Paradigm shift: Table-driven methods
Once logic becomes complex enough, you may find it easier to manage the rules from a data structure than from code. Here's how I picture that working for you here.
Disclaimer: I made some assumptions about your business processes that may not be correct. Definitely review this code for correctness, and maybe rewrite it so that it makes more sense to you.
// For each data field we care about, at what level do various
// conditions on that field become available?
const LEVEL_REQUIREMENTS = [
(description) =>
if (description.length >= 80) return 5; // or maybe Infinity?
if (description.length >= 50) return 4;
if (description.length >= 30) return 3;
if (description) return 2;
return 0;
,
(certifications) =>
if (certifications.length > 5) return 5;
if (certifications.length > 3) return 4;
if (certifications.length > 0) return 3;
return 0;
,
(teaser) => teaser ? 3 : 0,
(social) => social.length > 0 ? 3 : 0,
(locationLat) => locationLat ? 2 : 0,
(locationLong) => locationLong ? 2 : 0,
(workingHourEnd) => workingHourEnd ? 2 : 0,
(workingHourStart) => workingHourStart ? 2 : 0,
];
function validate(data)
return LEVEL_REQUIREMENTS.every(levelRequirement => data.level >= levelRequirement(data));
...
providerSchema.pre('save', function(next)
if (validate(this))
next();
else
next(new Error('your current plan does not have this feature'));
);
Here, LEVEL_REQUIREMENTS
is an array of functions (ES6 arrow functions with parameter destructuring, because I think they look nice - feel free to refactor if you disagree, or if you are restricted to ES5). All of the logic for whether a given data blob is allowed at a given plan level is contained within this array.
That reduces the validation function to a simple "Is your level at or above the plan level required to use each feature?"
You may wish to structure the data differently, to make it easier to tell why a given save request was rejected.
Hopefully this looks similar to how the rest of the business thinks about this feature; the more closely your code matches their conceptions, the easier it will be for them to communicate requirements to you, for you to respond to their requirements, for those requirements to change, and so on.
$endgroup$
Paradigm shift: Table-driven methods
Once logic becomes complex enough, you may find it easier to manage the rules from a data structure than from code. Here's how I picture that working for you here.
Disclaimer: I made some assumptions about your business processes that may not be correct. Definitely review this code for correctness, and maybe rewrite it so that it makes more sense to you.
// For each data field we care about, at what level do various
// conditions on that field become available?
const LEVEL_REQUIREMENTS = [
(description) =>
if (description.length >= 80) return 5; // or maybe Infinity?
if (description.length >= 50) return 4;
if (description.length >= 30) return 3;
if (description) return 2;
return 0;
,
(certifications) =>
if (certifications.length > 5) return 5;
if (certifications.length > 3) return 4;
if (certifications.length > 0) return 3;
return 0;
,
(teaser) => teaser ? 3 : 0,
(social) => social.length > 0 ? 3 : 0,
(locationLat) => locationLat ? 2 : 0,
(locationLong) => locationLong ? 2 : 0,
(workingHourEnd) => workingHourEnd ? 2 : 0,
(workingHourStart) => workingHourStart ? 2 : 0,
];
function validate(data)
return LEVEL_REQUIREMENTS.every(levelRequirement => data.level >= levelRequirement(data));
...
providerSchema.pre('save', function(next)
if (validate(this))
next();
else
next(new Error('your current plan does not have this feature'));
);
Here, LEVEL_REQUIREMENTS
is an array of functions (ES6 arrow functions with parameter destructuring, because I think they look nice - feel free to refactor if you disagree, or if you are restricted to ES5). All of the logic for whether a given data blob is allowed at a given plan level is contained within this array.
That reduces the validation function to a simple "Is your level at or above the plan level required to use each feature?"
You may wish to structure the data differently, to make it easier to tell why a given save request was rejected.
Hopefully this looks similar to how the rest of the business thinks about this feature; the more closely your code matches their conceptions, the easier it will be for them to communicate requirements to you, for you to respond to their requirements, for those requirements to change, and so on.
answered 5 hours ago
benj2240benj2240
71118
71118
add a comment |
add a comment |
$begingroup$
After looking at your code for a while, I think I understood your requirements. They can be summarized in this table:
Level Descrs Certs TSoc Other
1 0 0 no no
2 29 0 no yes
3 49 3 yes yes
4 79 5 yes yes
That's the essence of your feature matrix, and that's probably how it looks in the requirements document. The code should have this data in table form so that later requirements can be adjusted easily, without having to dive deep into the code.
You should have a function that tests if such a plan is satisfied:
const plans =
1: maxDescriptions: 0, maxCertifications: 0, teaserAndSocial: false, other: false,
2: maxDescriptions: 29, maxCertifications: 0, teaserAndSocial: false, other: true,
3: maxDescriptions: 49, maxCertifications: 3, teaserAndSocial: true, other: true,
4: maxDescriptions: 79, maxCertifications: 5, teaserAndSocial: true, other: true
;
function planSatisfied(plan, obj) obj.workingHourEnd
providerSchema.pre('save', function(next)
const plan = plans[this.level] );
Using this code structure, it is easy to:
- see what the actual requirements for the plans are, by only looking at the
plans
table. - change the features of a plan, you just have to edit the
plans
table. - add a new feature to all plans, you just have to add it to the table and then once to the
planSatisfied
function. - understand the structure of the code, since it still uses only functions, if clauses and comparisons.
This should cover the typical changes that you will face. Anything else will need a code rewrite anyway.
$endgroup$
add a comment |
$begingroup$
After looking at your code for a while, I think I understood your requirements. They can be summarized in this table:
Level Descrs Certs TSoc Other
1 0 0 no no
2 29 0 no yes
3 49 3 yes yes
4 79 5 yes yes
That's the essence of your feature matrix, and that's probably how it looks in the requirements document. The code should have this data in table form so that later requirements can be adjusted easily, without having to dive deep into the code.
You should have a function that tests if such a plan is satisfied:
const plans =
1: maxDescriptions: 0, maxCertifications: 0, teaserAndSocial: false, other: false,
2: maxDescriptions: 29, maxCertifications: 0, teaserAndSocial: false, other: true,
3: maxDescriptions: 49, maxCertifications: 3, teaserAndSocial: true, other: true,
4: maxDescriptions: 79, maxCertifications: 5, teaserAndSocial: true, other: true
;
function planSatisfied(plan, obj) obj.workingHourEnd
providerSchema.pre('save', function(next)
const plan = plans[this.level] );
Using this code structure, it is easy to:
- see what the actual requirements for the plans are, by only looking at the
plans
table. - change the features of a plan, you just have to edit the
plans
table. - add a new feature to all plans, you just have to add it to the table and then once to the
planSatisfied
function. - understand the structure of the code, since it still uses only functions, if clauses and comparisons.
This should cover the typical changes that you will face. Anything else will need a code rewrite anyway.
$endgroup$
add a comment |
$begingroup$
After looking at your code for a while, I think I understood your requirements. They can be summarized in this table:
Level Descrs Certs TSoc Other
1 0 0 no no
2 29 0 no yes
3 49 3 yes yes
4 79 5 yes yes
That's the essence of your feature matrix, and that's probably how it looks in the requirements document. The code should have this data in table form so that later requirements can be adjusted easily, without having to dive deep into the code.
You should have a function that tests if such a plan is satisfied:
const plans =
1: maxDescriptions: 0, maxCertifications: 0, teaserAndSocial: false, other: false,
2: maxDescriptions: 29, maxCertifications: 0, teaserAndSocial: false, other: true,
3: maxDescriptions: 49, maxCertifications: 3, teaserAndSocial: true, other: true,
4: maxDescriptions: 79, maxCertifications: 5, teaserAndSocial: true, other: true
;
function planSatisfied(plan, obj) obj.workingHourEnd
providerSchema.pre('save', function(next)
const plan = plans[this.level] );
Using this code structure, it is easy to:
- see what the actual requirements for the plans are, by only looking at the
plans
table. - change the features of a plan, you just have to edit the
plans
table. - add a new feature to all plans, you just have to add it to the table and then once to the
planSatisfied
function. - understand the structure of the code, since it still uses only functions, if clauses and comparisons.
This should cover the typical changes that you will face. Anything else will need a code rewrite anyway.
$endgroup$
After looking at your code for a while, I think I understood your requirements. They can be summarized in this table:
Level Descrs Certs TSoc Other
1 0 0 no no
2 29 0 no yes
3 49 3 yes yes
4 79 5 yes yes
That's the essence of your feature matrix, and that's probably how it looks in the requirements document. The code should have this data in table form so that later requirements can be adjusted easily, without having to dive deep into the code.
You should have a function that tests if such a plan is satisfied:
const plans =
1: maxDescriptions: 0, maxCertifications: 0, teaserAndSocial: false, other: false,
2: maxDescriptions: 29, maxCertifications: 0, teaserAndSocial: false, other: true,
3: maxDescriptions: 49, maxCertifications: 3, teaserAndSocial: true, other: true,
4: maxDescriptions: 79, maxCertifications: 5, teaserAndSocial: true, other: true
;
function planSatisfied(plan, obj) obj.workingHourEnd
providerSchema.pre('save', function(next)
const plan = plans[this.level] );
Using this code structure, it is easy to:
- see what the actual requirements for the plans are, by only looking at the
plans
table. - change the features of a plan, you just have to edit the
plans
table. - add a new feature to all plans, you just have to add it to the table and then once to the
planSatisfied
function. - understand the structure of the code, since it still uses only functions, if clauses and comparisons.
This should cover the typical changes that you will face. Anything else will need a code rewrite anyway.
edited 3 hours ago
answered 3 hours ago
Roland IlligRoland Illig
12.2k12050
12.2k12050
add a comment |
add a comment |
Arootin Aghazaryan is a new contributor. Be nice, and check out our Code of Conduct.
Arootin Aghazaryan is a new contributor. Be nice, and check out our Code of Conduct.
Arootin Aghazaryan is a new contributor. Be nice, and check out our Code of Conduct.
Arootin Aghazaryan is a new contributor. Be nice, and check out our Code of Conduct.
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f219167%2fchecks-user-level-and-limit-the-data-before-saving-it-to-mongodb%23new-answer', 'question_page');
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
$begingroup$
Welcome to Code Review! The standard for this site is for titles to state the task accomplished by the code; otherwise, too many questions would have the same title. See How to Ask.
$endgroup$
– 200_success
7 hours ago
$begingroup$
@200_success oh alright :) thanks for the correction.
$endgroup$
– Arootin Aghazaryan
6 hours ago