Asenim/Konfeton - Simulation.md

## Konfeton - Simulation.md

      
    Raw
  

              Konfeton - Simulation.md
            
          
    https://github.com/Konfeton/Simulation

Ревью от Алексея @Raketa4000az
[Dmitry Dulko]
Хорошая ООП архитектура.
Хорошо:

Существа не хранят в себе координату.
Существа не хранят в себе спрайт собственного изображения.
Простой понятный алгоритм.

Замечания:

Нейминг

-Название должно как можно лучше объяснять суть явления
/* class WorldMap */
private final int maxWorldX;
private final int maxWorldY;

//ЛУЧШЕ:
private final int height;
private final int width;

-Неочевидно, но
//ВМЕСТО
Class type

//ПРИНЯТО
Class clazz

//ПОТОМУ ЧТО НЕЛЬЗЯ
Class class

-MapConfigurator тут создает карту. Между тем, конфигурирование является действием, в котором вы изменяете настройки по умолчанию в различных частях продукта.
Т.е. конфигуратор должен проводить действие над уже созданным объектом. Если конфигуратор создает объект, это не конфигуратор, а создатель.

--
Oracle Java code conventions, part."Naming conventions"
Мартин, "Чистый код", гл.2
--

Если в блоке if есть return(break, continue, exit), то else не пишется

if (currentNumber < numberToSpawn){
  return numberToSpawn - currentNumber;
}else {
  return 0;
}

//ПРАВИЛЬНО:
if (currentNumber < numberToSpawn){
  return numberToSpawn - currentNumber;
}
return 0;

Не используй try-catch вместе с бизнес логикой, изолируй во вспомогательных методах

while (true) {
  try { 
    int maxSize = Integer.parseInt(input);
    //...
    return maxSize;    
  } catch (NumberFormatException e) {
     System.out.println("Введите число");
  }
}

//ПРАВИЛЬНО:
while (true) {
  if(isInteger(input)) {
    int maxSize = Integer.parseInt(input);
    //...
    return maxSize;    
  } else {
    System.out.println("Введите число");    
  }
}

private static boolean isInteger(String s) {
  try {
     Integer.parseInt(s);
     return true;
  } catch (NumberFormatException e) {
     return false;
  }
}


class Coordinates
+Все отлично.


class WorldMap, карта


-Метод возращает Map. Зачем? Довольно неочевидный вариант возврата результата
public Map<Coordinates, T> getEntitiesOfType(Class<? extends Entity> type)
Этот метод используется для получения Set координат
Set coordinates = map.getEntitiesOfType(Creature.class).keySet();
А дальше этот сет используется для получения существа по координате
Creature creature = (Creature) map.getEntityByCoordinates(coordinate);
Спрашивается, почему сразу не сделать метод, который будет возвращать список всех существ заданного типа
public List getEntities(Class<? extends Entity> clazz)
-Нарушение SRP, методы чужих ответственностей.
Карта должна только хранить существа и обеспечить базовые операции с ними- вставить, выдать одно существо и список всех хранимых существ, удалить.
Если какой-то метод не нужен для обеспечения этого функционала, значит он не принадлежит к ответственности карты, а принадлежит к чужой ответственности.
Метод подсчета площади(диагонали, периметра, стороны вписанного треугольника) не нужен для реализации единой ответственности карты
public int getMapSize(){ return maxWorldX * maxWorldY; }
Этот метод используется в одном классе- SpawnRates. Вот там он и должен находиться.
-Метод совершения хода- переносит существо из точки А в точку Б. А имеет ли карта право осуществлять перемещение существ? Не нарушит ли такое перемещение правил игры?
public void makeMove(Coordinates from, Coordinates to)
Что будет, если в точке "to"  будет находиться какое-то существо, например гора? Что будет, если дадим команду перенести зайца из одного края карты в другой, без учета пешеходных возможностей зайца?
Карта никак не отвечает на эти вопросы. Очевидно, что эти вопросы выходят за рамки компетенции карты. А значит, карта не должна содержать в себе метод перемещения существа внутри себя.
+В целом, в архитектуре Карты нет грубых нарушений.

