10
votes
\$\begingroup\$

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?

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

enter image description here

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 and all necessary ingredients
  • Clicking on already highlighted food item will remove the highlighting from the food item and all necessary ingredients
  • Clicking on an ingredient will highlight the ingredient
  • Clicking on already highlighted ingredient will remove the highlighting from the ingredient

The above is basic logic but here is where it gets complicated. There are couple of scenarios I'm trying to handle in the code above (these are independent scenarios):

  1. I click on Spaghetti Bolognese. Necessary ingredients, including onion, get highlighted. Then I click on onion, indicating that I already have this ingredient at home. The highlighting is removed from the onion. Then I click on Pork Stew (which requires onion) but onion shouldn't get highlighted because I have already indicated that I do have an onion at home.

  2. I click on Spaghetti Bolognese. Necessary ingredients, including onion, get highlighted. Then I click on Pork Stew. Pork Stew requires onion too so the onion item stays highlighted. But then I decide I don't want to make Spaghetti Bolognese after all so I unclick it. The onion should stay highlighted because it's required for Pork Stew.

So to handle these kind of scenarios I came up with two arrays: weNeed and weHave.

Here is a list of inputs/outputs:

  • ingr: integer; clicked ingredient
  • buy: unique list of ingredients, concatenation of weNeed + food (food is an array of ingredients coming from the FOOD list; not in the above code)
  • weNeed: list of ingredients collected when user clicks on unhighlighted ingredient item
  • weHave: list of ingredients collected when user clicks on highlighted ingredient item

P.S. Looking back at my question this has grown into a monster. I'm not sure if the extra information I provided is helpful or even more confusing. I could post the whole code if it helps but something is telling me that dealing with these kind of complex issues on the forum is probably not the right way to do things. I'll be happy for any suggestions.

\$\endgroup\$

locked by Jamal Dec 29 '14 at 16:32

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. See the help center for guidance on writing a good question.

Read more about locked posts here.

  • 2
    \$\begingroup\$ You're right, it's confusing. It'd be great if you could write a few lines about what the code's meant to accomplish. Obviously, there are 3 arrays (buy, have, and need) but you're doing some union/intersection/exclusion, but what exactly? \$\endgroup\$ – Flambino Oct 22 '12 at 19:59
  • \$\begingroup\$ Right, I'll provide more information and context but it's going to take some time. Right now the whole code is so mess that even I can't make sense of it :/. I would post the whole code here (150 lines) but it's so bad that it's embarrassing. \$\endgroup\$ – finspin Oct 22 '12 at 20:20
  • \$\begingroup\$ Well, we only really need to know what the inputs (the 3 arrays) are and what the output is supposed to be; we don't need the entire code necessarily. To me it looks like a shopping list system of some sort, maybe? I'm guessing ingr is short for "ingredient", and that the code's meant to figure out what needs/doesn't need to be bought (maybe - can't quite figure it out). \$\endgroup\$ – Flambino Oct 22 '12 at 20:32
11
votes
\$\begingroup\$

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

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

2. 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 duplicates

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

\$\endgroup\$
  • \$\begingroup\$ Thanks for your suggestions! Regarding your point #1, I get a list of ingredients for each recipe via ajax call. I provide the recipe (food) id and the call returns a list of ingredients for the given recipe. Is your suggestion still applicable in this situation? (sorry if my question sounds dumb). Regarding the suggestion #2, I'll definitely check out the underscore.js library. I'm currently using some custom functions for concatenating arrays and returning unique set of values but from what I see underscore.js makes it even simpler. \$\endgroup\$ – finspin Oct 23 '12 at 11:14
  • \$\begingroup\$ @finspin Yes, you should be able to at least "build your own" objects in JS. E.g. take ingredients array you get and toss that in an object. Plenty of ways to structure that. The point is mostly to have a logic encapsulation of "a recipe" and its ingredients. \$\endgroup\$ – Flambino Oct 23 '12 at 12:05
  • \$\begingroup\$ @finspin btw, just corrected some code in my answer - was using map where I'd meant to write reduce \$\endgroup\$ – Flambino Oct 23 '12 at 15:52
6
votes
\$\begingroup\$

You have 4 actions, two of which are repeated twice...

Action #1 -- repeated twice

weNeed.push(ingr);
whIndex = weHave.indexOf(ingr);
weHave.splice(whIndex, 1);

This action requires that either buy.indexOf(ingr) != -1 && weHave.indexOf(ingr) != -1 && weNeed.indexOf(ingr) === -1 be true, or that buy.indexOf(ingr) === -1 && weHave.indexOf(ingr) != -1 is true. This can be simplified to weHave.indexOf(ingr) != -1

Action #2 -- repeated twice

weHave.push(ingr);
wnIndex = weNeed.indexOf(ingr);
weNeed.splice(wnIndex, 1);

This action requires that either buy.indexOf(ingr) != -1 && weHave.indexOf(ingr) === -1 && weNeed.indexOf(ingr) != -1 or buy.indexOf(ingr) === -1 && weHave.indexOf(ingr) === -1 && weNeed.indexOf(ingr) != -1 be true. This one can be simplified to weHave.indexOf(ingr) === -1 && weNeed.indexOf(ingr) != -1

Action #3

weHave.push(ingr);

This action requires that buy.indexOf(ingr) != -1 && weHave.indexOf(ingr) === -1 && weNeed.indexOf(ingr) === -1 be true.

Action #4

weNeed.push(ingr);

This action requires that buy.indexOf(ingr) === -1 && weHave.indexOf(ingr) === -1 && weNeed.indexOf(ingr) === -1 be true.

That being said, you could structure the logic as follows, and you should end up with the same results. Note that I have not tested this though, so I could have made a mistake...

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
    {
        if( buy.indexOf(ingr) != -1 )
            weHave.push(ingr);
        else
            weNeed.push(ingr);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Did this work for you? \$\endgroup\$ – Drew Chapin Feb 20 '13 at 1:10

Not the answer you're looking for? Browse other questions tagged or ask your own question.