В процессе разработки 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();
}
}
Никогда так не делай)
ОтветитьУдалитьЛучше добавляй бомбы в лист на удаление, а потом в конце цикла удаляй их скопом.
Саш привет! Давно небыло тебя. Я соскучился.
ОтветитьУдалитьДавай похоливарим! :)
Так как ты предложил лучше почему?
"Никогда так не делать" тоже, почему?
У меня код постепенно модифицируется, все же по ТДД. Вот в какой-то момент появилась такая бяка. Написал тест, исправил как рассказал и оп, все зеленое. С точки зрения тестов больше делать ничего не надо. Прформанс мне не пока не стоит как задача. Тест деленый, пошел дальше.
А по твоему надо добавлять еще один лист в поля board, инитить его, потом удалять все, что в нем есть из исходного листа с бомбами, сам его читить...
Ты еще не видел, что я дальше сделал :) У меня сегодня день бредокодогенерации... Думаю тебе понравится... В следующем посте.
Прием.
Твой ход :)
Ну я из мира .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 соответственно условный.
Саш, спасибо за такой развернутый ответ. Стоит нам как-то поработать в паре. Есть чему поучиться у тебя.
ОтветитьУдалитьПо замечаниям твоим.
"- нарушил один из базовых принципов ООП"
"нарушил Single Responsibility Principle, т.е. у тебя бомба почему-то сама себя удаляет"
инкапсуляцию выставив наружу bombs.
Вся основная работа с bombs производится в Board, потому и бомба себя сама не удаляет - board подписывается на взрыв бомбы и удаляет бомбу.
"нарушил Interface Segregation Principle с Iterable"
Согласен, добавил в туду, посмотрим, как это поможет в будущем.
"Или в тесте ты мог проверить, что при взрыве на поле остается N-1 бомба, но это неправильно, т.к. тут будет раскрытие реализации."
Тут подробнее можешь рассказать?
>Саш, спасибо за такой развернутый ответ.
ОтветитьУдалитьДа он как-то сам получился, я не специально
>Стоит нам как-то поработать в паре.
Можно попробовать
>Вся основная работа с bombs производится в Board, потому и бомба себя сама не удаляет - board подписывается на взрыв бомбы и удаляет бомбу.
Ну удаляет бомбу анонимный класс потомок Boom (я не знаю как такое называется правильно), который получил доступ к полю bombs, объекта Board. Т.е. на мой взгляд тут есть нарушение инкапсуляции, но с этим можно поспорить.
>"Или в тесте ты мог проверить, что при взрыве на поле остается N-1 бомба, но это неправильно, т.к. тут будет раскрытие реализации."
>Тут подробнее можешь рассказать?
Я имел ввиду, что тест будет знать слишком много интимных подробностей тестируемой системы, и при смене реализации может сломаться.
Спасибо Саша, прояснилось.
ОтветитьУдалитьКоргда в Киеве будешь в очередной раз?