Start task 3

[As-pasa]

No description provided.

[alklepin]

А вот если бы я попросил описать в паре предложений, за что отвечает этот класс, каким был бы ответ?

[alklepin]

Отдельно отмечу, что от ответа на вопрос выше зависит, правильно ли реализован этот класс.

[As-pasa]

Controller разбит на 2 класса: 1-непосредственно управляет положением цели, 2- фоново обновляет модель согласно таймеру

[alklepin]

А зачем этот таймер?
Кажется, что в событийной модели можно было бы реагировать на изменение модели робота, и только в этом случае инициировать перерисовку

[As-pasa]

Таймер в окне отрисовки действительно оказался лишним, теперь все завязано исключительно на таймере обновления модели, а перерисовка происходит как часть реакции на обновление модели.

[alklepin]

Я так понял, что ответственность этого класса - нарисовать робота. Правильно?
Но тогда зачем в этом классе хранить состояние? Сейчас, получается, мы размазываем ответственность за хранение положения робота (оно хранится и здесь, и в RobotModel), что создает неопределенность: какое из положений верное?
Предлагаю попробовать не хранить состояние робота в этом классе и сообразить, как перестроить код в этом случае. (если непонятно, что имею ввиду, или не получится придумать решение - спишемся или созвонимся, и я объясню, что имею ввиду).

[As-pasa]

Теперь DefaultRobot агрегирует в себе RobotModel, тем самым избавившись от внутреннего состояния и необходимости в обновлении внешним классом. Однако так мы полностью связываем представление и модель и в случае если мы захотим во время исполнения программы заменить модель(предположим чтобы робот начал бежать от точки, а не к ней), то придется обновлять не только ссылки в главной программе и окне, но и в каждом из DefaultRobot. В то же время в случае сохранения внутреннего состояния и метода update потребуется только подменить ссылку в главной функции программы и в окне отрисовки.

P.S. Подумал, что при такой механике проще будет воспользоваться паттерном состояние и заменить в уже существующем объекте модели алгоритм, чем заменять ссылки в каждом объекте, агрегирующем модель, так что действительно вариант без внутреннего состояния лучше.

[alklepin]

Лучше было бы в этом классе вообще ничего не хранить. А передавать ту модель робота, которую хотим нарисовать, непосредственно в метод draw. Я подразумевал именно эту идею. Тогда и проблемы с заменой модели на лету, если потребуется, не возникнет. И у класса будет единственная ответственность: нарисовать на заданном контексте данные из заданной модели робота.
При этом сама модель могла бы реализовывать интерфейс, позволяющий получить координаты и направление. Тогда код рисования смог бы нарисовать любую реализацию такого интерфейса.

[alklepin]

Снова задумываемся над тем, что именно представляет этот интерфейс, какова его ответственность. Drawable - способный нарисовать себя. Но тогда зачем здесь статические методы round, fillOval, drawOval?
Мы же обсуждали, что для общего кода правильнее использовать абстрактные классы. А в этом случае все методы - статические. То есть их вообще можно вынести в отдельные классы, например, round - в MathUtils, drawOval и fillOval - в DrawUtils. И затем их можно просто вызывать как статические операции. А если хочется более компактной записи - можно использовать статический импорт.

[As-pasa]

Статические математические и графические функции вынес в Utils объекты. Спасибо за упоминание статического импорта, буду теперь пользоваться)

[alklepin]

Если следовать идее, что мы разделяем классы, отвечающие за модель (хранят состояние объектов на игровом поле), и классы, отвечающие за рисование, то здесь тоже следовало бы оставить только правило рисование, а информацию о размещении целевой точки унести в отдельный класс (того же уровня, что и RobotModel; условно, TargetModel)

[alklepin]

Если этот класс отвечает только за модель робота, то координаты целевой точки - лишние. Стоит разделить модели.
Достаточно представить ситуацию, что хотим изменить логику робота, но при этом сохранить логику указания цели. И что тогда будет с текущей версией кода?

[As-pasa]

Теперь RobotModel агрегирует TargetModel, убрав тем самым необходимость хранения координат точки и их обновления

[alklepin]

В Java обычно действует соглашение, что имена пакетов пишутся маленькими буквами.

[As-pasa]

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

[alklepin]

