Создать вторую реализацию менеджера - FileBackedTaskManager - #3
Conversation
| import java.nio.charset.StandardCharsets; | ||
| import java.util.*; | ||
|
|
||
| public class FileBackedTaskManager extends InMemoryTaskManager implements TaskManager { |
There was a problem hiding this comment.
достаточно объявить extends InMemoryTaskManager, чтобы неявно уже было, что класс FileBackedTasksManager реализует интерфейс TaskManager
| } | ||
| try { | ||
| reader = new FileReader(this.file, charset); | ||
| bufferedReader = new BufferedReader(reader); |
There was a problem hiding this comment.
можно сделать вот так new BufferedReader(new FileReader(file.getPath(), charset)) и не создавать не нужную переменную
| } | ||
|
|
||
| if (counter < taskObject.getId()) { | ||
| counter++; |
There was a problem hiding this comment.
не нужно наугад поднимать значение счетчика. Например: в файле две сущности с id равными 2 и 4, потому что сущности с id равными 1 и 3 удалили. Соответсветственно после вычитки из файла counter станет 3 и следующая же создаваемая сущность перетрет существующую (если они будут одного типа) или появятся две сущности с одним id (если они будут разного типа). Поэтому нужно найти наибольшее значение id из файла, потом сделать +1 и после этого передать его в counter.
| } | ||
|
|
||
| if (taskObject instanceof Epic) { | ||
| super.setEpic((Epic) taskObject); |
There was a problem hiding this comment.
здесь нельзя пользоваться методом setEpic(), потому что в нем сущности будет присвоено новое id. Нужно в InMemoryTaskManager заготовить protected метод, который будет наполнять соответсвующее хранилище полученными сущностями
|
|
||
| } catch (IOException e) { | ||
| try { | ||
| throw new ManagerSaveException("Файл не прочитан"); |
There was a problem hiding this comment.
перехватывать ManagerSaveException не нужно
| } | ||
| } | ||
|
|
||
| save(); |
There was a problem hiding this comment.
не нужно при вычитке из файла еще и сохранять в него
| } | ||
|
|
||
| } catch (IOException e) { | ||
| System.out.println(e.getMessage()); |
There was a problem hiding this comment.
тут нужно бросить ManagerSaveException с комментарием
| return null; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
переопределяй только те методы, которые отличаются от родительских. в родительском исполнении они итак доступны в дочерних классах
| @@ -0,0 +1,7 @@ | |||
| package exception; | |||
|
|
|||
| public class ManagerSaveException extends Exception { | |||
There was a problem hiding this comment.
сделай наследование от RuntimeException
| return; | ||
| } | ||
|
|
||
| counter = taskObject.getId(); |
There was a problem hiding this comment.
нет контракта, что в файле сущности пишутся в порядке возрастания id, поэтому сначала нужно найти самое большое значение в файле, только потом, после цикла его передать в counter
There was a problem hiding this comment.
Метод getAll() возвращает их отсортированными.
В Task добавлен метод compareTo().
public List<Task> getAll() {
if (counter == 1) {
return new ArrayList<>();
}
TreeSet<Task> allTasks = new TreeSet<>();There was a problem hiding this comment.
Вижу ты переделал, но все же поясню, чтобы было понятнее. Чтобы переопределить порядок хранения в TreeSet нужно при создании в сам TreeSet передать объект Comparator в котором будет указан порядок сортировки. Переопределение метода compareTo() в объектах хранения не распространяется на само хранилище
| counter = taskObject.getId(); | ||
|
|
||
| if (taskObject instanceof Epic) { | ||
| super.setEpic((Epic) taskObject); |
There was a problem hiding this comment.
не используй методы создания сущностей. Здесь не новая сущность, а старая, которую пользователь создал вчера, позавчера и т.д. Добавь в InMemoryTaskManager метод или отдельные методы, которые будут просто наполнять хранилища данными
There was a problem hiding this comment.
Боюсь в таком случае будет вероятность коллизий по айди.
There was a problem hiding this comment.
Или counter лучше вообще не трогать?
There was a problem hiding this comment.
Тогда может расшарить хранилище?
There was a problem hiding this comment.
Так же поясню для лучшего понимания. Тут было два варианта (но по сути это одно и тоже).
- Сами хранилища и counter сделать protected, т.е. доступными в классах наследниках. Соответственно наследник может изменять состояние этих объектов, т.е. наполнить хранилища и указать counter указать следующее значение, чтобы новая сущность получила свое уникальное id
- Все тоже самое мог ди делать пару методов в классе InMemoryTaskManager, получая на вход от наследников (т.е. тоже был бы protected) вычитанные сущности и новое (т.е. max + 1) значение для counter
| } | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
общий комментарий к переопределенным метода: не нужно перехватывать ManagerSaveException, когда он будет отнаследован от RuntimeException, он будет без проблем выбрасываться
| public Task setTask(Task newTask) { | ||
| Task result = null; | ||
| try { | ||
| checkAndReloadFileData(); |
There was a problem hiding this comment.
нет смысла перепроверять файл, в методе save() он будет перезаписан начисто из данных хранилищ (idToTask, idToEpic, idToSubtask) начисто
There was a problem hiding this comment.
Я подумал, а что если в процессе работы программы кто-то изменит файл?
Написал тест на эту тему.
@Test
void shouldReturnValidSubtaskAfterReloadFileWhereOneElementHasRemoved() {
manager.removeTask(1);
manager2 = new FileBackedTaskManager(file);
manager2.removeSubtask(4);
manager2.setEpic((Epic) manager.get(2).getCopy());
manager.setTask(new Task(1,"1","1",null));
FileBackedTaskManager manager3 = new FileBackedTaskManager(file);
String title = manager3.getTask(6).getTitle();
assertEquals("1", title,
"Менеджер не отслеживает изменения произведенные в файле извне.");
}Сейчас прежде чем что-то сделать с файлом, менеджер проверяет, не изменились ли данные в файле.
Если например была удалена последняя задача, он не присвоит новой задаче тот-же айди что и у удаленной.
А ты предлагаешь смотреть какой айди в файле был последним, мне кажется это не правильно с точки зрения назначения идентификаторов. Удаленный айди больше не должен создаваться.
There was a problem hiding this comment.
Ты решил усложнить себе задачу))) такого на учебном проекте не требуется))
| import java.util.Map; | ||
| import java.util.*; | ||
|
|
||
| class InMemoryTaskManager implements TaskManager { |
There was a problem hiding this comment.
в этом классе не должно быть использование ManagerSaveException.
Вся доработка текущего класса - это доступность некоторых его полей для классов-наследников (через изменение их модификатора доступа с private на protected или же через добавление protected методов, которые будут работать с этими полями - тут на твой выбор)
There was a problem hiding this comment.
Я это понял уже после того как дописал методы:
Task get(int id)
Task getWithoutHistory(int id)
Type getType(int id)
List getAll()
Хотя, думаю они давно уже напрашивались.
ManagerSaveException - убрал.
Подготовить проект к первому ревью седьмого спринта