Faust32 - Simulation.md

https://github.com/Faust32/Simulation
Ревью от Алексея

Запутанная архитектура.

Недостатки реализации:

1. Субъективно- выглядит, как движок, на который еще не натянули графику. Текстовый интерфейс, где существа это слова, а под ним список существ с их характеристиками. Но при использовании приемов ООП этот недуг можно в подвиг обратить.

Хорошо:

1. Работает

Замечания:

1. Нейминг:

-Не дублируй имя класса в названии метода класса

EntityMap.getEntityFromMap(Coordinates coordinates) // итак понятно, что из карты
         .removeFromMap(Coordinates coordinates)    // итак понятно, что из карты

//ПРАВИЛЬНО:
EntityMap.get(Coordinates coordinates)
         .remove(Coordinates coordinates)

-Метод обещает вернуть Сет, но возвращает не Сет. К тому же, что может говорить пользователю метод с названием entrySet()? Ничего. Что бы разобраться, зачем нужен этот метод, нужно лезть в Карту и смотреть, как она устроена public Iterable<? extends java.util.Map.Entry<Coordinates, Entity>> entrySet()

-Название метода должно быть глаголом

void herbivoreStatusPrinter(EntityMap entityMap)

//ПРАВИЛЬНО:
void herbivoreStatusPrint(EntityMap entityMap)

2. Небрежная манера расположения кода, переменные иногда обьявлены ниже методов

3. Всегда явно указывай область видимости и final, если переменная final:

SearchAlgorithm searchAlgorithm = new SearchAlgorithm();
CreaturesStatusRender statusRender = new CreaturesStatusRender();

//ПРАВИЛЬНО:
private final SearchAlgorithm searchAlgorithm = new SearchAlgorithm();
private final CreaturesStatusRender statusRender = new CreaturesStatusRender();

4. class Main

Здесь в main'e должна создаваться Карта и передаваться в Симуляцию в качестве аргумента. Карта не должна создаваться в Симуляции, как это сделано сейчас. Также здесь должен происходить начальный диалог "запустить-не запустить". Симуляция должна создаваться только тогда, когда юзер выбирает "запустить".

5. class Coordinates

Модель(а это модель) не должна быть завязана на представление ни прямо, ни косвенно, как в этом случае. В данном случаеметод getStringCoordinates() имеет отношение к тому, в каком виде он будет показываться юзеру, т.е. завязан на представление

public String getStringCoordinates(){
  return "{" + getX() + ", " + getY() + "}";
}

У тебя уже есть class CreaturesStatusRender, который распечатывает статусы сущностей, вот пусть там и формируется текстовое представление координат.

6. class EntityMap

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

public Entity getEntityFromMap(Coordinates coordinates) {
  return map.get(coordinates);  //может вернуть null
}

7. class MapDimension не используется. Не забывай чистить мусор, не оставляй лодочных якорей

8. record EntityName(String entityName) не имеет смысла, кроме того что бы делать программу более запутанной

9. class Entity

-Модель(а это модель) не должна быть завязана на представление и не должна знать, как ее будут показывать юзеру. Простой пример- захочешь ты в процессе работы приложения поменять язык интерфейса и вместо "TREE" нужно будет показывать "АГАЧ". Тогда придется писать костыльный метод, который будет ходить по всем моделям и менять их имена на другие. public EntityName entityName Понятно, что здесь такого не будет, но нужно понимать, к чему потенциально ведет то или иное решение.

Еще это потенциальная причина багов, потому что может получиться так Entity entity1 = new Tree(new Coordinates(1,1), new EntityName("COW")); Entity entity2 = new Tree(new Coordinates(2,2), new EntityName("WOLF")); Рекомендация та же- засунь эту ответственность в рендерер

-В Entity хранятся координаты. Но координаты нужны сущности, только что бы совершать ходы. Ходят сущности начиная с Млекопеитающее, вот там и должны храниться координаты.

10. Классы Herbivore и Predator

-Нарушение DRY, makeMove(...) у двух классов практически идентичен

-Кроме того, этот метод очень длинный, а значит нарушает еще SRP, о чем свидетельствуют многочисленные комментарии, призванные визуально отделить ответственности

11. class Action, нарушение SRP, занимается несколькими разными вещами

12. class HungerManager extends Action, зачем он наследуется от Action? Это не имеет никакого смысла

13. Группа классов SpawnRock, SpawnTree и т.д. - по факту, делают одно и тоже, населяют карту существами. Сплошное дублирование кода.

14. class Renderer, нарушение SRP, божественный обьект, содержит несколько ответственностей:

-распечатка карты с существами -распечатка меню -ввод от юзера пункта меню

Ты же помнишь, что у тебя класс называется Renderer а не RendererAndInputer?

15. class CreaturesStatusRender extends Renderer, зачем он расширает Render?

Нарушение DRY, herbivoreStatusPrinter(...) и predatorStatusPrinter(...) почти одинаковы.

=== В текущем виде твои Рендеры c точки зрения ООП никуда не годятся. Каждый рендер должен печатать что-то свое. Сначала введи интерфейс Render:

public interface Renderer<T> {
    void render(T value);
}

Потом сделай реализации рендеров для распечатки поля и статуса существ

Renderer<EntityMap> entityMapRenderer = new РендерерКарты();
entityMapRenderer.render(someEntityMap);

Renderer<Creature> creatureRenderer = new РендерерКреатуры();
creatureRenderer.render(someEntity);

И вот тогда можно будет развернуться во всю ширь и использовать всю мощь ООП. Можно сделать разные Майны, в каждом из которых ты будешь собирать и запускать разные конфигурации своего приложения:

-приложение, где интерфейс карты текстовый, как сейчас(new ТекстовыйРендерерКарты()) -приложение, где интерфейс карты c эмоджами (new ЭмоджевыйРендерерКарты()) -приложение, где статус зверей будет цветным (new ЦветнойРендерерКреатуры()) -приложение, где статус зверей будет на китайском (new КитайскийРендерерКреатуры())

и т.д.

Вот на этом можно будет хорошо потренировать понимание ООП.

Вывод: -разобраться, зачем нужно наследование -DRY, SRP, SOLID в целом -декомпозиция