class MapConfigurator, ведет диалог с юзером и и по его результатам создает Карту

-Не является Картой, либо ее составной частью, как Координата. Поэтому не может находиться вместе с ними в папке map.

abstract class HerbivoreFood и его наследник Carrot
-Наследники должны инжектить int healingPower HerbivoreFood'у в конструктор, а не сетить его

public class Carrot extends HerbivoreFood {
  public Carrot() { healingPower = 2; }
}

//ПРАВИЛЬНО:
public class Carrot extends HerbivoreFood {
  private final static int HEALING_POWER = 2;

  public Carrot() { 
    super(HEALING_POWER);
  }
}
?. class Creature extends Entity и его наследники Заяц и Волк
+Все хорошо, за исключением мелкого нюанса при использовании поиска, о чем ниже.

interface PathFinder, интерфейс поиска пути

+Интерфейс поиска- это хорошо. Можно сделать разные реализации интерфейса и т.о. ввести в программу разные алгоритмы поиска пути.
-Не понимаю, как работает поиск- он только принимает координату старта
public interface PathFinder {
  Path findPath(Coordinates start);
}
По идее, метод поиска пути должен принимать карту, координату старта и условия идентификации координаты конца, напр. класс существа, которое находится на этой координате.

class BreadthFirstSearch implements PathFinder, алгоритм поска BFS
-Теперь понятно- BreadthFirstSearch принимает карту и класс еды в конструктор
public BreadthFirstSearch(Class<? extends Entity> target, WorldMap map)
Если рассматривать ситуацию как сферического коня в вакууме, такая реализация хуже, чем если бы Поиск имел пустой конструктор, а необходимые для поиска аргументы принимал в метод.
Тогда один и тот же экземпляр можно было бы задействовать для поиска пути всеми существами, а не персонально конкретным, как сейчас

//поиск принимает аргументы в конструктор
Морковка морковка = new Морковка();
Заяц заяц = new Заяц();
Волк волк = new Волк();
...
Поиск ищетЗаяц = new Поиск(Морковка.getClass(), map);  //one
Поиск ищетВолк = new Поиск(Заяц.getClass(), map);  //two

Путь путьВолка = ищетВолк.get(координатаВолка);
Путь путьЗайца = ищетЗаяц.get(координатаЗайца);
...

//VS поиск принимает аргументы в метод
Морковка морковка = new Морковка();
Заяц заяц = new Заяц();
Волк волк = new Волк();
...
Поиск универсальныйПоиск = new Поиск(); //one

Путь путьЗайца = универсальныйПоиск.get(map, координатаЗайца, Морковка.getClass());
Путь путьВолка = универсальныйПоиск.get(map, координатаВолка, Заяц.getClass());
-Экземпляр поиска создается каждый раз при вызове метода makeMove() в креатурах
public abstract class Creature extends Entity {
  //...
  public void makeMove(Coordinates from, WorldMap map) {
    PathFinder pathFinder = new BreadthFirstSearch(...);
    Path path = pathFinder.findPath(...);
    //...
  }
}

//ПРАВИЛЬНО:
public abstract class Creature extends Entity {
  private final PathFinder pathFinder = new BreadthFirstSearch(...);
  //...
  public void makeMove(Coordinates from, WorldMap map) {
    Path path = pathFinder.findPath(...);
    //...
  }
}

Action'ы

+MakeMoveAction- хорошо.
+EntitySpawnAction, помещает существ на карту. Универсальный класс(т.е. селит на карту существ любого класса), для этого принимает в аргументы Supplier, который описывает создание нужных существ- хорошо.


interface Renderer
+Интерфейс для распечатки это хорошо- можно делать разные рендереры.


class ConsoleRenderer implements Renderer
+Хранит в себе спрайты существ, хорошо.
+Спрайты сделыны константами, хорошо.
+В дефолте свич-кейса бросается исключение, если пришло неизвестное существо, хорошо.


Simulation и SimulationController
+Все ок


=====
Вывод: получилось хорошо. Не зря столько времени продумывал архитектуру. Замечания есть, но несущественные.
#ревью #симуляция