Asenim/Ghennadi-Berezovschi - Simulation.md

## Ghennadi-Berezovschi - Simulation.md

      
    Raw
  

              Ghennadi-Berezovschi - Simulation.md
            
          
    https://github.com/Ghennadi-Berezovschi/Simulation

[Ghennadi-Berezovschi]

Ревью от Алексея @Raketa4000az
Хотелось бы видеть более интеллектуальных существ на карте.
Недостатки реализации:

Существа двигаются явно неоптимальными маршрутами.

Хорошо:

Нейминг.
Есть размножение существ.

Замечания:

Нейминг
+Особых притензий тут нет, но справочные ресурсы оставлю.

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

Комментарии

+На английском, это хорошо.
-В проекте очень много комментариев. Большая их часть констатирует очевидное
// Getter for the number of rows on the map
public int getRows() {
  return rows;
}
Когда в проекте много каментов, это плохо- пользы от них практически нет, они только забивают пространство и мешают читать код.
В идеале, комментариев вообще не должно быть, код должен объяснять сам себя через правильный нейминг и лаконичный код.
Правильное использование комментариев- "Чистый код", гл.4

Используй классы через их интерфейсы

HashMap<Position, Entity> entities = new HashMap<>();

//ПРАВИЛЬНО:
Map<Position, Entity> entities = new HashMap<>();

Никогда не возвращай null

/* class GameMap */
HashMap<Position, Entity> entities = new HashMap<>();

public void removeEntity(Position position) {
  entities.remove(position);  //может вернуть null
}

public Position findEmptyNearestValidPosition(Position position) {
  //...
  System.out.println("Not found cell for reproducing");  // Logs if no valid position is found
  return null; // Returns null if no valid position is available
}
Возврат null повышает риск возникновения NullPointerException в программе
--
Мартин, "Чистый код", гл.7.7-8
Ютуб, Немчинский "Почему нельзя возвращать NULL?"
--

Нарушение DRY, магические буквы, числа, слова. Вводи константы

this.hunger += 15;  // Increases hunger by 15 every time it's called
System.out.println("Hunger increased to: " + this.hunger);

