1

I have a case where I need to iterate through an array of objects, check a conditional on each element , delete existing object & add new object based on the outcome of the conditional. The code I have currently is as below, but of course it doesn't work.

What is the right approach to iterate through an array while adding / deleting new elements on certain iterations.

var arrayOfObjects = [] // a list to store objects of the same time
for(var i = 0; i < 5; i++){
  arrayOfObjects.push(new someClass());
}

while(true){
  for(var obj in arrayOfObjects){
    // some conditional check on obj
    // if check is true, delete the obj from array & add a new object
    arrayOfObjects.splice(arrayOfObjects.indexOf(obj),1);
    arrayOfObjects.push(new someClass());    
  }
}
Improve this question
7
  • so what seems to be the problem? Commented Nov 27, 2015 at 1:37
  • Iterate the array and nest the object iteration within it,possibly Commented Nov 27, 2015 at 1:40
  • 3
    While true... Looks like an infinite to me. Commented Nov 27, 2015 at 1:40
  • 1
    The OP says in first line it doesn't work. Then it is off-topic for Code Review. Please don't refer them to Code Review when the code is broken Commented Nov 27, 2015 at 1:44
  • Your main issue could be that with while(true) you'll keep on repeating the for loop, over and over again... There is no stopping or breaking out of the while loop Commented Nov 27, 2015 at 1:45

3 Answers 3

Reset to default
4

Modification of an object / array during iteration over it is an antipattern. Consequences are unpredictable. E.g. the loop is iterated over newly added items as well and it can happen that on every iteration a new item is added. Hence there is an infinite loop.

A better solution is to copy / add the items to a new empty object / array.

var result = [];
arrayOfObjects.forEach(function(obj) {
	if(isCopyRequired(obj)) {
		result.push(obj);
	}

	if(isNewObjectRequired(obj)) {
		result.push(new someClass());
	}
});

Improve this answer
Sign up to request clarification or add additional context in comments.

7 Comments

I think map would be better for that: arrayOfObjects.map(function(v){return check(v)? v : new someClass()}) where check does the conditional check. But the OP doesn't seem to want a new array.
@RobG Why? It seems you do not understand what the author asked.
@RobG if you like to use map, you can use it in your answer. Your current solution is really off-topic
I suggested map because it will do exactly what your answer does. However, your answer does not do what the OP wants (modify an existing array) and will return items in a different sequence to that in the OP. My answer also doesn't return them in the OP sequence, but I explained that in my answer. You haven't.
@RobG The author did not require that the object should be modified in the loop. He asked a solution how to avoid possible issues
|
2

If you really need to add the new object to the end of the array

for(var obj = 0; obj < arrayOfObjects.length; obj += 1){
    var index;
    // some conditional check on obj
    // if check is true, delete the obj from array & add a new object
    if(somecondtion) {
        var index = arrayOfObjects.indexOf(obj);
        arrayOfObjects.splice(index, 1);
        arrayOfObjects.push(new someClass());    
        if (index <= obj) {
            obj -= 1;
        }
    }
}

if the removed item is in current position (obj) or earlier in the array, then subtract one from obj, which will effectively mean that the same position in the array will be checked the next iteration - which is what was the next element before the slice was done

Improve this answer

3 Comments

Having posted that and really thinking about the original code, it just doesn't make sense at all (the original code) - I can't really see, with the re-use of the index (obj) as the item searched for in the indexOf, what the purpose could be
The object can be replaced using arrayOfObjects.splice(index, 1, new someClass()). but I don't see how that's better than just assigning the new object to arrayOfObjects[index].
The question adds a new object to the end. There's no reason to change that logic as there may be a logical reason to do so. I don't really grok what the intended logic is trying to achieve. I can almost see it but it's just not there for me
2

The "correct" way to iterate over an Array is to use a for loop, since for..in iterates over all enumerable properties (own and inherited) and order is not guaranteed, so additional checks may be required. for..in is useful for sparse arrays, but they aren't common.

There does not seem to be any rationale to removal of elements or their order in the array. All you need to do to remove them is to assign a reference to some other object to their index in the array, e.g.

while(true){

Do you really want this to loop forever? I think you should just remove the while block. 

for (var i=0, iLen=arrayOfObjects.length; i<iLen; i++) {

  // some conditional check on obj
  // if check is true, delete the obj from array & add a new object
  if (check) {
    arrayOfObjects[i] = new someClass();    
  }
}

The main difference here is that your original code will add the new object at the end of the array, whereas this verison will add the new object at the same index as the old one.

You can also use forEach:

arrrayOfObjects.forEach(function(value, index) {

  // do check on value
  if (check) arrayOfObjects[i] = new someClass();
});
Improve this answer

4 Comments

Your code does not add a new item to the array. It replaces an existing item with a new object.
@AlexanderElgin—yes, that is explained in the answer (I think). If it's not clear, happy to update if you suggest some changes. :-)
Your solution is absolutely incorrect. The author ask about adding items to the array or removing items from the array. The main point that the array length is changed. You wrote that your code adds a new items. But actually your code does not add anything. It modifies an existing item not changing the array length. Hence your solution is absolutely useless.
@AlexanderElgin—it replaces an existing member of the array with a new object. The only difference is that it puts it at the same index of the array, not at the end. It's much more efficient than using splice and push, which the OP didn't provide any justification for using. All that is explained in the answer, which is neither incorrect nor off topic.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.