Nodejs Express - Is there any way to reduce the following code to something simpler?

Question

I have the following code that I would like to reduce. This question might be very lame, so sorry about it. I wanted to replace the meal1..10 with a variable in a for loop, but I'm not sure if that can be done in node js.

function hasPortion(meals) {
const portions = ["4", "3", "2", "1", "1/8", "1/4", "1/2"];
if (meals.meal1 != undefined && meals.meal1.activado == "on" && portions.indexOf(meals.meal1.porcion) < 0) { return false; }
if (meals.meal2 != undefined && meals.meal2.activado == "on" && portions.indexOf(meals.meal2.porcion) < 0) { return false; }
if (meals.meal3 != undefined && meals.meal3.activado == "on" && portions.indexOf(meals.meal3.porcion) < 0) { return false; }
if (meals.meal4 != undefined && meals.meal4.activado == "on" && portions.indexOf(meals.meal4.porcion) < 0) { return false; }
if (meals.meal5 != undefined && meals.meal5.activado == "on" && portions.indexOf(meals.meal5.porcion) < 0) { return false; }
if (meals.meal6 != undefined && meals.meal6.activado == "on" && portions.indexOf(meals.meal6.porcion) < 0) { return false; }
if (meals.meal7 != undefined && meals.meal7.activado == "on" && portions.indexOf(meals.meal7.porcion) < 0) { return false; }
if (meals.meal8 != undefined && meals.meal8.activado == "on" && portions.indexOf(meals.meal8.porcion) < 0) { return false; }
if (meals.meal9 != undefined && meals.meal9.activado == "on" && portions.indexOf(meals.meal9.porcion) < 0) { return false; }
if (meals.meal10 != undefined && meals.meal10.activado == "on" && portions.indexOf(meals.meal10.porcion) < 0) { return false; } 
return true;

}

Why isn't `meals.meal` an array? That's the fundamental problem. — ggorlen, Jul 20 '20 at 01:49
Does this answer your question? ["Variable" variables in Javascript?](https://stackoverflow.com/questions/5187530/variable-variables-in-javascript) — ggorlen, Jul 20 '20 at 01:50
@ggorlen in fact initially it was, but I had to changed it because I didn't know how to itinerate the object with handlebars. Now that I learnt, I had to change everything (code+database structure) — Javier Destefanis, Jul 20 '20 at 01:57
As the link indicates, any time you have a bunch of var1, var2, var3... varN, putting it in an array is the correct solution. You can always iterate an array or object collection, but you can never iterate them when they're separate variables, at least not without using a hacky and unsafe solution like concatting numbers and strings and looking them up in the `window` which is basically a giant object anyway. — ggorlen, Jul 20 '20 at 01:59
So, the only way to do it right is by changing meals into an array? — Javier Destefanis, Jul 20 '20 at 02:03
I'd think so. If you show that array there's a bit more that can be said. You need `meals.meal.every(e => e && e.activado === "on" && portions.indexOf(e.porcion) < 0)`. But the point isn't so much that putting it an array helps this one thing, it'll make it usable in every other regard throughout your codebase. — ggorlen, Jul 20 '20 at 02:22

Pylon · Accepted Answer · 2020-07-20T03:10:03.707

If meals only have these 10 properties, you may like this,

function hasPortion(meals) {
    const portions = ["4", "3", "2", "1", "1/8", "1/4", "1/2"];
    for (const prop in meals) {
        const meal = meals[prop];
        if (meal != undefined && meal.activado == "on" && portions.indexOf(meal.porcion) < 0) {
            return false;
        }
    }
    return true;
}

If you have more properties, but only want to check these 10 properties, you may like this,

function hasPortion(meals) {
    const portions = ["4", "3", "2", "1", "1/8", "1/4", "1/2"];
    for (let i = 1; i <= 10; i++) {
        const prop = `meal${i}`;
        const meal = meals[prop];
        if (meal != undefined && meal.activado == "on" && portions.indexOf(meal.porcion) < 0) {
            return false;
        }
    }
    return true;
}

Append: new solution

As we mentioned, you should use an Array to store all meal items, also you may use a Set to store portions because searching in Set is faster than Array.

const meals = [
    { activado: "on", porcion: "4" },
    { activado: "on", porcion: "1/8" },
    { activado: "on", porcion: "3" },
    { activado: "on", porcion: "1/2" },
];

function hasPortion(meals) {
    const portions = new Set(["4", "3", "2", "1", "1/8", "1/4", "1/2"]);
    return !meals.some((meal) => meal.activado === "on" && !portions.has(meal.porcion));
}

// Good case
console.log(hasPortion(meals));                // Output: true
// Add bad item
meals.push({ activado: "on", porcion: "1/3" })
console.log(hasPortion(meals));                // Output: false

Solves the problem but I'd mention that this is basically a hacky antipattern that simulates an array and not really the solution you'd want in your codebase. This sort of thing should never pass code review. — ggorlen, Jul 20 '20 at 02:24
Totally agree. Use an array instead of an object question will be resolved. — Pylon, Jul 20 '20 at 02:30
Thank you very much to both.Your solution @Pylon worked. But as you both said, I would consider to change it. — Javier Destefanis, Jul 20 '20 at 02:35
My pleasure @kasperuza. And I appended an example as our idea mentioned above. — Pylon, Jul 20 '20 at 02:59
@Pylon this is how I have configured the field in the Model table: meals: {type: JSON, default: null}, // { "meals": { "meal_1":... That's the only way worked for me when needed to handle the values in Handlebars side. — Javier Destefanis, Jul 21 '20 at 00:33

Nodejs Express - Is there any way to reduce the following code to something simpler?

1 Answers1