/ Java

Java Smells #0

Иногда в проектах можно встретить много странного кода и найти много багов. Некоторые из интересных случаем я начал собирать.

Что не так в этом for-each?

Очень простая штука, но такой код я находил в репозитории, совершенно очевидно, что он не работает, но он будет падать с интересной ошибкой. В данном случае getMetadata вернёт ArrayList.

for (KeyValuePair keyValuePair : rootNode.getMetadata()) {
    if (StringUtils.startsWith(keyValuePair.getKey(), id)) {
       rootNode.getMetadata().remove(keyValuePair);
    }
}

Прежде всего нужно помнить правило Не изменяйте коллекцию во время итерации по ней. Вполне можно изменять объекты, но нельзя изменять количество элементов. В данном примере как раз производится удаление.

Вот код удаления, как вы видите, происходит вызов it.remove();, это метод итератора, поэтому нам надо взглянуть на его реализацию.

 public boolean remove(Object o) {
        Iterator<E> it = iterator();
        if (o==null) {
            while (it.hasNext()) {
                if (it.next()==null) {
                    it.remove();
                    return true;
                }
            }
        } else {
            while (it.hasNext()) {
                if (o.equals(it.next())) {
                    it.remove();
                    return true;
                }
            }
        }
        return false;
    }

А вот реализация итератора в ArrayList, обратить стоит внимание, на то, что перед любым действием выполняется проверка на изменение количества элементов. И если оно произошло, будет выброшена ConcurrentModificationException, хотя в данном случае и нет потоков.

        public E next() {
            checkForComodification();
            int i = cursor;
            if (i >= size)
                throw new NoSuchElementException();
            Object[] elementData = ArrayList.this.elementData;
            if (i >= elementData.length)
                throw new ConcurrentModificationException();
            cursor = i + 1;
            return (E) elementData[lastRet = i];
        }

        public void remove() {
            if (lastRet < 0)
                throw new IllegalStateException();
            checkForComodification();

            try {
                ArrayList.this.remove(lastRet);
                cursor = lastRet;
                lastRet = -1;
                expectedModCount = modCount;
            } catch (IndexOutOfBoundsException ex) {
                throw new ConcurrentModificationException();
            }
        }

Надеюсь теперь полностью понятно, почему не надо вызывать remove в for-each, тем не менее, я встретил такой код в репозитории одного проекта.

Заметка про удаление

Ещё одна интересная штука, само удаление тоже не совсем корректно, на самом деле в данном примере в KeyValuePair не определён equals и hashCode, а значит при попытке удалить из коллекции объект, ничего не произойдёт, т.к. equals будет сравнивать ссылки. *Но в данном примере, всё-таки, удаление произойдёт, т.к. мы итерируемся по объектам и переменная keyValuePair получает ссылки, то в данном случае сравнение по equals даст true.

Java Smells #0
Share this

Subscribe to Yet another blog