Здесь (да и в других частях кода) есть общая проблема из области многопоточности: значения связанных полей запрашиваются в разных операциях. То есть возможна ситуация, что мы начинаем здесь чтение, читаем компоненту X, приостанавливаемся (поток переключается), происходит изменение X и Y в модели, снова переключатся потоки, и читаем Y. В итоге получаем комбинацию X и Y, которая в модели вообще не возникала! Надо осознать проблему, разобраться и поправить. Пока не подсказываю как, но тоже готов обсудить, если возникнут сложности.

[As-pasa]

Для атомарности чтения заменил во всех моделях методы получения отдельных координат на общий метод, возвращающий тройку чисел(вектор), описывающий поведение. Кроме того, внутри методов, возвращающих вектор, поставил критические секции, в которых копирую текущие значения полей, после чего возвращаю копии, чтобы операции мутации объекта никак не отражались на возвращаемом результате.

[alklepin]

После разделения контроллеров стало лучше.
И теперь обращу внимание вот на что. В коде стараемся использовать имена так, чтобы они были логичным, чтобы они мешали написать код неправильно. Поэтому подобные вещи надо называть похоже.
Так, в данном случае ModelPositionController чем управляет? Он управляет моделью TargetModel. Поэтому было бы логично назвать его TargetModelController (я бы так сделал). Тогда сами имена будут семантически связывать эти разные классы.
И, аналогично, RobotModelController.

[alklepin]

Эти поля теперь не используются. Следует привыкать удалять ставший ненужным код, чтобы не замусоривать проект.

[alklepin]

Тут тоже замечание по именованию. RobotDraw - это "роботский розыгрыш", если перевести. :)
Так что тоже совет: надо серьезно относиться к выбору имен полей, проверять правильность выбора метода с помощью словарей или переводчиков (попутно и знание английского будет улучшаться).
В данном случае было бы, наверное, уместно назвать поле как-то так: m_robotView (view - это представление, довольно стандартный термин для описания вида чего-то).

[As-pasa]

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

[alklepin]

И тут снова про имена: похожие сущности должны называться похоже. А что здесь: DefaultRobot и TargetDrawRepresentation; m_robotDraw и m_target - я бы сказал, что вообще похожесть не просматривается, хотя в обоих случаях речь идет о визуальном представлении чего-то, робота и целевой точки соответственно.

[As-pasa]

Сделал именование более однородным

[alklepin]

Здесь тоже есть семантическая путаница. Не советую смешивать понятия вектора и точки (хотя они и представляются одинаково набором координат). А в этой точке вообще лучше говорить не про вектор, а про состояние робота. Поэтому лучше было бы ввести класс RobotState, который и читался бы из модели робота.

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

[As-pasa]

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

[alklepin]

В предыдущем ревью я попытался подсказать, что в таких случаях не надо распаковывать состояние на отдельные переменные, а надо передавать его целиком. В том числе и здесь при вызове конструктора.

[As-pasa]

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

[alklepin]

И здесь тоже было бы логичнее не распаковывать состояние, а передавать его целиком. А еще лучше было бы в этом вызове передавать в метод update полную модель робота, чтобы уже сам визуализатор принял решение, какая именно информация ему нужна для отрисовки.

[alklepin]

И еще один пример неудачного выбора набора параметров.
Вот смотрим, для чего нужны эти три параметра? Нужны ли они классу GameWindow? Нет, не нужны. Ему нужен визуализатор (GameVisualizer). А параметры нужны визуализатору. Но тогда зачем визуализатор создавать внутри окна? Его можно создать снаружи и передать как параметр.

[alklepin]

А еще лучше было бы ввести новый класс, содержащий состояние как набор моделей. И его бы тогда и передавали в конструкторы окошек. При этом контроллер здесь выглядит как-то странно... С учетом его простоты и отсутствия какого-то выраженного контракта я бы сам его вообще убрал.
Контроллеры как отдельные сущности нужны, когда в них есть какая-то нетривиальная логика (контроля модели), но здесь ее нет. Поэтому нет смысла в отдельной сущности.

[alklepin]

И вот эту подписку тоже правильнее было бы сделать внутри кода инициализации визуализатора. Ведь это ему нужны оповещения от модели, а не окну.

[alklepin]

Вот тут просматриваются сраз два улучшения.

