1. Java / Говнокод #4792

    +76

    1. 1
    2. 2
    3. 3
    4. 4
    5. 5
    for (int j = 0; j < fieldsToRemove.size(); j++) {
	if (fieldsToDelete.getField(j).getKind().equals("GroupField")) {
		resFieldContr.remove(j--);
	}
}

    собственно цикл.
    нашел в рабочем проекте

    Запостил: tas, 30 Ноября 2010

    Комментарии (21) RSS

    • ava gegMOPO4 30.11.2010 19:14 # 0

      Может и работает, но в одной конструкции обращаться к одному списку тремя разными способами -- это нужно уметь.
      Ответить

    • ava tas 30.11.2010 19:28 # 0

      да тут уабще не нужна J
      Ответить

    • ava absolut 30.11.2010 21:56 # 0

      модификация переменной цикла в разных местах не есть гуд.
      Ответить

      • ava gegMOPO4 01.12.2010 00:57 # 0

        Это нормальная практика, когда в цикле по условию удаляют элементы.
        for (int i = 0; i < somelist.size(); i++)
    if(todelete(somelist.get(i)))
        somelist.remove(i--);

        Можно
        for (int i = 0; i < somelist.size();)
    if(todelete(somelist.get(i)))
        somelist.remove(i);
    else
        i++;

        но это длиннее и непривычно.
        Ну и для длинных списков и большого количества удалений имеет смысл избавиться от O(N^2):
        int j = 0;
for (int i = 0; i < somelist.size(); i++)
    if(!todelete(somelist.get(i)))
        somelist.set(j++, somelist.get(i));
// дальше усекаем до размера j
        Ответить

        • ava bugmenot 01.12.2010 02:02 # +2

          --i++ и что в этом нормального
          с хвоста перебрать элементы религия не позволяет?
          Ответить

        • ava absolut 01.12.2010 08:05 # 0

          Второй вариант куда лучше, а длиннее всего на пару символов.
          К тому же, нормальная практика получать в цикле ссылку (указатель) на следующий элемент для обработки,
          не зависимо от того, удаляется текущий или нет.
          Я считаю, что изменение переменной цикла должно быть локализовано, так проще понять его назначение и совершить меньше ошибок.
          Ответить

          • ava gegMOPO4 01.12.2010 11:11 # 0

            Длиннее на две строки. В общем, скорее дело вкуса.
            Ответить

            • ava absolut 01.12.2010 11:30 # 0

              Напишите всё в одну строку - будет короче на четыре :)
              Ответить

    • ava alt 01.12.2010 11:39 # +2

      А итератор никто не придумал?
      for (Iterator<BlaBlaBla> iterator = fieldsToRemove.iterator(); iterator.hasNext();) {
	if (iterator.next().getKind().equals("GroupField")) {
		iterator.remove();
	}
}
      Ответить

      • ava Анонимус 01.12.2010 13:47 # 0

        ВНЕЗАПНО, concurrent modification exception
        Ответить

        • ava ArtPetroff 06.12.2010 02:18 # 0

          Учим матчасть и идём запускать код руками.
          Итератор на то и имеет метод remove, чтобы можно было текущий элемент удалить.
          Ответить

          • ava Анонимус 06.12.2010 13:23 # 0

            Removes from the underlying collection the last element returned by the iterator (optional operation).
            Ответить

            • ava ArtPetroff 06.12.2010 13:47 # 0

              Назовите реализацию List'а, которая поддерживает метод remove, но не поддерживает remove у Iterator'a по этому List'у.
              Ответить

              • ava Анонимус 06.12.2010 14:27 # 0

                LinkedList
                Ответить

                • ava ArtPetroff 06.12.2010 14:51 # +1

                  LinkedList использует в качестве итератора внутренний класс ListItr, в котором метод remove вполне себе определён и ConcurrentModificationException кинет только если этот список поменял в процессе итерации кто-либо другой.

                  И вообще, предлагаю запустить код.

                  public void testArrayListIterator() {
        final List<String> list = new ArrayList<String>();
        list.add("1");
        list.add("2");
        list.add("1");
        for (Iterator<String> iterator = list.iterator(); iterator.hasNext();) {
            if ("1".equals(iterator.next())) {
                iterator.remove();
            }
        }
        assertEquals(1, list.size());
    }

    public void testLinkedListIterator() {
        final List<String> list = new LinkedList<String>();
        list.add("1");
        list.add("2");
        list.add("1");
        for (Iterator<String> iterator = list.iterator(); iterator.hasNext();) {
            if ("1".equals(iterator.next())) {
                iterator.remove();
            }
        }
        assertEquals(1, list.size());
    }
                  Ответить

                  • ava Анонимус 06.12.2010 14:57 # +2

                    да, Вы оказались правы.
                    Удалять из итератора можно. Нельзя удалять напрямую из коллекции, по которой создан итератор.

                    я пальцем в лужу, бывает)
                    Ответить

    Добавить комментарий