Если нельзя, но очень хочется, то нужно обязательно и ничего в мире не стоит того, чтобы делать из этого проблему!


Интересна Java? Кликай по ссылке и изучай!
Если тебе полезно что-то из того, чем я делюсь в своем блоге - можешь поделиться своими деньгами со мной.
с пожеланием
столько времени читатели провели на блоге - 
сейчас онлайн - 

пятница, 8 марта 2013 г.

Бомбермен: Любопытная бага с лиснерами

В процессе разработки Bombrman для Coding Dojo я сталкивался с рядом любопытных моментов. Например.
Каждый такт игры я итерируюсь по всем бомбам чтобы уведомить их таймеры ...
    private void tactAllBombs() {
        for (Bomb bomb : bombs) {
            bomb.tick();
        }
    }
...а в bomb.tick() я отсчитываю такты самой бомбы - когда-то же она должна взорваться...
    public void tick() {
        timer--;
        if (timer == 0) {
            boom();
        }
    }
...и если таймер истек, то бомба бабахает!
    private void boom() {
        if (affect != null) {
            affect.boom(this);
        }
    }
affect - это слушатель, подписанный на "бабах" бомбы. В данном конкретном случае у меня это анонимный класс на борде. То есть борда прописывается на взрыв бомбы и делает беспорядок среди попавших под раздачу. Подписывается она в момент когда кто-то бомбермен дропает бомбу.
    public void drop(Bomb bomb) {
            bomb.setAffect(new Boom() {
                @Override
                public void boom(Bomb bomb) {
                    bombs.remove(bomb);
                    killAllNear();
                }
            });
            bombs.add(bomb);
        }
    }
И вот тут самое интересное - я в этом лиснере уничтожаю бобму из списка оставленных на карте. Но тут конфликт. На самом верхнем уровне я итерируюсь по бомбам, а потом в самой глубине я удаляю из итерируемого списка - это приводит к тому, что последняя бомба не тикает, если перед ней взорвалась предыдущая.
Лечится так.
    private void tactAllBombs() {
        for (Bomb bomb : bombs.toArray(new Bomb[0])) {
            bomb.tick();
        }
    }

