Delete an object from ArrayList by iterator

Question

I want to create a program which would be like a home budget, so I have a class AmountModel (I know that Integer is not so good for id, but it's not a problem now):

import java.time.LocalDate;

public class AmountModel {
  private Integer id;
  private Double amount;
  private CategoryModel categoryModel;
  private LocalDate localDate;

  // getters/setters etc.
}

And in another class I built this deleteAmount method:

static Scanner sc = new Scanner(System.in);

public List<amountModel> deleteAmount() {
    Iterator<AmountModel> it = amountList.iterator();
    while (it.hasNext()) { 
        System.out.println("Choose index to delete ");
        AmountModel am = it.next();
        if (am.getId().equals(sc.nextInt())) {
            it.remove();
        }
        break;
    }
    return amountList;
}

Adding object works good, but when I try to use the delete method I have to put first index.

Example:
I have three objects (with index 0, 1, 2).

When I choose 1 or 2 program do nothing.
When I choose 0 program deletes first index, remains index 1 and 2.
When I choose 2, program do nothing.
When I choose 1, program deletes index 1, remains index 2... etc.

What is wrong with this method?

Move the System.our.println and the call to sc.nextInt() to outside of the loop. Otherwise you're asking for the index once for each item in the list. — mypetlion
– mypetlion, Commented Mar 10, 2020 at 17:26
I think your sc.nextInt() call needs to be outside the while loop, so that every index in the list is compared. Currently, you are only comparing a single list item every time you read an index to delete. — Jamie
– Jamie, Commented Mar 10, 2020 at 17:26
if (am.getId().equals(sc.nextInt())) { This is just not a good idea at so many levels. Do as @Jamie suggests. — DevilsHnd - 退した
– DevilsHnd - 退した, Commented Mar 10, 2020 at 17:38

Ryan · Accepted Answer · 2020-03-10 17:49:37Z

1

You should separate your input logic from your delete logic and accept the list as a parameter.

Note: this only works with a mutable list. If you use something like Arrays.asList() it will throw an exception.

public void deleteAmount(List<AmountModel> list, int key) {
    list.removeIf(a -> a.getId().equals(key));
}

answered Mar 10, 2020 at 17:49

Ryan

1,7588 silver badges11 bronze badges

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

Comments

Jesse Stuart · Accepted Answer · 2020-03-10 18:17:49Z

Welcome to Stack Overflow!

As others have mentioned, there are a few ways to tackle this. But I think you can make this even simpler by changing the data structure used to access your AmountModel collection: if you're frequently accessing an item by ID, a Map is a great fit.

No more worrying about iterator state; you could just do something like:

// Map "amounts" by ID for easy O(1) lookup.
static Map<Integer, AmountModel> amountMap

public void deleteAmount(Integer id) {
  if (!amountMap.containsKey(id)) { 
    // (TODO: Handle invalid input)
    throw new Exception()
  }

  amountMap.remove(id)
  return
}

Hope this helps! I threw together a working example in a gist here if you're interested. (In Groovy, but should be enough to give you the idea)

user3197884 · Accepted Answer · 2020-03-10 17:57:44Z

0

Your break statement is breaking the while loop in the first iteration only. So, it will work only if the first am.getId() matches with your fist input. Also, your sc.nextInt() will keep on scanning for next available input, Remove it from while loop.

static Scanner sc = new Scanner(System.in);
public List<AmoutModel> deleteAmount() {
    Iterator<AmoutModel> it = amountList.iterator();
    Integer scId = sc.nextInt();
    while (it.hasNext()) { 
        System.out.println("Choose index to delete ");
        AmoutModel am = it.next();
        if (am.getId().equals(scId)) {
            it.remove();
            break;
        }
    }
    return amountList;
}

answered Mar 10, 2020 at 17:57

user3197884

262 bronze badges

Comments

Slackow · Accepted Answer · 2020-03-10 17:32:15Z

-1

call your sc.nextInt() outside of the loop, otherwise it will get run everytime the loop returns, as the condition gets reevaluated every time the loop ends. also you could use the remove method of list

    static Scanner sc = new Scanner(System.in);
    public List<AmoutModel> deleteAmount() {
        System.out.println("Choose index to delete ");
        int index = sc.nextInt();
        amountList.remove(index);
        return amountList;
    }

answered Mar 10, 2020 at 17:32

Slackow

3101 silver badge9 bronze badges

Collectives™ on Stack Overflow

Delete an object from ArrayList by iterator

4 Answers 4

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related