how to iterate an ArrayList of objects and to access a method of that class to which the object belongs while iterating?

Question

I've a class Employee and a method in that class as getName() which returns the name of the employee. Each employee has some attributes (name, dob, designation, salary) and I store them in an ArrayList.

Now I want to write a method deleteEmployee(String, ArrayList<Employee>) which will compare the string given as the parameter with an employee name and if any match is found, it will remove entire details of that/those employee(s) I have tried as follows and I have got errors & I have no idea how to solve...

 public void deleteEmployee (String str,ArrayList <Employee> arr)
 {
    Employee obj2;

    for (int i = 0; i < arr.size(); i++)
    {
        if(str.equalsIgnoreCase(arr.get(obj2[i].getName())))
            arr.remove(obj2[i]);
        break;
    }
 }

You could use this pattern: http://stackoverflow.com/questions/223918/iterating-through-a-list-avoiding-concurrentmodificationexception-when-removing. — Ioan, Aug 05 '15 at 15:01
There are at least 4 problems with this. `obj2` is never assigned. `obj2` is not an array so you can't write `obj2[i]`. The `break` means the loop doesn't loop. Also I don't understand `arr.get(obj2[i].getName())`. `arr.get` requires an `int`. — Paul Boddington, Aug 05 '15 at 15:12

score 2 · Answer 1 · answered Aug 05 '15 at 15:07

2

You probably have a ConcurrentModificationException or something of this kind. That's because you can't modify an array you are iterating on except with an Iterator

You should try something like this

Iterator<Employee> it = arr.iterator();
Employee currentEmployee = null;
while(it.hasNext()){
   currentEmployee = it.next();
   if(currentEmployee.getName().equals(obj2.getName()){
      it.remove();
   }
}

In order to help solve your problem in the future, please post also your errors and stacktraces.

answered Aug 05 '15 at 15:07

gavard.e

142
11

I think it would have been an IndexOutOfBoundsException - since the OP is not using an iterator, but a for-loop up to the initial size of the list. But the list is getting smaller.... – NickJ Aug 05 '15 at 15:20
@Nick, by contract iterators never throw IndexOutOfBoundsException. Answer is correct. – Viktor Aug 05 '15 at 15:29
Few notes: 1) we can declare variable inside cycle: "Employee currentEmployee = it.next();" 2) removing element from ArrayList tooks O(n) time. – Viktor Aug 05 '15 at 15:31
@Viktor but the original question uses a for-loop to traverse the arraylist, not an iterator. – NickJ Aug 06 '15 at 08:20
Removing an item while iterating an Array is a good way to get an Exception. Only iterators are able to remove while iterating. And no OutOfBoundException too, even when removing items. Furthermore, he could be using for(Employee emp : arr) if he doesn't need that i variable – gavard.e Aug 06 '15 at 08:25

Paul Bilnoski · Accepted Answer · 2015-08-05T15:25:17.183

0

If you only have a single item that matches the name, your approach will work. However, your mistake is here:

if(str.equalsIgnoreCase(arr.get(obj2[i].getName())))

Instead you need to get the object from the array then call getName on the result:

if(str.equalsIgnoreCase(arr.get(i).getName()))

You also need your break in the if block: if(...) { arr.remove(i); break; }

There are certainly other things that could be improved in your implementation as well.

edited Aug 05 '15 at 15:25

answered Aug 05 '15 at 15:08

Paul Bilnoski

556
4
13

I have tried as you mentioned. But, then, how to remove that object??? is it simply "arr.remove(i);" ???? i have written it... but then when i'm printing the list, i have found that the ith 'employee' is not deleted @Paul Bilnoski – Arpan Mukherjee Aug 05 '15 at 15:58
@ArpanMukherjee Did you move the `break` to the block within the `if`? If it is still outside, the loop will break after the first iteration and will only appear to succeed if the matched employee is first in the list. – Paul Bilnoski Aug 05 '15 at 16:03
I have completely removed the break statement.... My display function is as follows... public void displayEmployees(ArrayList xyz) { for(Employee object:xyz) System.out.println("Name : "+object.getName()+", Employee-I'd : "+object.getEmployeeID()+", Date-of-Birth : "+formatterx.format(object.getDob())+", Designation : "+object.getDesignation()+", Salary : "+object.getSalary()); } & i'm calling ob.deleteEmployee(p,arr); ob.displayEmployees(arr); where 'p' is the string & 'arr' is the arraylist containg the details of the employees – Arpan Mukherjee Aug 05 '15 at 16:10

score 0 · Answer 3 · answered Aug 05 '15 at 15:58

As far as I can see, there are a few problems with your code.

First of all, using break is not very good practice. Especially using it where you've used it. In your code, after the if statement processes, the for loop ends. Regardless of the result of the if statement, the for loop will not iterate. So unless you only want to check if the desired employee is at index 0, this is far from desirable.

for (int i = 0; i < arr.size(); i++)

This is the second issue problem. It works well enough in the ideal case due to the break statement you use, but if the desired employee is NOT in the ArrayList, you'll get an IndexOutOfBoundsException because on the last iteration, you'll be trying to get the index of the size of the ArrayList, which doesn't exist. Instead, your for loop should be

for (int i = 0; i < arr.size() - 1; i++)

This way, in the event of the employee not being present, the loop will stop when it gets to the last index of the ArrayList, which is 1 less than the size.

Another, more immediate problem is this line:

if(str.equalsIgnoreCase(arr.get(obj2[i].getName())))

In this line, you're calling arr.get with an Employee object as the argument, when you should be using an index of the ArrayList.

if(str.equalsIgnoreCase(arr.get(i).getName()))

With this modification, you call an index of the ArrayList, which will return an Employee object. You then call getName() on that object, and it returns the string, which is compared for equality to str, like you intend to do.

One last problem is the line where you remove the object.

arr.remove(obj2[i]);

Again, you seem to be trying to access an index of the Employee object, rather than the ArrayList. Changing this line to:

arr.remove(i);

would be much more effective. This way, you're removing the index of the array. You CAN use an object as an argument for the remove() method, but you would first need to assign the object at i to obj2, and it's altogether far more troublesome. So, putting everything together, the new, fixed loop would be

for (int i = 0; i < arr.size() - 1; i++){
   if(str.equalsIgnoreCase(arr.get(i).getName())){
      arr.remove(i);
   }
}

Assuming that there will only be one employee with a given name, this will not be a problem, but be warned that there are issues with removing objects inside ArrayLists while iterating through the lists. For example, if you remove the object at i, then the objects after it in the ArrayList all shift downward by one index. Then if you iterate again, you're accessing the object at i+1 in the List. BUT, the ACTUAL object at i+1 is the object that was originally at i+2. So if you remove an object in the ArrayList, you'll skip examining the object coming directly after it on the next iteration. One simple solution to this is to iterate BACKWARDS through the ArrayList, so that when an object is removed from the list, all the objects that shift down one index have already been examined. There are other, more proper solutions to this, but iterating backwards is a simple way around it.

I have tried this, but then when i'm printing the list, i have found that the ith 'employee' is not deleted — Arpan Mukherjee, Aug 05 '15 at 16:07

how to iterate an ArrayList of objects and to access a method of that class to which the object belongs while iterating?

3 Answers3