Writing a function to add or modify an existing object inside an array

Question

I have an array of objects. Each object has two properties, id and quantity.

I wrote a function to add an object to the array. But the function must check if the object already exists within the array. If it does exist the function must increment the quantity property. Otherwise it must add a new object.

Is there an idiomatic way to achieve the same result? I feel that looping over the array to compare the id of the object at index against the id passed as argument is not the right approach.

I am also suspicious of the found = false temporary variable.

Finally, in Ruby we are encouraged to break down long methods into smaller ones. I am aware that JavaScript is not the same, but I feel that this could be further refactored.

addItem = function(id, items) {
 var found, i;
 found = false;
 i = 0;
 while (i < items.length) {
 if (id === items[i].id) {
 items[i].quantity++;
 found = true;
 break;
 }
 i++;
 }
 if (!found) {
 return items.push({
 id: id,
 quantity: 1
 });
 }
};

Answer 1

I also don't like the found variable, I think you should extract that part to a indexOf method to return the index of the item you want to change, as you only seem to want to do items[i].quantity++; once. This way you can return the index directly. I would also use a for loop rather than while, as you're looping through a fixed number of items.

indexOf = function(id, items) {
 var i = 0;
 var len = items.length;
 for (i = 0; i < len; i++) {
 if (id === items[i].id) {
 return i;
 }
 }
 return -1;
}

This way you can call your indexOf function from your addItem function and check if the return value is -1, if it is then you call items.push, if it's not -1 then you perform items[index].quantity++;.

Instead of using an array structure, you could use more of a Set structure, which can be implemented in JavaScript as seen in this StackOverflow question, assuming that your id can easily be converted to a string.

• Jumbalaya Wanton: Thanks. id is already a string. Can I ask you why a for loop is preferred to a while loop?
• Jumbalaya Wanton: Also, doesn't adding this method now require two loops? I would still need the loop inside the addItem method to increment the quantity at index, right?
• Simon Forsberg: @JumbalayaWanton When you iterate over an array index-by-index, programmers are more used to for-loops. Technically they have about the same performance, but for-loop is more compact, as the initialization, condition-check and increment is done on the same line.
• Simon Forsberg: @JumbalayaWanton No, you don't need to use a loop inside addItem, just increment the quantity at the index stored in the index variable (that you retreived from the indexOf function), as long as that index is >= 0 (meaning that a match was found).

Answer 2

Why are you using an array to do the job of a map? Use the right data structures....

... instead, use a hashmap (associative array) of [id, quantity]. If id exists in hashmap, then increment up the value at hashmap[id].

• rolfl: Brief answers which do not explain why a suggestion is better are often converted to comments on CodeReview. I have taken the liberty of expressing why this 'obvious' suggestion is better than what the oP has... please feel free to revise if you want to.
• Jumbalaya Wanton: I'm not sure I understand this answer. Do you mean to create a hash or arrays? Like {[1, 1], [2, 1]} etc...?
• rolfl: @JumbalayaWanton - see this quick blog about it
• Jumbalaya Wanton: @rolfl OK. I think I understand. I could use { id: [id, quantity] }... so, assuming ids 1 and 2, with values 1 and 1 respectively, I would do { 1: [1, 1], 2: [2, 1] }. Then I can check for the presence of the id in the hash... sorry to sound like a dunce, but this is new stuff for me.
• Simon Forsberg: This is essentially the same thing that I say in the bottom of my answer, where I link to this SO question that shows how it can be done.

Answer 3

I don't really like that found flag. Also using while or for loops ads a bit of extra code to write to get the value of an arrays items. I would use the filter method for this case.

addItem = function(id, items) {
 var foundItem = items.filter(function(item) {
 return item.id === id;
 })[0];
 if (foundItem) {
 foundItem.quantity++;
 } else {
 //return the new length here cause that is what you did
 return items.push({
 id: id,
 quantity: 1
 });
 }
};

Answer 4

You don't need to use the found variable. Use return when done.

You can also use for instead of while.

addItem = function(id, items) {
 for (var i = 0; i < items.length; i++) {
 if (id === items[i].id) {
 items[i].quantity++;
 return;
 }
 }
 return items.push({
 id: id,
 quantity: 1
 });
};