1. можно было бы для оповещения об изменении координат сделать отдельный тип сообщения. Иначе, в текущей реализации, любое оповещение будет приводить к обновлению окна с координатами, даже если координаты не изменялись. Это замечание демонстрирует важность внедрения типов сообщений, чтобы получатель мог их различать.
2. можно было бы координаты передавать прямо в сообщении. Тогда не потребовалось бы сохранять локаьлную ссылку на модель. Просто подписались бы на событие и через событие получали бы данные. Это замечание проявляет вот какую мысль. У каждого объекта есть некоторый жизненный цикл. И большое количество ссылок на обхект (ссылки на модель в данном случае) усложняют обслуживание жизненного цикла объекта. Достаточно представить, что в какой-то момент во время работы программы захотим заменить модель робота. И как тогда заменить все ссылки на эту модель в других классах? Поэтому если можно обойтись без дополнительных ссылок, лучше их и не создавать.

[alklepin]

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

[alklepin]

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

[alklepin]

Лучше все же не называть этот класс Vector. Это состояние робота. То есть RobotState.
Если хочется, то можно выделить интерфейс, представляющий возможность получить координаты точки (вот он похож на семантику вектора) и реализовать его и в состоянии робота, и в состоянии целевой точки.

[alklepin]

Кстати, несмотря на то, что теперь есть окошко с координатами, навигация робота все равно работает некорректно - робот можно загнать в вечный цикл, когда он не может попасть в целевую точку (ездит по кругу рядом с ней).
Надо бы поправить. :)

[alklepin]

Код стал лучше, но все же продолжаем его улучшать с точки зрения читаемости.
Вот вопрос: а какая ответственность у этого класса?
У меня складывается такая формулировка (исходя из того, что в этом классе написано): этот класс формирует события, заставляющие робота обновить свое состояние. Так ведь?
Но тогда и название должно быть каким-то таким: RobotsMovementEventsGenerator (вместо Generator можно использовать слово Producer). Тогда из названия проще извлекается смысл кода. Либо надо этот смысл писать в комментариях, чтобы не было неоднозначности. И контроллером этот класс все же, как мне кажется, называть не очень уместно, так он не занимается какой-то трансляцией событий от пользователя в модель.
И если следовать моей идее, то в дальнейшем возможно следующее расширение: один такой класс может генерировать события для обновления всех роботов на игровом поле. То есть при формировании классов, планировании их ответственности желательно продумывать возможные пути развития кода.

[alklepin]

Я бы прямо здесь вызывал метод m_mode.updatePos, без перевызова метода.
А еще в подобном коде обычно делается связь через слабую ссылку (т.е. тип m_model следовало бы сделать WeakReference), чтобы избежать ситуации, что мы по каким-то причинам решили отказаться от конкретного робота (удалить из памяти), но он остается в памяти и продолжает получать события из этого таймера.
Соответственно, тогда перед вызовом updatePos сначала получаем жесткую ссылку, проверяем, что она отлична от null. Если не null, то вызываем updatePos. Если null, вызываем метод calcel для отмены задания таймера.
Предлагаю попробовать это реализовать, чтобы опробовать работу со слабыми ссылками.

[alklepin]

А зачем здесь интерфейс RobotStateReader? Почему бы просто не использовать RobotState?
Тут рассуждения такие. У интерфейса ровно одна реализация, причем трудно представить, ради чего могла бы потребоваться иная (ибо интерфейс представляет собой так называемую структуру DTO - Data Transfer Object, для таких объектов-структур обычно интерфейсы просто не вводят, ибо смысла в них нет). И вообще, если есть единственная реализация интерфейса и других (очевидных) не предполагается, то не надо вводить интерфейс, можно пользоваться просто классом (ибо он все равно индуцирует интерфейс). А если потребуется вторая реализация, то вот именно в этот момент и выделим интерфейс (это простой рефакторинг, который можно провести даже в автоматическом режиме).
И даже если делать связку интерфейс+реализация, то тогда имена были бы такими:

• или IRobotState (интерфейс) и RobotState (реализация)
• или RobotState (интерфейс) и RobotStateImpl (реализация)
  Это стандартные схемы именования интерфейсов и классов в подобных случаях.

[alklepin]