// If hunger reaches 100, decrease health and reset hunger
if (this.hunger >= 100) {
  decreaseHp(15);  // Reduce HP by 15 if hunger maxes out
//...  

//ПРАВИЛЬНО:
private final static int ВСЕ_ОБЪЯСНЯЕТ_А = 15;
private final static int ВСЕ_ОБЪЯСНЯЕТ_Б = 100;

//...
this.hunger += ВСЕ_ОБЪЯСНЯЕТ_А; //теперь не надо каментов, имя константы все само объяснит
System.out.println("Hunger increased to: " + this.hunger);

if (this.hunger >= ВСЕ_ОБЪЯСНЯЕТ_Б) {
  decreaseHp(ВСЕ_ОБЪЯСНЯЕТ_A);  
//...  
--
Фаулер, "Рефакторинг", гл.8 п."Замена магического числа символической константой"
refactoring.guru "Замена магического числа символьной константой"
--

Используй форматированный вывод там, где это напрашивается

System.out.println("Current rabbit's hunger: " + getHunger() + " HP: " + getHp());

//ПРАВИЛЬНО:
System.out.printf("Current rabbit's hunger: %d  HP: %d \n", getHunger(), getHp());

7. class Position, координаты.

-Нарушение инкапсуляции. Всегда явно указывай область видимости и final, если переменная final
public class Position {
  int column;
  int row;
  //...
}

//ПРАВИЛЬНО:
public class Position {
  private int column;
  private int row;
  //...
}
+Хорошо, что координаты называются row и column а не x и y.
В этом случае нет, как часто бывает, путаницы с порядком расположения в массивах: координата(x,y) vs массив[y, x].

class GameMap

-Название должно как можно лучше объяснять суть явления
private int rows; // Number of rows on the map
private int columns; // Number of columns on the map

//ЛУЧШЕ:
private int height; // Number of rows on the map
private int width; // Number of columns on the map
-Нарушение инкапсуляции, класс предоставляет клиентскому коду подробности внутреннего устройства и доступ к нему
HashMap<Position, Entity> entities = new HashMap<>();

//ПРАВИЛЬНО:
private final HashMap<Position, Entity> entities = new HashMap<>();
В данный момент клиентский код может выполнять любые операции с HashMap Карты напрямую, игнорируя допустимые классом способы.
Например так
GameMap gameMap = new GameMap();
/* добавление существ в карту */
gameMap.entities.clear();  //геноцид
Из-за этого класс является гибридом c соответствующими последствиями: "Чистый код", гл.6
-Зависимость модели от представления.
Модель(а это модель) не должна ничего печатать в консоль.
Иначе модель перестает быть универсальной и становится заточенной под конкретную среду и конкретное представление себя в этой среде/или какое-либо сообщение о себе в этой среде- в данном случае в консоли.
В других средах(напр. Андроид) модель нельзя будет использовать
public Position findEmptyNearestValidPosition(Position position) {
   //...
  System.out.println("Not found cell for reproducing");  // модель печает в консоль
  //...
}
Если модель во время выполнения каких-либо действий должна проинформировать об этом систему, то можно использовать паттерн Callback.
-Нарушение SRP, методы чужих ответственностей.
Карта должна только хранить существа и обеспечить базовые операции с ними: вставить, выдать одно существо и список всех хранимых существ, удалить.
Если какой-то метод не нужен для обеспечения этого функционала, значит он не принадлежит к ответственности карты, а принадлежит к чужой ответственности.
Здесь методы чужих ответственностей:
Position findEmptyNearestValidPosition(Position position)
void setupDefaultEntitiesPosition()
-Карта не должна сама себя заселять существами, иначе она становится не универсальной и нельзя будет создать несколько конфигураций игры с разными комбинациями существ на карте.
Объект не должен сам себя инициализировать, в данном случае путем заселения себя существами.
--
Мартин, "ЧК", гл.11,п."Отделение конструирования системы от ее использования".
--

class Entity

-Все потомки переопределяют toString() и хранят там спрайт своего изображения.
toString() должен быть стандартным(как делает IDE по Alt+Ins), использоваться только для отладки.
toString() не должен использоваться для представления
public class Rock extends EnvironmentObject{
  //...
  @Override
  public String toString() {
    return "\uD83E\uDEA8";
  }
}

//ПРАВИЛЬНО:
public class Rock extends EnvironmentObject{
  //...
  @Override
  public String toString() {
    return "Rock{" +
      "position=" + position +
    '}';
  }
}
---
Блох, "Java эффективное программирование" гл.3.3
---
-Нарушение SRP, зависимость модели от представления- класс и все его потомки хранят спрайт с собственным изображением(в toString()).
Модель(а это модель) не должна зависеть от представления и знать, как ее будут показывать юзеру.
Потому что в разных средах(консоль, Swing, Android) одна и та же модель может быть показана разными способами.
Спрайты всех существ должны храниться в классе, который распечатывает карту.
-Содержит координату. Но координата нужна только тому существу, которое ходит. Поэтому entities должен хранить координату начиная только с уровня Creature.


Классы стационарных существ: Tree, Rock, Grass
-Своих грехов не имеют, но получают их по наследству от Entity.


Наследники Creature: Wolf и Rabbit


-Зависимость модели от представления: что-то пишут в консоль. См. GameMap.
-Нарушение DRY, makeMove(GameMap map) в двух классах подозрительно похожие. Общее из них нужно вынести в предка.
-Нарушение DRY, методы Position findPositionGrassOrEmpty(GameMap map) и Position findPositionRabbitOrEmpty(GameMap map) делают одно и то же- ищут на карте существ нужного класса.
-Нарушение SRP, Волк и Заяц в себе реализуют процесс поиска еды на карте.

Сергей Жуков:
Реализацию поиска пути к еде нужно вынести из Волка и Зайца в отдельный класс ПоискаторПути.
Поискатор должен искать путь от точки А до точки, соответствующей условиям поиска, например до точки на которой пасется существо нужного класса.
Путь должен быть представлен в виде последоватиельности координат. Далее Заяц и Волк должны использовать Поискатор при совершении хода.
Выглядеть в простейшем варианте это должно примерно так

класс Заяц{
  private final ПоискаторПути поискаторПути = new ПоискаторПути();
  //...
  public void makeMove(GameMap map) {
    List<Position> путь = поискаторПути.get(map, this.position, Марковка.getClass()); //ищет путь до ближайшей координаты с марковкой
    //проскакать по пути "путь" и съесть марковку
  }
}

class RandomEvents, вставляет траву на карту на случайную координату.

-Не является Entity, поэтому не должен находиться в папке entities.
-Пишет что-то в консоль. Модель(а это все-таки модель) не должна зависеть от представления.

class MapConsoleRenderer

+Цветовые коды сделаны константами, это хорошо.
-Если пришел неизвестный Entity, нужно кидать исключение.
Иначе при развитии программы и добавлении в нее нового существа, можно забыть добавить его идентификацию в Рендерер и тогда существо будет жить на карте, но будет при этом невидимкой
private String getColoredSymbol(String symbol, Entity entity) {
  if (entity instanceof Tree) {
    return GREEN + symbol + RESET;
  } else if (entity instanceof Grass) {
    return GREEN + symbol + RESET;
  } else if ......
  }else {
    return symbol;
  }
}

//ПРАВИЛЬНО:
private String getColoredSymbol(String symbol, Entity entity) {
  if (entity instanceof Tree) {
    return GREEN + symbol + RESET;
  } else if (entity instanceof Grass) {
    return GREEN + symbol + RESET;
  } else if ......
  } 
  throw new IllegalArgumentException(НЕИЗВЕСТНОЕ_СУЩЕСТВО);
}
-Спрайты существ должны храниться именно здесь, а не в самих существах.
-Нарушен порядок размещения методов- публичные методы нужно писать вверху, приватные внизу. Возможно чередование: публичный метод, под ним вспомогательный приватный, и снова публичный с приватным.

