-
Notifications
You must be signed in to change notification settings - Fork 0
Первая реализация ТЗ 9ого спринта... #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
EugeneLenkevich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Привет, Вячеслав! Спасибо за PR.
Отличная работа! :)
Есть совсем небольшие пометки, посмотри комментарии ниже, пожалуйста
Ну и тесты не стал делать на HttpTaskManager? Лучше, конечно, сделать, хотя бы самый простейший, это был бы ценный практический опыт :)
src/manager/Managers.java
Outdated
| return new InMemoryHistoryManager(); | ||
| } | ||
|
|
||
| public static Gson getGson() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Единое статическое поле для создания этого объекта - все верно, но будь то бы не место ему в Managers, пусть лучше отдельный файлик будет, все таки по смыслу совсем разные вещи сериализатор и менеджеры :)
src/server/HttpTaskServer.java
Outdated
| public class HttpTaskServer { | ||
| public static final int PORT = 8080; | ||
| private HttpServer httpServer; | ||
| /* public static final Gson gson = new GsonBuilder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ну и убрать бы сразу закомментироанный код, не к чему его держать, бардак только от него :)
src/server/HttpTaskServer.java
Outdated
| } | ||
|
|
||
| public static void main(String[] args) throws IOException { | ||
| InMemoryTaskManager taskManager = new InMemoryTaskManager(Managers.getDefaultHistory()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new InMemoryTaskManager
А метод из Managers тут не подойдет разве? Лучше использовать один способ делания одинаковых вещей чем много, добавляет сложности
EugeneLenkevich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тест получился, отлично, спасибо!
No description provided.