Why is this for loop not returning the intended output?

Question

This is just an exercise in a course, this app selects a random fruit from the fruits array and then the removeItem() function is supposed to remove that one from the array and returns the modified array, but I'm getting a weird output, and the function is not working.

The problem can be seen here

function randomFruit(importedArray) {
  let random = Math.floor(Math.random() * importedArray.length);
  return importedArray[random];
}

function removeItem(importedArray, item) {
  for (let i = 0; i < importedArray.length; i++) {
    if (importedArray[i] === item) {
      importedArray.splice(i, 1);
      return [...importedArray.slice(0, i), ...importedArray.slice(i + 1)];
    } else {
      return "not found";
    }
  }
}

function makeFruitArray() {
  var foods = ["🍒", "🍉", "🍑", "🍐", "🍏"];
  return foods;
}

let fruitArray = makeFruitArray();
let fruitItem = randomFruit(fruitArray);
let remaining = removeItem(fruitArray, fruitItem);

console.log({fruitArray, fruitItem, remaining});

Please post the output as text here instead of links to images, so it's easier for us to understand what's "wrong". — deceze
– deceze ♦, Commented May 16, 2022 at 14:43
You should explain in English what the actual problem is: "When a cherry is picked, it seems to also remove the watermelon from the list". Try to minimize the work others will have to do when scanning through your question. — Ruan Mendes
– Ruan Mendes, Commented May 16, 2022 at 14:58
Thanks you for the advice, I'm learning and will take that to account when asking questions in the future🙏 — randle
– randle, Commented May 16, 2022 at 15:10
Note that your question has nothing to do with React. I added a live example you can use that demonstrates your non-working code. You should always to remove any code that is not related to the problem — Ruan Mendes
– Ruan Mendes, Commented May 16, 2022 at 15:21

himayan · Accepted Answer · 2022-05-16 15:34:00Z

2

There are two issues in the removeItem function -

If the random one is not the first item on the array, the function returns not found. It wouldn't run for the second loop at all, as your function returns not found after the first iteration.
The splice method updates the original array. While you are passing the fruitArray to the removeItem method, it gets passed as reference and updating it within the function using splice will update the actual array as well.

The simplest and safest way of removing an item from an array would be -

function removeItem(importedArray, item) {
  const filteredArray = importedArray.filter((each) => each !== item);
  if (filteredArray.length === 0) return 'Not Found';
  return filteredArray;
}

edited May 16, 2022 at 15:34

answered May 16, 2022 at 14:57

himayan

8424 silver badges9 bronze badges

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

6 Comments

Ron Hillel Over a year ago

There will be an issue if there are two items of the same type (value).

Ruan Mendes Over a year ago

Why are you suggesting that splice should not be used? Using filter to remove an element may cause it to scan the entire array even if it finds a match at the first element. No need for the includes test, is there? There's a lot of unnecessary work.

Ruan Mendes Over a year ago

@RonHillel From the question and the OP's code, it doesn't look like there would be two items of the same type.

himayan Over a year ago

@JuanMendes you are right. It was not the most performant solution. I have updated it. This would most likely address the problems you mentioned.

Ruan Mendes Over a year ago

@himayan It still suffers from the problem you actually mentioned 😜 You are modifying an array that you were using for displaying the full list, you can create a new array spreading the array as you suggested before. I was just saying that you weren't explaining why they shouldn't modify the array in place (because of the bug I just mentioned)

|

Nimantha · Accepted Answer · 2023-10-28 17:12:26Z

There were two major problems:

Your for loop in removeItem was always returning on the first iteration
You were modifying the initial array as you removed items from it

I've also removed all the unnecessary code used to reproduce your problem.

function randomFruit(importedArray) {
  let random = Math.floor(Math.random() * importedArray.length);
  return importedArray[random];
}

function removeItem(importedArray, item) {
  for (let i = 0; i < importedArray.length; i++) {
    if (importedArray[i] === item) {
      // Don't modify the original array since we want to
      // display the original fruits  
      return [...importedArray.slice(0, i), ...importedArray.slice(i + 1)];
    }
  }
  // Error condition goes outside the loop, not inside.
  return null;
}

function makeFruitArray() {
  var foods = ["🍒", "🍉", "🍑", "🍐", "🍏"];
  return foods;
}

let fruitArray = makeFruitArray();
let fruitItem = randomFruit(fruitArray);
let remaining = removeItem(fruitArray, fruitItem);

console.log({fruitArray, fruitItem, remaining});

Vikrant Kashyap · Accepted Answer · 2022-05-17 09:30:28Z

1

Your remove item function is not working correctly. Instead of writing loops and splicing the array to create a new one you should just use the filter method

function removeItem(importedArray, item) {
    let newArray = importedArray.filter(function (element) {
        return element !== item;
    });
    return newArray;
}

edited May 17, 2022 at 9:30

Vikrant Kashyap

6,9343 gold badges34 silver badges53 bronze badges

answered May 16, 2022 at 15:10

Hammad Ali

16811 bronze badges

4 Comments

Ruan Mendes Over a year ago

yes, slightly slow since there's no need to scan the entire array, but it works

randle Over a year ago

@JuanMendes Can you please explain why is this solution slower?

Hammad Ali Over a year ago

I think it's because I am scanning the entire array which has n complexity. There might be a better way to do it.

Ruan Mendes Over a year ago

Yes, it's because if you use a regular for loop, you can exit the loop as soon as you find your item whereas filter will keep iterating over the entire array.

Nimantha · Accepted Answer · 2023-10-28 02:45:16Z

1

As himayan said, the issue was that splice already changes the array.

Here's my solution:

function removeItem(importedArray, item) {
  for (let i = 0; i < importedArray.length; i++) {
    if (importedArray[i] === item) {
      importedArray.splice(i, 1);
      break;
    }
  }

  return importedArray;
}

edited Oct 28, 2023 at 2:45

Nimantha

6,5376 gold badges32 silver badges78 bronze badges

answered May 16, 2022 at 15:01

Ron Hillel

1609 bronze badges

6 Comments

Ruan Mendes Over a year ago

But if you modify the array, then the display the existing array will be broken...

Ron Hillel Over a year ago

It's in the function scope, not the app scope. The function doesn't need a record of the original array afterward.

Ruan Mendes Over a year ago

It does need it, it displays the original array also. I've made your code runnable so you can see how it doesn't work.

Ron Hillel Over a year ago

:) It does work. It's not displaying the array in the removeItem function scope...

Dave Newton Over a year ago

@RonHillel importedArray is a reference to the original array; this modifies the original array. That's why the OP was spreading into a new array.

|

Collectives™ on Stack Overflow

Why is this for loop not returning the intended output?

4 Answers 4

6 Comments

Comments

4 Comments

6 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

6 Comments

Comments

4 Comments

6 Comments

Your Answer

Sign up or log in

Post as a guest

Related