Первая реализация ТЗ 8ого спринта. #3
Conversation
EugeneLenkevich
left a comment
EugeneLenkevich
left a comment
There was a problem hiding this comment.
Привет, Вячеслав! Спасибо за PR.
Отличная работа! :)
Есть совсем небольшие пометки, посмотри комментарии ниже, пожалуйста
В первую очередь переделать бы исключение в проверяемое и сделать на него тест, остальное в принципе мелочи. НУ и закомментированный код лучше не оставлять, толку не особо, но заметно захлямляет
| @@ -0,0 +1,8 @@ | |||
| package exception; | |||
|
|
|||
| public class IntersectionException extends RuntimeException { | |||
There was a problem hiding this comment.
extends Exception
Тут наверно лучше проверяемое исключение, то что задачи пересеклись - это же ожидаемая ситуация, а не непредвиденная ошибка :)
src/manager/InMemoryTaskManager.java
Outdated
| return o1.getStartTime().compareTo(o2.getStartTime()); | ||
| } | ||
| }; | ||
| private TreeSet<Task> prioritizedTasks = new TreeSet<>(comparator); |
There was a problem hiding this comment.
private Set prioritizedTasks = ...
Давай, все таки, будем использовать абстрактные интерфейсы как типы переменных, можно даже Collection. По сути дела это сокрытие деталей внутренней реализации, упрощение кода
| subtasks.put(subtask.getId(), subtask); | ||
| subtask.getEpic().getSubtaskList().put(subtask.getId(), subtask); | ||
| if (subtask.getStartTime() != null) { //для создания сабтасок без времени в конструкторе. | ||
| // Может есть смысл в принципе убрать возможность создавать таски без времени, но пока не уверен |
There was a problem hiding this comment.
Пусть будет, не сильно мешает же
src/manager/InMemoryTaskManager.java
Outdated
| List<Task> intersectionList = getPrioritizedTasks().stream() | ||
| .filter(task1 -> newTask.getStartTime().isBefore(task1.getEndTime()) && newTask.getEndTime().isAfter(task1.getStartTime())) | ||
| .collect(Collectors.toList()); | ||
| if (!intersectionList.isEmpty()) { |
There was a problem hiding this comment.
вместо .collect(Collectors.toList()) можно было попробовать сделать, например, anyMatch() и обойтись без цикла :)
| .stream() | ||
| .filter(task -> task.getStartTime() != null) | ||
| .collect(Collectors.toList()); | ||
| prioritizedTasks.addAll(notNullTasks); |
There was a problem hiding this comment.
Кстати, а нужен ли тут TreeSet если можно было список отсортировать и получилось бы то же самое ...
Он бы был очень кстати если бы мы при добавлении задач сразу их складывали в TreeSet, тогда бы у нас был всегда отсортированный список, а тут мы его сортируем при каждом обращении, можно был любую коллекцию использовать :)
| taskManager.createSubtask(subtask2); | ||
| assertEquals(epic1.getStatus(), Status.IN_PROGRESS); | ||
|
|
||
| } |
There was a problem hiding this comment.
Хорошо бы еще исключение при пересечении задач протестировать :)
src/manager/InMemoryTaskManager.java
Outdated
| if (intersection) { | ||
| throw new IntersectionException("Задача " + newTask.getName() + " пересекается с уже существующей"); | ||
| } | ||
| } catch (IntersectionException e) { |
There was a problem hiding this comment.
Что то не то, мы кидаем проверяемое исключение чтобы обернуть тут же в менеджере в непроверяемое. В таком виде так не делают :)
Достаточно не ловить это исключение. Компилятор добавит его в сигнатуру метода, ну и хватит в рамках этого задания. А в целом, так мы обяжем внешний код обрабатывать ситуацию пересечения задач. Внешний код добавляет задачу, вот пусть и решает что делать с тем что данные не корректны
А тест ок, в нем только тип исключения поменять надо будет :)
EugeneLenkevich
left a comment
EugeneLenkevich
left a comment
There was a problem hiding this comment.
Ага, именно так. Мы в общем случае не знаем как обрабатывать эту ситуацию и вот, с помощью таких исключений, мы делегируем эту проверку на уровень выше. Причем работает это на уровне компилятора. Вышестоящий код обязан или обработать ситуацию или выбросить еще выше. Если же совсем никто не перехватит исключение, то приложение будет вовсе падать так как поведение получится неопределенным.
Еще важная особенность - проверяемое исключение есть в бизнес-процессе потому мы его явно и объявляем в сигнатурах методов и пр. В отличие от непроверяемых которых в БП нет, предугадать их невозможно и означают они вообще другое - дефекты в самих алгоритмах :)
No description provided.