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;








7












$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 );
```









share|improve this question









New contributor




Arootin Aghazaryan is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







$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

















7












$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 );
```









share|improve this question









New contributor




Arootin Aghazaryan is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







$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













7












7








7





$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 );
```









share|improve this question









New contributor




Arootin Aghazaryan is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







$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






share|improve this question









New contributor




Arootin Aghazaryan is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.











share|improve this question









New contributor




Arootin Aghazaryan is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









share|improve this question




share|improve this question








edited 7 hours ago









200_success

132k20158423




132k20158423






New contributor




Arootin Aghazaryan is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









asked 17 hours ago









Arootin AghazaryanArootin Aghazaryan

363




363




New contributor




Arootin Aghazaryan is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.





New contributor





Arootin Aghazaryan is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.






Arootin Aghazaryan is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.











  • $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$
    @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










5 Answers
5






active

oldest

votes


















8












$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





share|improve this answer









$endgroup$












  • $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


















5












$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!





share|improve this answer











$endgroup$




















    4












    $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






    share|improve this answer









    $endgroup$




















      1












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






      share|improve this answer









      $endgroup$




















        0












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






        share|improve this answer











        $endgroup$













          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.









          draft saved

          draft discarded


















          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









          8












          $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





          share|improve this answer









          $endgroup$












          • $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















          8












          $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





          share|improve this answer









          $endgroup$












          • $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













          8












          8








          8





          $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





          share|improve this answer









          $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






          share|improve this answer












          share|improve this answer



          share|improve this answer










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















          $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













          5












          $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!





          share|improve this answer











          $endgroup$

















            5












            $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!





            share|improve this answer











            $endgroup$















              5












              5








              5





              $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!





              share|improve this answer











              $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!






              share|improve this answer














              share|improve this answer



              share|improve this answer








              edited 12 hours ago

























              answered 13 hours ago









              Blindman67Blindman67

              10.1k1622




              10.1k1622





















                  4












                  $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






                  share|improve this answer









                  $endgroup$

















                    4












                    $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






                    share|improve this answer









                    $endgroup$















                      4












                      4








                      4





                      $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






                      share|improve this answer









                      $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







                      share|improve this answer












                      share|improve this answer



                      share|improve this answer










                      answered 16 hours ago









                      MargonMargon

                      1794




                      1794





















                          1












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






                          share|improve this answer









                          $endgroup$

















                            1












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






                            share|improve this answer









                            $endgroup$















                              1












                              1








                              1





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






                              share|improve this answer









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







                              share|improve this answer












                              share|improve this answer



                              share|improve this answer










                              answered 5 hours ago









                              benj2240benj2240

                              71118




                              71118





















                                  0












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






                                  share|improve this answer











                                  $endgroup$

















                                    0












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






                                    share|improve this answer











                                    $endgroup$















                                      0












                                      0








                                      0





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






                                      share|improve this answer











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







                                      share|improve this answer














                                      share|improve this answer



                                      share|improve this answer








                                      edited 3 hours ago

























                                      answered 3 hours ago









                                      Roland IlligRoland Illig

                                      12.2k12050




                                      12.2k12050




















                                          Arootin Aghazaryan is a new contributor. Be nice, and check out our Code of Conduct.









                                          draft saved

                                          draft discarded


















                                          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.




                                          draft saved


                                          draft discarded














                                          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





















































                                          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







                                          Popular posts from this blog

                                          Nidaros erkebispedøme

                                          Birsay

                                          Was Woodrow Wilson really a Liberal?Was World War I a war of liberals against authoritarians?Founding Fathers...