task6#6
Conversation
…ия MVC связки робота; Создан класс ModuleLoader для загрузки классов, реализующих интерфейс MVCController; Добавлен новый пункт меню для выбора модели. При взаимодействии создаётся окно ModuleLoaderDialog, в котором можно выбрать jar файл и указать пакетный путь к нужному классу;
…WithWindows; Добавлена Димина модель для теста :);
vpyzhyanov
left a comment
vpyzhyanov
left a comment
There was a problem hiding this comment.
Есть замечания. В целом задача сделана хорошо. Задача принята.
| */ | ||
| public class GameVisualizer extends JPanel implements PropertyChangeListener { | ||
| private RobotModel robotModel; | ||
| public class GameVisualizer extends JComponent implements PropertyChangeListener { |
There was a problem hiding this comment.
Забыли обновить JavaDoc, теперь это не панель
| public void propertyChange(PropertyChangeEvent evt) { | ||
| if (evt.getPropertyName().equals("robot_update")) { | ||
| this.robotModel = (RobotModel) evt.getNewValue(); | ||
| if (evt.getPropertyName().equals("model_update")) { |
There was a problem hiding this comment.
Кстати, "model_update" лучше вынести в константу. Тогда если она изменится, то это сразу поменяется во всех местах.
| return (JInternalFrame[]) getWindows.invoke(object); | ||
| } catch (IllegalAccessException e) { | ||
| Logger.error("moduleLoader.illegalAccess"); | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
- Если вы выкидываете исключение, то не надо писать ничего в логгер. В логгер будете писать только тогда, когда будете по настоящему обрабатывать исключение. А то может случится так, что ошибка заллогируется дважды, тут и на уровне выше.
- Человекочитамый текст ошибки должен быть именно в сообщении исключения. А вот тут вы должны прочувствовать всю странность и неуместность вашего решения запихнуть локализацию внутрь логгера (логгер не должен отвечать за локализацию и вообще не должен быть с ней связан). Кстати, почему это до сих пор не исправлено?
| try { | ||
| Class<?> clazz = urlClassLoader.loadClass(className); | ||
| Object object = clazz.getConstructor().newInstance(); | ||
| Method getPainter = clazz.getMethod("getPainter"); |
There was a problem hiding this comment.
название переменной сбивает с толку. Так называются только методы, а никак не переменные. Можно назвать, например methodGetPainter
| Object object = clazz.getConstructor().newInstance(); | ||
| Method getPainter = clazz.getMethod("getPainter"); | ||
| try { | ||
| Method getWindows = clazz.getMethod("getWindows"); |
| @Override | ||
| public JComponent getPainter() { | ||
| try { | ||
| return (JComponent) getPainter.invoke(object); |
There was a problem hiding this comment.
Вот тут я вообще не понял. Какой вы метод собираетесь вызывать? Ведь случился NoSuchMethodException и нужно обрабатывать ошибку, а не рисовать неизвестно что.
Либо код написан очень запутанно и в одном методе создаётся несколько разных объектов, из-за чего я не не могу уловить его суть.
| PaneMenuBar.optionMenu.browse=Browse... | ||
| PaneMenuBar.optionMenu.load=Load Jar | ||
| moduleLoaderDialog.fileChooser.title=Path to the jar file: | ||
| moduleLoaderDialog.notEmptyField=Don't leave the fields empty!\n |
There was a problem hiding this comment.
сообщения не должны начинаться и заканчиваться на пробельные и служебные символы. Эти символы должны быть в коде, а не в локализации. Ведь в коде они будут написаны один раз, а в локализации - столько, столько локалей в системе. И их можно забыть дописать.
2 participants
Изменения: