Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
    });
  }
};
share|improve this question

3 Answers 3

up vote 6 down vote accepted

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

share|improve this answer
    
Thanks. id is already a string. Can I ask you why a for loop is preferred to a while loop? –  Jumbalaya Wanton Mar 4 at 21:01
    
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? –  Jumbalaya Wanton Mar 4 at 21:04
    
@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 André Forsberg Mar 4 at 21:06
1  
@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). –  Simon André Forsberg Mar 4 at 21:07

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

share|improve this answer
2  
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. –  rolfl Mar 4 at 22:09
    
I'm not sure I understand this answer. Do you mean to create a hash or arrays? Like {[1, 1], [2, 1]} etc...? –  Jumbalaya Wanton Mar 4 at 22:53
    
@JumbalayaWanton - see this quick blog about it –  rolfl Mar 4 at 23:09
    
@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. –  Jumbalaya Wanton Mar 4 at 23:45
1  
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. –  Simon André Forsberg Mar 5 at 7:24

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
    });
  }
};
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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