0
\$\begingroup\$

I have implemented a trim method which removes objects having empty properties.

This list has average size of 3 or so. Probably, the max length is 7 or 8.

I wonder which of these implementations is faster:

Implementation 1

    private void trim() {
       Iterator<Child> iterator = mList.iterator();
       while (iterator.hasNext()) {
           Child child = iterator.next();
           if (child.isEmpty()) {
               iterator.remove();
           }
       }
   }

Implementation 2

    private void trim() {
       for (Child child : mList) {
           if (child.isEmpty()) {
               mList.remove(child);
           }
       }
   }

Implementation 3

   private void trim() {
      for (int i = 0; i < mList.size(); i++) {
        IChild child = mList.get(i);
        if (child.isEmpty()){
            mList.remove(i);
        }
      }
   }
\$\endgroup\$
6
  • 1
    \$\begingroup\$ I think second implementation gonna throw ConcurrentModificationException. \$\endgroup\$ Commented Jun 19, 2017 at 16:39
  • \$\begingroup\$ The first two implementations invalidate the iterator whilst it's in use; the third likely skips elements following those that are deleted. So I don't think any of them are even correct. Are you sure this is ready for review yet? \$\endgroup\$ Commented Jun 19, 2017 at 16:47
  • \$\begingroup\$ I'm sorry but I have to close this question because it's clear that your code is untested. The only option that works properly is the first one. Also, you're question is essentially the same as this one: codereview.stackexchange.com/q/64011/31562 \$\endgroup\$ Commented Jun 19, 2017 at 16:55
  • \$\begingroup\$ Also, if you're interested in measuring speed, you should look into benchmarking. In fact, one of the users around here wrote a very good Benchmarking library for Java 8 \$\endgroup\$ Commented Jun 19, 2017 at 16:57
  • \$\begingroup\$ It was a prevision. You do your job, its ok :) \$\endgroup\$ Commented Jun 19, 2017 at 16:57

1 Answer 1

1
\$\begingroup\$

Implementation 2 will in fact throw ConcurrentModificationException. What comes to Implementation 3, consider you have a list \$1,2,3,4\$, and the third implementation is about to remove \$2\$ and \$3\$. First, the index is 1 and \$2\$ is removed. Now \$3,4\$ is shifted one position to the left so that the entire list is \$1,3,4\$. At this point, the index is 1 and is incremented to 2. Clearly, \$3\$ is omitted and is not removed.

Basically, if removal is done in iteration, your only choice is to use Iterator explicitly.

Hope that helps.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for review. I do not want to take your time but if you interested, what would be the best practice or any suggestions? \$\endgroup\$ Commented Jun 19, 2017 at 16:55
  • 1
    \$\begingroup\$ @EmreAktürk It really depends. Actually, you can get 3rd to work if you iterate backwards from larger indices towards smaller indices. \$\endgroup\$
    – coderodde
    Commented Jun 19, 2017 at 17:02

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