6 комментариев:

  1. Никогда так не делай)

    Лучше добавляй бомбы в лист на удаление, а потом в конце цикла удаляй их скопом.

    ОтветитьУдалить
  2. Саш привет! Давно небыло тебя. Я соскучился.
    Давай похоливарим! :)
    Так как ты предложил лучше почему?
    "Никогда так не делать" тоже, почему?

    У меня код постепенно модифицируется, все же по ТДД. Вот в какой-то момент появилась такая бяка. Написал тест, исправил как рассказал и оп, все зеленое. С точки зрения тестов больше делать ничего не надо. Прформанс мне не пока не стоит как задача. Тест деленый, пошел дальше.

    А по твоему надо добавлять еще один лист в поля board, инитить его, потом удалять все, что в нем есть из исходного листа с бомбами, сам его читить...

    Ты еще не видел, что я дальше сделал :) У меня сегодня день бредокодогенерации... Думаю тебе понравится... В следующем посте.

    Прием.
    Твой ход :)

    ОтветитьУдалить
  3. Ну я из мира .NET, и там при операции foreach такое невозможно, т.к. будет в рантайме исключение - "исходная коллекция изменилась", т.е. багу ты бы заметил при первом же взрывы, а не в тот момент, когда стали не взрываться бомбы.

    Если хочется и итерировать коллекцию, и изменять ее одновременно то для этого есть 3 способа:

    1. как ты написал, через создание другой коллекции.
    2. добавлять элементы в другую коллекцию и потом пройтись по ней.
    3. если порядок итерирования не важен, то нужно итерировать с конца в начало, тогда такой баги не будет (через цикл for).

    Казалось бы это должны все знать, но как выяснилось недавно - не все. Причем там это никто не заметил

    Но это все лечение пиявками, т.е. борьба с симптомами.

    Проблема же тут в том, что ты:
    - нарушил один из базовых принципов ООП - инкапсуляцию выставив наружу bombs.
    - нарушил Interface Segregation Principle, т.е. в данном случае нужно было выставлять наружу IEnumerable (.NET) или Iterable (Java).
    - нарушил Single Responsibility Principle, т.е. у тебя бомба почему-то сама себя удаляет.

    Ну и TDD тут не причем. Ну и я не видел твой набор тестов. Как мне кажется у тебя Affect остался не протестированным.

    Или в тесте ты мог проверить, что при взрыве на поле остается N-1 бомба, но это неправильно, т.к. тут будет раскрытие реализации.

    ИМХО, самым оптимальным было бы возвращать из tick бомбы TickResult и его проверять. Т.е. твой код стал бы примерно таким:

    private void tactAllBombs() {
    List exploeded = new ArrayList();
    for (Bomb bomb : bombs) {
    TickResult result = bomb.tick();
    if (result == TickResult.Bang) {
    exploeded.add(bomb);
    }
    }
    for (Bomb bomb : exploeded) {
    bombs.remove(bomb);
    }
    }

    Т.е. никакого _поля_ тут не будет. Считай, что я разработал этот код с помощью TDD, вот пример "тестов":

    //Arrange
    Board board = new Board();

    // StubExploedBomb возвращает на tick TickResult.Bang
    Bomb bomb = new StubExploedBomb();
    // Я не знаю какой у тебя интерфейс постановки бомб на поле, поэтому предположу что он будет
    // принимать бомбу и координаты.
    board.placeBomb(bomb, x, y);

    //Act
    board.tackAllBombs();

    //Assert
    Assert.False(board.Has(bomb));

    Тесты на взрыв

    @Test
    public void testBombDoesNotExplodeIfTickedLessThanBombLifetime() {
    //Arrange
    const int TICKS_COUNT = 2;
    Bomb bomb = new Bomb(TICKS_COUNT);

    //Act
    TickResult reslt = bomb.tick();

    //Assert
    Assert.notEqual(result, TickResult.Bang);
    }

    Дальше я реализую tick: public TickResult tick () { return TickResult.None; }

    При написании следующего теста Arrange из предыдущего вынесу в fixture setup и напишу такой тест:

    @Test
    public void testBombExplodesIfTickedExactlyBombLifetime() {
    //Arrange in fixture setup

    //Act
    for (int i=0; i<TICKS_COUNT; i++) {
    testBombDoesNotExplodeIfTickedLessThanBombLifetime(); //здесь я позволю себе вызывать другой тест.
    }
    TickResult result = bomb.tick();

    //Assert
    Assert.equal(result, TickResult.Bang);
    }

    Делаю тест зеленым:

    public TickResult tick () {
    tick--;
    if (tick == 0)
    return TickResult.Bang;
    return TickResult.None;
    }

    PS: весь код писал прямо в окне комментария, поэтому могут быть ошибки, TDD соответственно условный.

    ОтветитьУдалить
  4. Саш, спасибо за такой развернутый ответ. Стоит нам как-то поработать в паре. Есть чему поучиться у тебя.

    По замечаниям твоим.

    "- нарушил один из базовых принципов ООП"
    "нарушил Single Responsibility Principle, т.е. у тебя бомба почему-то сама себя удаляет"

    инкапсуляцию выставив наружу bombs.
    Вся основная работа с bombs производится в Board, потому и бомба себя сама не удаляет - board подписывается на взрыв бомбы и удаляет бомбу.

    "нарушил Interface Segregation Principle с Iterable"

    Согласен, добавил в туду, посмотрим, как это поможет в будущем.

    "Или в тесте ты мог проверить, что при взрыве на поле остается N-1 бомба, но это неправильно, т.к. тут будет раскрытие реализации."

    Тут подробнее можешь рассказать?

    ОтветитьУдалить
  5. >Саш, спасибо за такой развернутый ответ.

    Да он как-то сам получился, я не специально

    >Стоит нам как-то поработать в паре.

    Можно попробовать

    >Вся основная работа с bombs производится в Board, потому и бомба себя сама не удаляет - board подписывается на взрыв бомбы и удаляет бомбу.

    Ну удаляет бомбу анонимный класс потомок Boom (я не знаю как такое называется правильно), который получил доступ к полю bombs, объекта Board. Т.е. на мой взгляд тут есть нарушение инкапсуляции, но с этим можно поспорить.

    >"Или в тесте ты мог проверить, что при взрыве на поле остается N-1 бомба, но это неправильно, т.к. тут будет раскрытие реализации."
    >Тут подробнее можешь рассказать?

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

    ОтветитьУдалить
  6. Спасибо Саша, прояснилось.
    Коргда в Киеве будешь в очередной раз?

    ОтветитьУдалить