-
Notifications
You must be signed in to change notification settings - Fork 103
For review #84
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
base: for-pr
Are you sure you want to change the base?
For review #84
Conversation
| carRepository.rewrite(); | ||
| } | ||
|
|
||
| public void displayCars() { |
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.
этот сервис работает с машинами. Он должен отвечать за работу с машинами. А за вывод отвечать не должен. Завтра будем писать в файл/БД или отдавать результаты по Сети. Что тогда делать с этим методом?
| return carRepository.getCars()[foundCarIndex]; | ||
| } | ||
|
|
||
| public void update() { |
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.
Мутная история. Выглядит, будто это не ответственность конкретно этого сервиса
|
|
||
| public class UnableParsingException extends RuntimeException { | ||
| public UnableParsingException(String message, String value) { | ||
| super("%s %s".formatted(message, value)); |
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.
Просто ParsingException будет достаточно
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.
И достаточно странное формирование сообещния. Я же могу что угодно передать параметрами, из-за чего на выходе будет бессмысленная строка. В данном случае логичнее сформировать строку при создании эксепшна и передать одним параметром
| private static final String ELEMENT_SEPARATOR = "\n"; | ||
| private static final String FIELD_SEPARATOR = " "; | ||
|
|
||
| public String toLine(Car car) { |
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.
это не ответственность парсера - ведь ты делаешь ровно противоположное парсингу действие:)
| this.cars = cars; | ||
| } | ||
|
|
||
| public void append(Car car) { |
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.
лучше таки add()
| public Car find(Car car) { | ||
| int foundCarIndex = indexOf(car); | ||
|
|
||
| if (foundCarIndex == -1) { |
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.
можно объединить с return ниже, описав как тернарное выражение
| .formatted(years, months, days, hours, minutes, seconds); | ||
| } | ||
| } | ||
|
|
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.