@msk, wybacz, że tak długo, ale mam teraz sporo swoich spraw na głowie i mniej czasu na wnikliwe recenzowanie kodu i pisanie sensownych postów dłuższych niż 3 zdania.
Nadal masz logikę biznesową w repozytoriach.
Np. metoda AddTask
. Ona nie powinna dostawać userId
, tylko dostać gotowy obiekt task
z wartością User ustawioną warstwę wyżej (czyli w serwisie). Podobnie z ustawianiem Category
/OldCategory
. Serwis niech ma zależności do trzech repozytoriów: zadań, kategorii i użytkowników i niech on utworzy obiekt Task
, ustawi mu User
oraz Category
, a UserRepository niech go jedynie doda do bazy.
Podobnie metody DoneTask
i RestoreTask
- one nie powinny znajdować się w repozytorium. Mogą iść do serwisu - ale najlepiej, gdyby znalazły się w Task
, bo przecież zmieniają jedynie stan wewnętrzny tego konkretnego obiektu.
EditTask
- dziwne wydaje mi się to przepisywanie właściwości z argumentu tej funkcji do tego nowo załadowanego z contextu. Pracujesz na odłączonych obiektach?
Repozytorium ma mieć zestaw metod Get
oraz Add
, Update
i Delete
. Nic więcej nie jest w nim potrzebne, miejsce na logikę biznesową jest wyżej. Repozytorium to tylko kolekcja danych - jak tablica czy lista, tylko z nieco innym API.
Ten ApplicationUserManager/Store
powinien być zależnością serwisu, nie repozytoriów.
TaskService
:
W metodzie DeleteTask
najpierw pobierasz task z repozytorium, a potem wołasz metodę Delete
z repozytorium, która również wczytuje task z contextu. Po co robić to samo dwa razy? Tak samo jest zresztą z większością metod w tej klasie - najpierw coś pobierasz, żeby zawołać metodę repozytorium, która pobiera to samo... No, ale jak pozbędziesz się zbędnych metod z repozytorium, to nie będzie tego problemu.
W DoneTask
- dwa razy pod rząd wołasz repository.GetTasksByUser(userID)
, przy czym raz sprawdzasz, czy wynik nie jest null
, co w ogóle nie jest możliwe w tym przypadku.
ChangeTaskStatus
- ta metoda powinna wyglądać tak:
public void ChangeTaskStatus(int taskId)
{
Task task = repository.GetTaskById(taskId);
task.SwitchStatus();
repository.Save();
}
A task.SwitchStatus() tak:
public void SwitchStatus()
{
this.IsDone = !this.IsDone;
}
I w 4 linijkach zawarłem to, co u Ciebie zajmuje około 20 i jest rozbite na 2 warstwy.
Ale najgorsze w tej klasie jest to message
i TempData
. Serwis ma być niezależny od GUI. Jak chcesz ustawiać jakieś komunikaty dla użytkownika, to robisz to w GUI, nie w serwisach.
Encje...
https://github.com/micskw/TaskToDo/blob/master/TaskToDo.Domain/Entities/Task.cs - to jest tak naprawdę ViewModel. To nie jest żadna domena ani encja, bo nie ma tam logiki. Ty po prostu mapujesz ViewModele na bazę. To nie jest dobre z architektonicznego punktu widzenia, bo to zwykłe złamanie zasady jednej odpowiedzialności. Kończy się tym, że tę samą klasę zmieniasz zarówno, gdy potrzebujesz zmienić format przechowywanych danych, jak i sposób ich wyświetlania w GUI - a powinien być tylko jeden powód do zmiany każdej klasy.
Może to prowadzić do kłopotów, gdy np. nie będziesz chciał czegoś wyświetlać w GUI, i przypadkiem usuniesz właściwość z tego modelu, to będziesz miał błędy po stronie warstwy dostępu do danych, i vice versa.
Nie wspominając już o tym, że po co w ogóle wczytywać z bazy dane, których nie ma zamiaru się wyświetlić w GUI?
Napisałeś, żeby nie ustawiać viewmodelu w kontrolerze. W takim razie gdzie to powinienem zrobić?
Moim zdaniem, ViewModel powinien być zwracany z serwisu aplikacyjnego, a GUI nie powinno mieć żadnego dostępu do encji, repozytoriów, warstwy dostępu do danych, itd. GUI ma być głupie i operować na własnych obiektach dostosowanych do tego, co chcemy za jego pomocą wyświetlać i edytować. Tylko to wymaga prawdziwych ViewModeli, a Twoje pełnią również funkcję persistence modeli.
No i popracuj trochę nad nazewnictwem, np. nie DoneTask
ale MarkAsDone
lub coś w tym rodzaju. No i namespace nazwałbym raczej Repositories
niż Repository
, podobnie CategoriesRepository
, a nie CategoryRepository
.