Hcfyk

Question

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 || this.certifications.length > 5){

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

        } else {

            next()

        }

    } else if(this.level === 3){

        if(this.description.length >= 50 || this.certifications.length > 3){

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

        } else {

            next()

        }

    } else if(this.level === 2){

        if(this.description.length >= 30 || this.certifications.length > 0 || this.teaser || this.social.length > 0){

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

        } else {

            next()

        }

    } else if(this.level === 1){

        if(this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart){

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

        } else {

            next()

        }

    } else {

        if(this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart){

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

        } else {

            next()

        }

});

After:

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



    if(((this.level === 4) && (this.description.length >= 80 || this.certifications.length > 5)) || 

       ((this.level === 3) && (this.description.length >= 50 || this.certifications.length > 3)) ||

       ((this.level === 2) && (this.description.length >= 30 || this.certifications.length > 0 || this.teaser || this.social.length > 0)) || 

       ((this.level === 1 || this.level === 0) && (this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart))){

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

    } else {

         next()

    }

});

```

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. — Apr 26 at 19:21

score 8 · Accepted Answer · 2019-04-26 14:47:16Z

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

  if (foo === 2 || foo === 4) { foo = 3 } // path 1 and 2

  else { foo = 4 } // path 3

  return foo;

}

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

function poo(foo) {

  if (foo === 2 || foo === 4) { foo = 3 }

  else { foo = 4 }

  return foo;

}

function bar(foo) {

  if (foo === 2 || 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) {



    if(((this.level === 4) && (this.description.length >= 80 || this.certifications.length > 5)) || 

       ((this.level === 3) && (this.description.length >= 50 || this.certifications.length > 3)) ||

       ((this.level === 2) && (this.description.length >= 30 || this.certifications.length > 0 || this.teaser || this.social.length > 0)) || 

       ((this.level === 1 || this.level === 0) && (this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart))){

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

    } else {

         next()

    }

});

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 || this.teaser || this.social.length > 0;

    const checkLocation = () => this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart;

    const fail = () => false;

    const levels = {

        "4": {desc: 80, cert: 6, fail},

        "3": {desc: 50, cert: 4, fail},

        "2": {desc: 30, cert: 1, fail() { return checkSocial() } },

        "1": {desc: -1, cert: 1, fail() { return checkSocial() || checkLocation() } },

        "0": {desc: -1, cert: 1, fail() { return checkSocial() || checkLocation() } },

    };



    const checkPass= ({level, description, certifications}) => {

        if(levels[level]) {

            const check = levels[level];

            if(check.fail() && check.desc < description.length && check.cert < certifications.length) {

                return false;

            }

        }

        return true;

    }

    checkPass(this) ? next() : next(new Error("Your current plan does not have this feature."));



});

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!

score 8 · Accepted Answer · 2019-04-27 07:18:52Z

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 within this answer why).

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

The next calls seem duplicate, so let's reduce them first.

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



     var valid = true;

    if (this.level === 4) {

        if (this.description.length >= 80 || this.certifications.length > 5) {

            valid = false;

        }

    } else if (this.level === 3) {

        if (this.description.length >= 50 || this.certifications.length > 3) {

            valid = false;

        }

    } else if (this.level === 2) {

        if (this.description.length >= 30 || this.certifications.length > 0 || this.teaser || this.social.length > 0) {

            valid = false;

        }

    } else if (this.level === 1) {

        if (this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart) {

            valid = false;

        }

    } else {

        if (this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart) {

            valid = false;

        }

    }



    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 lowering CC, in fact it increased to 30, because of the added if statement. However this can let us to split the validation responsibility from enabling the actual feature (as @Margon mentioned).

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

    if (isValidFeatureRequest()) {

        next();

    } else {

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

    }



    function isValidFeatureRequest() {

        if (this.level === 4) {

            if (this.description.length >= 80 || this.certifications.length > 5) {

                return false;

            }

        } else if (this.level === 3) {

            if (this.description.length >= 50 || this.certifications.length > 3) {

                return false;

            }

        } else if (this.level === 2) {

            if (this.description.length >= 30 || this.certifications.length > 0 || this.teaser || this.social.length > 0) {

                return false;

            }

        } else if (this.level === 1) {

            if (this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart) {

                return false;

            }

        } else {

            if (this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart) {

                return false;

            }

        }

        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 blocks have the the same checks for other levels than 2, 3 or 4, so let's merge them.

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

    if (isValidFeatureRequest()) {

        next();

    } else {

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

    }



    function isValidFeatureRequest() {

        if (this.level === 4) {

            if (this.description.length >= 80 || this.certifications.length > 5) {

                return false;

            }

        } else if (this.level === 3) {

            if (this.description.length >= 50 || this.certifications.length > 3) {

                return false;

            }

        } else if (this.level === 2) {

            if (this.description.length >= 30 || this.certifications.length > 0 || this.teaser || this.social.length > 0) {

                return false;

            }

        } else {

            if (this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart) {

                return false;

            }

        }

        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 seems to vary, so I can encapsulate it 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) {

            if (descriptionOrCertificationsOffLimits(30, 0) || this.teaser || this.social.length > 0) {

                return false;

            }

        } else {

            if (this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart) {

                return false;

            }

        }

        return true;

    }



    function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) {

        return this.description.length >= descriptionLimit || 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 it 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) {

            if (descriptionOrCertificationsOffLimits(30, 0) || this.teaser || this.social.length > 0) {

                return false;

            }

        } else if (hasAny()) {

            return false;

        }

        return true;

    }



    function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) {

        return this.description.length >= descriptionLimit || this.certifications.length > certificationsLimit;

    }



    function hasAny() {

        return this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart;

    }

});

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, by displaying one condition per line. 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 the analyzer.

 function hasAny() {

        return this.description ||

            this.certifications.length > 0 ||

            this.teaser ||

            this.social.length > 0 ||

            this.locationLat ||

            this.locationLong ||

            this.workingHourEnd ||

            this.workingHourStart;

    }

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) || hasTeaserOrSocial()) {

                return false;

            }

        } else if (hasAny()) {

            return false;

        }

        return true;

    }



    function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) {

        return this.description.length >= descriptionLimit || this.certifications.length > certificationsLimit;

    }



    function hasTeaserOrSocial() {

        return this.teaser || this.social.length > 0;

    }



    function hasAny() {

        return this.description ||

            this.certifications.length > 0 ||

            this.teaser ||

            this.social.length > 0 ||

            this.locationLat ||

            this.locationLong ||

            this.workingHourEnd ||

            this.workingHourStart;

    }

});

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) || hasTeaserOrSocial()) {

            return false;

        } else if ((this.level === 1 || this.level === 0) && hasAny()) {

            return false;

        }

        return true;

    }

~~The CC doesn't change, but~~ it enables me to extract validation for every level, so I gain smaller functions, with smaller complexity.
Edit to fix a bug here - This step introduced a bug, as @Roland Illig mentioned in the comments (the story of my life when I refactor even a simple if).
After fixing it the CC actually increased with 2, to 11, as I introduced two new checks and I also had to add a new function. end of edit

function isValidFeatureRequest() {

    if (isLevel4AndNotValid()) {

        return false;

    } else if (isLevel3AndNotValid()) {

        return false;

    } else if (isLevel2AndNotValid()) {

        return false;

    } else if (isBellowLevel2AndNotValid()) {

        return false;

    }

    return true;

}



function isLevel4AndNotValid() {

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

}



function isLevel3AndNotValid() {

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

}



function isLevel2AndNotValid() {

    return this.level === 2 && (descriptionOrCertificationsOffLimits(30, 0) || hasTeaserOrSocial());

}



function isBellowLevel2AndNotValid() {

    return (this.level === 1 || this.level === 0) && hasAny();

}

Which are

function                               | CC

-------------------------------------------

providerSchema                         | 2

isValidFeatureRequest                  | 5

isLevel4AndNotValid                    | 2

isLevel3AndNotValid                    | 2

isLevel2AndNotValid                    | 3

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

    }



    if (isBellowLevel2AndNotValid()) {

        return false;

    }



    return true;

}

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;

        }



        if (isBellowLevel2AndNotValid()) {

            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) || hasTeaserOrSocial());

    }



    function isBellowLevel2AndNotValid() {

        return (this.level === 1 || this.level === 0) && hasAny();

    }



    function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) {

        return this.description.length >= descriptionLimit || this.certifications.length > certificationsLimit;

    }



    function hasTeaserOrSocial() {

        return this.teaser || this.social.length > 0;

    }



    function hasAny() {

        return this.description ||

            this.certifications.length > 0 ||

            this.teaser ||

            this.social.length > 0 ||

            this.locationLat ||

            this.locationLong ||

            this.workingHourEnd ||

            this.workingHourStart;

    }

});

With the following resuts:

function                               | CC

-------------------------------------------

providerSchema                         | 2

isValidFeatureRequest                  | 5

isLevel4AndNotValid                    | 2

isLevel3AndNotValid                    | 2

isLevel2AndNotValid                    | 3

isBellowLvel2AndNotValid               | 3

descriptionOrCertificationsOffLimits   | 2

hasTeaserOrSocial                      | 2

hasAny                                 | 8

"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. — Apr 26 at 22:36

MargonMargon 1894 · Accepted Answer · 2019-04-26 10:36:39Z

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

        case 0:

        case 1:

            return data.description || data.certifications.length > 0 || data.teaser || data.social.length > 0 || data.locationLat || data.locationLong || data.workingHourEnd || data.workingHourStart

        case 2: 

            return data.description.length >= 30 || data.certifications.length > 0 || data.teaser || data.social.length > 0);

        case 3: 

            return (data.description.length >= 50 || data.certifications.length > 3));

        case 4: 

            return (data.description.length >= 80 || data.certifications.length > 5);

    }

}



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

score 3 · Accepted Answer · 2019-04-26 23:44:35Z

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

    if (obj.description.length > plan.maxDescriptions) {

        return false;

    }



    if (obj.certifications.length > plan.maxCertifications) {

        return false;

    }



    if (!plan.teaserAndSocial && (obj.teaser || obj.social.length > 0)) {

        return false;

    }



    if (!plan.other && (obj.locationLat || obj.locationLong || obj.workingHourEnd || obj.workingHourStart)) {

        return false;

    }



    return true;

}



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

    const plan = plans[this.level] || plans[1];

    if (planSatisfied(plan, this)) {

        next();

    } else {

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

    }

});

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.

benj2240benj2240 84629 · Accepted Answer · 2019-04-26 21:47:00Z

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.

score 8 · Accepted Answer · 2019-04-26 14:47:16Z

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

  if (foo === 2 || foo === 4) { foo = 3 } // path 1 and 2

  else { foo = 4 } // path 3

  return foo;

}

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

function poo(foo) {

  if (foo === 2 || foo === 4) { foo = 3 }

  else { foo = 4 }

  return foo;

}

function bar(foo) {

  if (foo === 2 || 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) {



    if(((this.level === 4) && (this.description.length >= 80 || this.certifications.length > 5)) || 

       ((this.level === 3) && (this.description.length >= 50 || this.certifications.length > 3)) ||

       ((this.level === 2) && (this.description.length >= 30 || this.certifications.length > 0 || this.teaser || this.social.length > 0)) || 

       ((this.level === 1 || this.level === 0) && (this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart))){

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

    } else {

         next()

    }

});

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 || this.teaser || this.social.length > 0;

    const checkLocation = () => this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart;

    const fail = () => false;

    const levels = {

        "4": {desc: 80, cert: 6, fail},

        "3": {desc: 50, cert: 4, fail},

        "2": {desc: 30, cert: 1, fail() { return checkSocial() } },

        "1": {desc: -1, cert: 1, fail() { return checkSocial() || checkLocation() } },

        "0": {desc: -1, cert: 1, fail() { return checkSocial() || checkLocation() } },

    };



    const checkPass= ({level, description, certifications}) => {

        if(levels[level]) {

            const check = levels[level];

            if(check.fail() && check.desc < description.length && check.cert < certifications.length) {

                return false;

            }

        }

        return true;

    }

    checkPass(this) ? next() : next(new Error("Your current plan does not have this feature."));



});

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!

score 8 · Accepted Answer · 2019-04-27 07:18:52Z

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 within this answer why).

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

The next calls seem duplicate, so let's reduce them first.

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



     var valid = true;

    if (this.level === 4) {

        if (this.description.length >= 80 || this.certifications.length > 5) {

            valid = false;

        }

    } else if (this.level === 3) {

        if (this.description.length >= 50 || this.certifications.length > 3) {

            valid = false;

        }

    } else if (this.level === 2) {

        if (this.description.length >= 30 || this.certifications.length > 0 || this.teaser || this.social.length > 0) {

            valid = false;

        }

    } else if (this.level === 1) {

        if (this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart) {

            valid = false;

        }

    } else {

        if (this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart) {

            valid = false;

        }

    }



    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 lowering CC, in fact it increased to 30, because of the added if statement. However this can let us to split the validation responsibility from enabling the actual feature (as @Margon mentioned).

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

    if (isValidFeatureRequest()) {

        next();

    } else {

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

    }



    function isValidFeatureRequest() {

        if (this.level === 4) {

            if (this.description.length >= 80 || this.certifications.length > 5) {

                return false;

            }

        } else if (this.level === 3) {

            if (this.description.length >= 50 || this.certifications.length > 3) {

                return false;

            }

        } else if (this.level === 2) {

            if (this.description.length >= 30 || this.certifications.length > 0 || this.teaser || this.social.length > 0) {

                return false;

            }

        } else if (this.level === 1) {

            if (this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart) {

                return false;

            }

        } else {

            if (this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart) {

                return false;

            }

        }

        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 blocks have the the same checks for other levels than 2, 3 or 4, so let's merge them.

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

    if (isValidFeatureRequest()) {

        next();

    } else {

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

    }



    function isValidFeatureRequest() {

        if (this.level === 4) {

            if (this.description.length >= 80 || this.certifications.length > 5) {

                return false;

            }

        } else if (this.level === 3) {

            if (this.description.length >= 50 || this.certifications.length > 3) {

                return false;

            }

        } else if (this.level === 2) {

            if (this.description.length >= 30 || this.certifications.length > 0 || this.teaser || this.social.length > 0) {

                return false;

            }

        } else {

            if (this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart) {

                return false;

            }

        }

        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 seems to vary, so I can encapsulate it 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) {

            if (descriptionOrCertificationsOffLimits(30, 0) || this.teaser || this.social.length > 0) {

                return false;

            }

        } else {

            if (this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart) {

                return false;

            }

        }

        return true;

    }



    function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) {

        return this.description.length >= descriptionLimit || 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 it 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) {

            if (descriptionOrCertificationsOffLimits(30, 0) || this.teaser || this.social.length > 0) {

                return false;

            }

        } else if (hasAny()) {

            return false;

        }

        return true;

    }



    function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) {

        return this.description.length >= descriptionLimit || this.certifications.length > certificationsLimit;

    }



    function hasAny() {

        return this.description || this.certifications.length > 0 || this.teaser || this.social.length > 0 || this.locationLat || this.locationLong || this.workingHourEnd || this.workingHourStart;

    }

});

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, by displaying one condition per line. 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 the analyzer.

 function hasAny() {

        return this.description ||

            this.certifications.length > 0 ||

            this.teaser ||

            this.social.length > 0 ||

            this.locationLat ||

            this.locationLong ||

            this.workingHourEnd ||

            this.workingHourStart;

    }

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) || hasTeaserOrSocial()) {

                return false;

            }

        } else if (hasAny()) {

            return false;

        }

        return true;

    }



    function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) {

        return this.description.length >= descriptionLimit || this.certifications.length > certificationsLimit;

    }



    function hasTeaserOrSocial() {

        return this.teaser || this.social.length > 0;

    }



    function hasAny() {

        return this.description ||

            this.certifications.length > 0 ||

            this.teaser ||

            this.social.length > 0 ||

            this.locationLat ||

            this.locationLong ||

            this.workingHourEnd ||

            this.workingHourStart;

    }

});

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) || hasTeaserOrSocial()) {

            return false;

        } else if ((this.level === 1 || this.level === 0) && hasAny()) {

            return false;

        }

        return true;

    }

~~The CC doesn't change, but~~ it enables me to extract validation for every level, so I gain smaller functions, with smaller complexity.
Edit to fix a bug here - This step introduced a bug, as @Roland Illig mentioned in the comments (the story of my life when I refactor even a simple if).
After fixing it the CC actually increased with 2, to 11, as I introduced two new checks and I also had to add a new function. end of edit

function isValidFeatureRequest() {

    if (isLevel4AndNotValid()) {

        return false;

    } else if (isLevel3AndNotValid()) {

        return false;

    } else if (isLevel2AndNotValid()) {

        return false;

    } else if (isBellowLevel2AndNotValid()) {

        return false;

    }

    return true;

}



function isLevel4AndNotValid() {

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

}



function isLevel3AndNotValid() {

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

}



function isLevel2AndNotValid() {

    return this.level === 2 && (descriptionOrCertificationsOffLimits(30, 0) || hasTeaserOrSocial());

}



function isBellowLevel2AndNotValid() {

    return (this.level === 1 || this.level === 0) && hasAny();

}

Which are

function                               | CC

-------------------------------------------

providerSchema                         | 2

isValidFeatureRequest                  | 5

isLevel4AndNotValid                    | 2

isLevel3AndNotValid                    | 2

isLevel2AndNotValid                    | 3

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

    }



    if (isBellowLevel2AndNotValid()) {

        return false;

    }



    return true;

}

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;

        }



        if (isBellowLevel2AndNotValid()) {

            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) || hasTeaserOrSocial());

    }



    function isBellowLevel2AndNotValid() {

        return (this.level === 1 || this.level === 0) && hasAny();

    }



    function descriptionOrCertificationsOffLimits(descriptionLimit, certificationsLimit) {

        return this.description.length >= descriptionLimit || this.certifications.length > certificationsLimit;

    }



    function hasTeaserOrSocial() {

        return this.teaser || this.social.length > 0;

    }



    function hasAny() {

        return this.description ||

            this.certifications.length > 0 ||

            this.teaser ||

            this.social.length > 0 ||

            this.locationLat ||

            this.locationLong ||

            this.workingHourEnd ||

            this.workingHourStart;

    }

});

With the following resuts:

function                               | CC

-------------------------------------------

providerSchema                         | 2

isValidFeatureRequest                  | 5

isLevel4AndNotValid                    | 2

isLevel3AndNotValid                    | 2

isLevel2AndNotValid                    | 3

isBellowLvel2AndNotValid               | 3

descriptionOrCertificationsOffLimits   | 2

hasTeaserOrSocial                      | 2

hasAny                                 | 8

"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. — Apr 26 at 22:36

MargonMargon 1894 · Accepted Answer · 2019-04-26 10:36:39Z

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

        case 0:

        case 1:

            return data.description || data.certifications.length > 0 || data.teaser || data.social.length > 0 || data.locationLat || data.locationLong || data.workingHourEnd || data.workingHourStart

        case 2: 

            return data.description.length >= 30 || data.certifications.length > 0 || data.teaser || data.social.length > 0);

        case 3: 

            return (data.description.length >= 50 || data.certifications.length > 3));

        case 4: 

            return (data.description.length >= 80 || data.certifications.length > 5);

    }

}



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

score 3 · Accepted Answer · 2019-04-26 23:44:35Z

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

    if (obj.description.length > plan.maxDescriptions) {

        return false;

    }



    if (obj.certifications.length > plan.maxCertifications) {

        return false;

    }



    if (!plan.teaserAndSocial && (obj.teaser || obj.social.length > 0)) {

        return false;

    }



    if (!plan.other && (obj.locationLat || obj.locationLong || obj.workingHourEnd || obj.workingHourStart)) {

        return false;

    }



    return true;

}



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

    const plan = plans[this.level] || plans[1];

    if (planSatisfied(plan, this)) {

        next();

    } else {

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

    }

});

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.

benj2240benj2240 84629 · Accepted Answer · 2019-04-26 21:47:00Z

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.

Checks user level and limit the data before saving it to mongoDB

5 Answers 5

Calculating Cyclomatic Complexity.

Your function

Reducing the CC

Applying to your code

Summing up.

Paradigm shift: Table-driven methods

Your Answer

Sign up or log in

Post as a guest

Post as a guest

5 Answers 5

5 Answers 5

Calculating Cyclomatic Complexity.

Your function

Reducing the CC

Applying to your code

Summing up.

Calculating Cyclomatic Complexity.

Your function

Reducing the CC

Applying to your code

Summing up.

Calculating Cyclomatic Complexity.

Your function

Reducing the CC

Applying to your code

Summing up.

Calculating Cyclomatic Complexity.

Your function

Reducing the CC

Applying to your code

Summing up.

Paradigm shift: Table-driven methods

Paradigm shift: Table-driven methods

Paradigm shift: Table-driven methods

Paradigm shift: Table-driven methods

Sign up or log in

Post as a guest

Post as a guest

Sign up or log in

Post as a guest

Sign up or log in

Post as a guest

Sign up or log in

Post as a guest

Popular posts from this blog

Færeyskur hestur Heimild | Tengill | Tilvísanir | LeiðsagnarvalRossið - síða um færeyska hrossið á færeyskuGott ár hjá færeyska hestinum

5 Answers
5

5 Answers
5

5 Answers
5