Тут есть вот какая проблема. Вот получили мы на входе настроенный графический контекст. И начали его изменять. Но ведь этот контекст потом окажется в каком-то другом методе рисования, он там окажется измененным (мы же установили цвет, изменили правило трансляции координат и т.п.)! То есть следующий метод рисования окажется зависим от этого нашего (и непредсказуемо зависим!).
Поэтому общее правило такое: полученный графический контекст должен остаться в исходном состоянии. Так что:

• или запоминаем те элементы контекста, которые собираемся изменять, а потом восстанавливаем их в блоке finally;
• или создаем копию контекста и рисуем уже на ней, в конце разрушая ее, Получается как-то так:

public void paint(Graphics g1) {
 final Graphics2D g = (Graphics2D)g1.create();
    try {
         // здесь рисуем
    } finally {
         g.dispose();
    }
}

[alklepin]

Эту проблему все же надо починить.

[alklepin]

Здесь тоже интерфейс по сути не нужен...

[alklepin]

Класс спроектирован как неизменный. Зачем тогда поля с пометкой volatile? Здесь было бы уместнее final...

[alklepin]

Здесь тоже неизменный класс. Уместно и поля сделать финальными.

[alklepin]

Вот в этой строчке среда разработки подсказывает, что переменная не используется. То есть,, получается, что строчку можно бы удалить (ибо объект не используется). Но в реальности это не так, ибо у конструктора есть побочный эффект в виде создания таймера и привязки таймера к модели. Но этот эффект неочевиден, то есть фактически нарушили инкапсуляцию.
Правильный подход здесь напрашивается такой:

1. RobotUpdateController (который на самом деле не контроллер) реализуется по шаблону синглтон
2. У этого синглтона реализуется операция наподобие attachModel(RobotModel), чтобы проявить этот эффект связывания модели с каким-то статическим контекстом.
   При таком подходе логику кода понять будет проще.
   Альтернативный вариант такой. RobotUpdateController оставляется обычным классом (просто будем использовать лишь одну копию). Ссылку на него храним, например, в MainApplicationFrame (он будет владельцем этого класса, ведь при остановке программы может оказаться нужным остановить поток таймера). Ну а модели по-прежнему регистрируем в этом экземпляре RobotUpdateController с помощью метода attachModel.

[alklepin]

Здесь не буду рассматривать как замечание, но обращу внимание, что в реальном коде так лучше не делать. Если в классе есть конструктор, то лучше все поля инициализировать именно там. Неудобно, когда часть полей инициализируется в конструкторе, а часть - вне него. Особенно, когда вне конструктора используется нетривиальная логика (как в данном случае - вызов даже отдельного метода для запуска таймера)

[alklepin]

Ну вот снова тот же нюанс, на который уже неоднократно я обращал внимание: слева - View, справа - Representation. Но схожие вещи надо называть одинаково! Читать код и проверять его правильность проще!

[alklepin]

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

[alklepin]

И снова код, где вместе идут модель, контроллер и модель. Это должно наводить на мысль, что эти сущности связаны и, вероятно, имеет смысл иметь какую-то более общую сущность, наподобие GameModel, агрегирующую эти отдельные модели. Или даже GameState, где смогли бы находиться и модели, и контроллеры.
Эту доработку можно сделать (ибо несложно, да и в дальнейшем скорее всего поможет), но придираться к этому не буду.

[alklepin]

Забавное использование BorderLayout. Но вообще в таких случаях, когда надо разместить однотипные элементы лучше работает или BoxLayout, или GridLayout, или GridBagLayout.
Здесь придираться не буду.

[alklepin]

И вот тут как раз видно, что к этому моменту мы подготовили набор объектов, составляющих основу "игры" (модели и контроллеры). То есть класс, объединяющий все эти сущности был бы действительно уместен.
В реальной ситуации такой общий контекст создавался бы за счет контейнера зависимостей. Но можно было бы и просто создать класс, в котором будут находиться все базовые компоненты, необходимые для работы приложения (что-то наподобие конструкции ServiceLocator).
Это опять же не замечание, а демонстрация, как могла бы пригодиться рассмотренная на занятиях технология.

[alklepin]

И надо все же починить ошибку навигации: если целевая точка находится близко к роботу, то он будет кругами вокруг ездить. :)
Все же одна из целей задачи - именно починить навигацию...