class GameController, основная игровая логика.

-Нарушение DI. Не должен сам создавать карту, должен уже созданную карту принимать в конструктор. Иначе нельзя будет создать разные игровые конфигурации с разными картами.
-Магическая буква 's', нужно вынести ее в константу.

class Main, содержит точку входа main.

+Только создание и запуск симуляции(GameController), это хорошо.
-GameController, чтобы быть универсальным в использовании, соблюдать DI и ООП дизайн, должен принимать в конструктор необходимые данные.
Здесь GameController должен принимать в конструктор как минимум Карту, уже созданную и населенную существами. Но об этом ниже.
--
Про особенности использования main ищи соответствующую главу в "Чистом коде"
--
=====
Архитектура.
Отсутствует класс поиска пути- нужно сделать.
Алгоритм поиска пути  должен соответствовать тем, которые указаны в ТЗ.
Один из смыслов задания состоит в том, что бы разобраться с BFS и прокачать этим свои скиллы.
В проекте ты отказался от использования Action'ов.
Ок, в отсутствии InitAction'а могут быть свои плюсы- ООП будет лучше, если создание и инициализацию Карты вынести из GameController в Main.
Еще лучше- вынести в отдельный класс, например в простую фабрику(Simple Factory). Эта фабрика должна по запросу создавать карту и населять ее существами.
Тогда майн будет выглядеть примерно так
public static void main(String[] args) {  
  GameMap gameMap = GameMapFactory.create(ШИРИНА, ВЫСОТА, КОЛ_ЗАЙЦЕВ, КОЛ_ВОЛКОВ /* oth. args*/)

  GameController gameController = new GameController(gameMap);
  gameController.startGame();
}
Именно в эту фабрику из GameMap нужно перенести логику заселения.
Только сделай, что бы существа помещались на случайные координаты, а не на фиксированные.
Симуляция- коварный проект. Основная ООП декомпозиция уже выполнена и описана в ТЗ.
Студент выполняет по ТЗ и радуется как он освоил ООП, хотя ООП он тут проезжает на кабриолете.
Поэтому чтобы лучше понять на примере проекта в чем выгода от ООП, можно немного покреативить:

Создать разные Майны с разными конфигурациями карты.
Сделать несколько разных Рендереров. Например, один, такой как есть. И другой, где все существа будут обозначаться буквами- креатуры большими, остальные маленькими.
Сделать Майн, который будет запускать Game с таким рендерером. Естественно, рендерер придется тоже инжектить в конструктор Game. Это для понимания про (не)зависимость модели от представления.
Сделать еще один Майн, который через диалог с юзером будет собирать конфигурацию приложения как из кубиков и запустит ее: укажите размер карты, укажите количество существ, вид рендерера и т.д.

Вывод: изучить декомпозицию ООП, для начала посмотреть видео Сергея про Шахматы. Больше практиковаться с ООП.