Conversation
…on from GameVisualizer to Drawable interface
…ions removed from GameVisualizer
…ced by constructor local variables
…alls replaced by controller calls, model and controller creation moved to MainApplicationFrame contructor
… ModelPositionController
… and Update method removed
…nerating it and rebuilding source from state
…cs subsystem. Logger now saves its state
|
|
||
| import java.io.IOException; | ||
|
|
||
| public class ConfigException extends IOException { |
There was a problem hiding this comment.
А зачем наследоваться именно от IOException?
Обычно в своем коде строятся собственные иерархии исключений, чтобы они не смешивались с другими предопределенными типами...
robots/src/config/ConfigReader.java
Outdated
| import java.io.ObjectInputStream; | ||
|
|
||
| public interface ConfigReader extends Closeable { | ||
| Object readObject() throws ConfigException, IOException, ClassNotFoundException; |
There was a problem hiding this comment.
Здесь две проблемы:
- исключение IOException фактически маскирует ConfigException (как наследника)
- зачем вызывающему коду показывать ошибки обнаружения классов? следовало бы их обернуть в наш ConfigException.
| public PredeterminedPathConfigReader(String path){ | ||
| m_path=new File(path); | ||
| try{ | ||
| m_data=new ObjectInputStream( new BufferedInputStream(new FileInputStream(m_path))); |
There was a problem hiding this comment.
А зачем открывать файловый поток в конструкторе? Почему бы не открывать и не закрывать его непосредственно в методе readObject? Логика оказалась бы проще... Да и тогда можно было бы не делать классы PredeterminedPathConfigReader и PredeterminedPathConfigWriter закрываемыми...
Та логика, которая сейчас реализована, была бы уместной, если бы работа с файлом состояла бы из серии операций (т.е. они были бы объединены общим состоянием - например, положением в файле). Но в реальности такой необходимости нет.
There was a problem hiding this comment.
Посмотрел код дальше и понял, зачем так было сделано. Так что замечание отменяется. Тут, получается, фактически сделали обертку над сериализатором, и действительно выполняется серия операций.
| m_data=new ObjectInputStream( new BufferedInputStream(new FileInputStream(m_path))); | ||
| } | ||
| catch (IOException e){ | ||
| m_data=null; |
There was a problem hiding this comment.
Так "проглатывать" исключение нельзя. Получается. что увидим эту ошибку лишь позже, когда вызовем операцию записи. Причем в стеке вызова не увидим исходную точку, где ошибка реально возникла.
Надо переделать эту логику.
| public PredeterminedPathConfigReader(String path){ | ||
| m_path=new File(path); | ||
| try{ | ||
| m_data=new ObjectInputStream( new BufferedInputStream(new FileInputStream(m_path))); |
There was a problem hiding this comment.
Здесь есть еще одна проблема. Представим, что конструктор FileInputStream уже успел выполниться, но остальные классы создаться не успели, и в m_data оказалось значение null. Тогда файл, если он был, окажется заблокированным, ибо закрыть файловый поток будет некому...
Надо подумать, как правильно организовать логику создания объектов и их закрытия в ситуациях, когда в коде возникают исключения.
| if(m_data==null){ | ||
| return; | ||
| } | ||
| m_data.close(); |
There was a problem hiding this comment.
Метод close должен обладать свойством идемпотентности (уж использую этот термин из программирования и математики). То есть многократный вызов метода должен быть эквивалентным однократному.
В данном случае это означает, что после успешного выполнения операции close (а может быть и после неуспешного тоже) надо выставить m_data в null, чтобы предотвратить попытки повторного закрытия потока.
| import java.io.*; | ||
| import java.nio.file.Paths; | ||
|
|
||
| public class PredeterminedPathConfigWriter implements ConfigWriter { |
There was a problem hiding this comment.
Здесь все те же замечания, что и для reader-а
| m_stream.close(); | ||
| } | ||
|
|
||
| public static void main(String[] args) { |
There was a problem hiding this comment.
Да, можно делать такие стартовые методы для проверки кода. Но правильнее использовать JUnit для этих целей (ибо тесты же пишем).
| String d ="fooo"; | ||
|
|
||
| var p= new File(System.getProperty("user.home"),"config.conf"); | ||
| System.out.println(p); |
There was a problem hiding this comment.
Когда код отлажен, вывод на консоль, по идее, надо убирать.
robots/src/gui/RobotsProgram.java
Outdated
| } | ||
| }); | ||
| } | ||
| private static void initProgramState(){ |
There was a problem hiding this comment.
Этот метод и следующий очень похожи. И это не случайно. Ибо оба решают две задачи: формирование состояния (нового или существующего), а затем инициализация оконных классов.
Так вот логик инициализации (которая общая) стоило бы вынести в отдельный метод, чтобы избежать дублирования кода.
robots/src/gui/RobotsProgram.java
Outdated
| RobotState robotContainer=m_robotModel.getState(); | ||
| TargetState targetContainer=m_targetModel.getState(); | ||
| LoggerSourceState logs=m_logs.getState(); | ||
| writer.writeObject(windowContainer); |
There was a problem hiding this comment.
Вот если подумать, то логика, основанная на сериализации (как сейчас реализовано) в дальнейшей скорее всего окажется неудобной. Достаточно представить, что будем добавлять какие-то новые (опциональные) окошки. И как тогда хранить состояние? Всегда формировать все окошки, даже если их нет на экране? А иначе будет проблема, что мы не знаем, какие именно объекты были записаны в этом внешнем файле.
Банально, как только добавим еще одно окно, старая конфигурация окажется непригодной... Здесь в учебной программе это не очень страшно, а в настоящем продукте?
There was a problem hiding this comment.
И, кстати, если в процессе работы программы окно (например, окно с координатами) закрыть, то код сохранения не сработает...
| import java.io.Serializable; | ||
|
|
||
| public class MainWindowStateContainer implements Serializable { | ||
| public InnerWindowStateContainer GameState; |
There was a problem hiding this comment.
Не забываем, что в Java поля и переменные называются с маленькой буквы.
| PositionShowState = positionShowState; | ||
| } | ||
|
|
||
| public InnerWindowStateContainer PositionShowState; |
There was a problem hiding this comment.
Все поля стоит группировать в начале класса, чтобы было легко оценивать устройство класса с точки зрения данных.
| } | ||
| } | ||
| public MainWindowStateContainer getFrameState(){ | ||
| return new MainWindowStateContainer(m_game.getState(),m_logger.getState(),m_coordShow.getState()); |
There was a problem hiding this comment.
А как теперь будет изменяться код по мере добавления новых окошек? Так и будет разрастаться набор параметров конструктора?
| @@ -0,0 +1,4 @@ | |||
| package gui.serial; | |||
|
|
|||
| public interface MySerializable { | |||
There was a problem hiding this comment.
Может быть, раз уж введен интерфейс для собственной механики сериализации, то и метод получения состояния для сохранения в нем описать? В принципе, и метод для восстановления тоже мог бы находиться в этом интерфейсе (т.е. логика восстановления была бы такой: сформировали объект, а потом вызвали операцию восстановления состояния).
robots/src/gui/GameWindow.java
Outdated
| pack(); | ||
| } | ||
| public InnerWindowStateContainer getState(){ | ||
| return new InnerWindowStateContainer(getX(),getY(),getSize().width,getSize().height); |
There was a problem hiding this comment.
Можно заметить, что эта конструкция встречается в коде несколько раз (в разных классах). То есть имеем дублирование кода. И тут напрашивается введение конструктора InnerWindowStateContainer от java.awt.Component. Собственно, и восстановлением мог бы, в принципе, заниматься InnerWindowStateContainer.
|
|
||
| import java.io.File; | ||
|
|
||
| public class PredeterminedPathFileSupplier implements FileSupplier{ |
There was a problem hiding this comment.
А зачем так усложнять код? Почему было просто не использовать File в качестве абстракции для представления целевого файла? :)
Я бы понял смысл этой абстракции, если бы предполагалась возможность изменять эти целевые файлы на ходу. Но тогда было бы правильнее просто ввести сущность для хранения конфигурации приложения, где были бы, в частности, нужные данные о файлах.
There was a problem hiding this comment.
Я как раз предполагал возможность использования окна выбора файлов(JFileChooser). Мне казалось что абстрагироваться от способа задачи файла хардкодом/выбором с помощью пользовательского интерфейса было бы неплохо. Но логика не была реализована, так что пока это действительно абстракция ради абстракции
There was a problem hiding this comment.
P.S. не до конца прочитал ваше предолжение о новой сущности+выяснил что логика работы с JFileChooser не является блокирующией и текущая система абстракций все равно не позволит без костылей добавить этот функционал
| import java.io.Serializable; | ||
|
|
||
|
|
||
| public abstract class AbstractWindowConstructor implements WindowConstructor, Serializable { |
There was a problem hiding this comment.
Идея с фабрикой окошек - хорошая. Хвалю.
Только все же стоило класс назвать, например, так: WindowFactory (слово abstract в названии является лишним, ибо все остальные фабрики будут отличаться по имени, то есть конфликта имен изначально нет, ну а то, что класс абстрактный, увидим по ключевому слову abstract в его описании.
А еще здесь также появляется возможность для применения дженериков. Смысл в чем. Мы же знаем, что конкретная фабрика порождает окошки конкретного типа. Поэтому можно было зафиксировать эту связь:
public abstract class WindowFactory<T extends JInternalFrame>
{
public T construct(ModelAndControllerLocator locator){ ... }
}
public class LogWindowConstructor extends AbstractWindowConstructor<LogWindow>
{
@Override
protected LogWindow windowFabricMethod(ModelAndControllerLocator locator) {
return new LogWindow(locator.getLogSource());
}
}
Видно же, что в итоге удается и описать общую логику (универсальную на основе полиморфизма), и сохранить специфику конкретных систем классов?
Очень советую попробовать сделать этот рефакторинг, чтобы опробовать работу с дженериками.
| public AbstractWindowConstructor(int x, int y,int width,int height){ | ||
| m_windowState=new InnerWindowStateContainer(x,y,width,height); | ||
| } | ||
| protected abstract JInternalFrame windowFabricMethod(ModelAndControllerLocator locator); |
There was a problem hiding this comment.
Снова про именование. Методы обычно называются глаголами. Классы - существительными. Соответственно, здесь более уместным было бы какое-то название, которое обозначало бы операцию построения окна (например, createWindow). А прямо сейчас windowFabricMethod - это существительное (оконный фабричный метод).
|
|
||
| import javax.swing.*; | ||
|
|
||
| public interface WindowConstructor { |
There was a problem hiding this comment.
По идее, этот интерфейс не нужен. Его роль вполне может выполнить соответствующих абстрактный класс. Ведь если посмотреть по коду, этот интерфейс сам по себе особо и не нужен. Везде фактически используются реализации на основе AbstractWindowConstructor...
А если интерфейс и оставлять, то его также можно сделать дженериком.
| } | ||
| } | ||
|
|
||
| private static void onExit() throws FileNotFoundException { |
There was a problem hiding this comment.
А зачем указывать, что выбрасывается FileNotFoundException, если оно на самом деле не выбрасывается (да и не должно ведь, по идее)?
| writeProgramState(modelsInputStream, windowsInputStream); | ||
|
|
||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
А невозможность записать конфигурацию точно должна приводить к завершению программы? Наверное было бы правильнее сообщить пользователю о проблеме (попросить подтвердить выход) и, возможно, остановить выход из приложения, чтобы можно было решить проблему и все же сохранить конфигурацию?
|
Мне понравилась реализация сохранения. Красиво получилось. За это +5 бонусных баллов. |
2 participants
No description provided.