patternjavascriptModerate
Handling click events on ingredients list
Viewed 0 times
handlingclickeventslistingredients
Problem
I'm learning programming and would appreciate any feedback on my code. I've come up with the below code. The logic works fine but the code itself seems rather confusing to me. Are there some patterns I could use to improve this code?
EDIT: Edited the code above and provided more background information below.
Here is a simplified picture of the application I'm trying to create:
Briefly how the application should work:
// This code snippet handles click events on the Ingredients list (see the image below)
$('.items a').click(function (event) {
// Remove leading letter from the href id (e.g. id="i15" --> 15)
ingr = parseInt((this.id).substring(1));
// Here comes the complicated logic for handling click event
// It works well but can it be improved/simplified?
if (buy.indexOf(ingr) != -1) {
if (weHave.indexOf(ingr) != -1) {
if (weNeed.indexOf(ingr) === -1) {
weNeed.push(ingr);
whIndex = weHave.indexOf(ingr);
weHave.splice(whIndex, 1);
}
} else {
if (weNeed.indexOf(ingr) === -1) {
weHave.push(ingr);
} else {
weHave.push(ingr);
wnIndex = weNeed.indexOf(ingr);
weNeed.splice(wnIndex, 1);
}
}
} else {
if (weHave.indexOf(ingr) != -1) {
weNeed.push(ingr);
whIndex = weHave.indexOf(ingr);
weHave.splice(whIndex, 1);
} else {
if (weNeed.indexOf(ingr) != -1) {
weHave.push(ingr);
wnIndex = weNeed.indexOf(ingr);
weNeed.splice(wnIndex, 1);
} else {
weNeed.push(ingr);
}
}
}
shoplist();
return false; // to prevent the page from scrolling to the top
});EDIT: Edited the code above and provided more background information below.
Here is a simplified picture of the application I'm trying to create:
Briefly how the application should work:
- The idea is to create a shopping list by selecting food and/or ingredients
- Clicking on a food item will highlight the food item an
Solution
Druciferre seems to have found a way to streamline your current code quite nicely (I would add that an "is-value-x-in-array-y?" helper function would greatly improve readability, as all the
There are plenty of ways to do this, but the simplest, most direct one I can think of is:
(can you tell I don't cook much?)
Anyway, the point is that each recipe "contains" its ingredients. A neat logical encapsulation of data.
Underscore's got some neat array functions. Use 'em. Let's say you have both of the above recipes selected. To get all the ingredients needed, you could do
Edit Changed
Thus,
So now you have an array of all the ingredients needed to cook those two dishes.
Now, let's say you also have an array of ingredients that the user has manually selected and one that contains those that he/she already has (i.e. manually unselected):
And, well, that's pretty much it. When you select a new recipe, re-run that code. When you add/remove an ingredient, re-run that code. With just the above you'd get a shopping list of
Of course, this is me taking a stab in the dark, not knowing the rest of your app. But this, to me, seems like a better approach than manually trying to manage arrays element-by-element.
indexOf comparisons make me cross-eyed. A "remove-x-from-array-y" function would also help DRY the code), so allow me to suggest a different tack altogether; more rewrite than tweak. Reading your description, here are my suggestions.- Objects for each recipe
There are plenty of ways to do this, but the simplest, most direct one I can think of is:
var recipes = [
{
name: "Pork stew",
ingredients: ["pork", "stew", "onion"]
},
{
name: "Spaghetti bolognese",
ingredients: ["spaghetti", "city of bologna", "onion"]
},
// ....
];(can you tell I don't cook much?)
Anyway, the point is that each recipe "contains" its ingredients. A neat logical encapsulation of data.
- Use underscore.js (or similar, or roll your own) array functions
Underscore's got some neat array functions. Use 'em. Let's say you have both of the above recipes selected. To get all the ingredients needed, you could do
// concatenate every recipe's ingredients into one array
var requiredIngredients = _.reduce(selectedRecipes, function (ingredients, recipe) {
return ingredients.concat(recipe.ingredients);
}, []);
requiredIngredients = _.uniq(requiredIngredients); // remove duplicatesEdit Changed
_.map to _.reduce which is what I meant to write first time around, but somehow messed up.Thus,
requiredIngredients will be:["pork", "stew", "onion", "spaghetti", "city of bologna"]So now you have an array of all the ingredients needed to cook those two dishes.
Now, let's say you also have an array of ingredients that the user has manually selected and one that contains those that he/she already has (i.e. manually unselected):
var ownedIngredients = ["onion", "olive oil"];
var selectedIngredients = ["curry"];
// combine manually selected ingredients with recipe ingredients,
// and remove duplicates
var allIngredients = _.uniq(requiredIngredients.concat(selectedIngredients));
// "subtract" unselected ingredients
var shoppingList = _.difference(allIngredients, ownedIngredients);And, well, that's pretty much it. When you select a new recipe, re-run that code. When you add/remove an ingredient, re-run that code. With just the above you'd get a shopping list of
["pork", "stew", "spaghetti", "city of bologna", "curry"]Of course, this is me taking a stab in the dark, not knowing the rest of your app. But this, to me, seems like a better approach than manually trying to manage arrays element-by-element.
Code Snippets
var recipes = [
{
name: "Pork stew",
ingredients: ["pork", "stew", "onion"]
},
{
name: "Spaghetti bolognese",
ingredients: ["spaghetti", "city of bologna", "onion"]
},
// ....
];// concatenate every recipe's ingredients into one array
var requiredIngredients = _.reduce(selectedRecipes, function (ingredients, recipe) {
return ingredients.concat(recipe.ingredients);
}, []);
requiredIngredients = _.uniq(requiredIngredients); // remove duplicates["pork", "stew", "onion", "spaghetti", "city of bologna"]var ownedIngredients = ["onion", "olive oil"];
var selectedIngredients = ["curry"];
// combine manually selected ingredients with recipe ingredients,
// and remove duplicates
var allIngredients = _.uniq(requiredIngredients.concat(selectedIngredients));
// "subtract" unselected ingredients
var shoppingList = _.difference(allIngredients, ownedIngredients);["pork", "stew", "spaghetti", "city of bologna", "curry"]Context
StackExchange Code Review Q#17816, answer score: 11
Revisions (0)
No revisions yet.