Ocena aplikacji

0

Od jakiegoś czasu uczę się programować w C# i ASP.NET MVC. Napisałem prostą aplikację do zarządzania zadaniami. Chciałem prosić o ocenę kodu, przydatności, itp. Wszelkie uwagi mile widziane.

Aplikacja oczywiście nigdzie nie będzie wykorzystywana. Jest to projekt stworzony na potrzeby mojej edukacji.

Link do kodu: https://github.com/micskw/TaskToDo

1

Ja bym nie dzielił klas na takie paczki Abstract i Concrete.

0

Prosiłbym o ocenę mojego programu. Chciałbym się dowiedzieć czy tego typu aplikacja nadaje się do umieszczenia w CV starając się o staż.

2
  1. Repozytorium Task nie powinno zawierać żadnych metod do operowania na kategoriach, ani też żadnych pól typu CurrentUser czy ApplicationUserManager.
  2. Tasks nie powinno być właściwością repozytorium. Powinieneś mieć metodę w rodzaju GetSomeTasks. Właściwości nie powinny wykonywać operacji tylko od razu zwracać jakieś informacje.
  3. Tutaj panuje bałagan: https://github.com/micskw/TaskToDo/tree/master/TaskToDo.Domain/Concrete Każda klasa jest od czego innego, każda powinna być w innym module, a nawet warstwie. Np. implementacje repozytoriów i kontekst EF nie powinny być w module Domain.
  4. Cała logikę biznesową masz w "repozytorium" zamiast w encjach lub serwisach, w których faktycznie jest miejsce na takie rzeczy: https://github.com/micskw/TaskToDo/blob/master/TaskToDo.Domain/Concrete/EFTaskRepository.cs#L43
  5. Twoje encje to nie są tak naprawdę encje, tylko DTO służące do zapisu do bazy.
  6. Czemu ta klasa ma publiczne settery: https://github.com/micskw/TaskToDo/blob/master/TaskToDo.WebUI/Models/PagingInfo.cs ?
  7. Kontroler to nie jest miejsce na używanie repozytoriów ani na składanie ViewModeli.

Lektura obowiązkowa na forum:
http://commitandrun.pl/2016/05/11/Repozytorium_najbardziej_niepotrzebny_wzorzec_projektowy/
http://commitandrun.pl/2016/05/30/Brutalne_prawdy_o_MVC/

0

@somekind
ad1. Czyli powinienem stworzyć dwa repozytoria, dla Task i Category. Właściwość CurrentUser utworzyłem po to by sprawdzić aktualnie zalogowanego użytkownika.
ad2. Chciałbym pobierać zadania tylko dla aktualnie zalogowanego użytkownika, niestety z tego co wiem sprawdzić można to tylko z poziomu kontrolera, dlatego utworzyłem właściwość CurrentUser, a w kontrolerze przypisywałem do niej:

 _repository.CurrentUser = _repository.manager.FindById(User.Identity.GetUserId());

Da się to rozwiązać w taki sposób aby pobierać z repozytorium zadania tylko dla aktualnie zalogowanego użytkownika bez takich kombinacji?

ad3. Czyli powinienem np. utworzyć nowy projekt na implementacje repozytoriów ale same interfejsy zostawić w modelu domeny? Kontekst również powinienem umieścić w tym projekcie?

ad4. Chyba nie do końca rozumiem. Mam umieścić w serwisach, np. metodę w której będę oznaczał zadania, których czas wykonania już minął? Teraz mam w repozytorium dodaną taką metodę do konstruktora, dzięki czemu w dowolnej implementacji oznacza mi takie zadania jako wykonane.

ad7. Czyli powinienem utworzyć serwisy, które będą korzystały z repozytorium, a kontroler z serwisów?

Może powinienem całkowicie zrezygnować z repozytorium?

Ciekawa lektura, wynika z niej, że oficjalna dokumentacja błędnie opisuje mvc oraz ideę repozytorium. Większość programistów do nauki jednak wykorzystuje oficjalne materiały. Czy osoba stosująca się do zaleceń autora bloga nie będzie mimo wszystko odbierana jako osoba wprowadzająca zamieszanie do projektu? A tym samym z taką osobą pozostali programiści nie koniecznie będą chcieli współpracować.

3
msk napisał(a):

ad1. Czyli powinienem stworzyć dwa repozytoria, dla Task i Category. Właściwość CurrentUser utworzyłem po to by sprawdzić aktualnie zalogowanego użytkownika.

Tak, powinieneś mieć odrębne repozytoria. Task i Category to dwie przecież dwa oddzielne byty.

ad2. Chciałbym pobierać zadania tylko dla aktualnie zalogowanego użytkownika, niestety z tego co wiem sprawdzić można to tylko z poziomu kontrolera, dlatego utworzyłem właściwość CurrentUser, a w kontrolerze przypisywałem do niej:

 _repository.CurrentUser = _repository.manager.FindById(User.Identity.GetUserId());

Da się to rozwiązać w taki sposób aby pobierać z repozytorium zadania tylko dla aktualnie zalogowanego użytkownika bez takich kombinacji?

Zdefiniuj w repozytorium metodę GetTasksByUser(int userId) i przekazuj do niej id użytkownika.

ad3. Czyli powinienem np. utworzyć nowy projekt na implementacje repozytoriów ale same interfejsy zostawić w modelu domeny? Kontekst również powinienem umieścić w tym projekcie?

Tak.

ad4. Chyba nie do końca rozumiem. Mam umieścić w serwisach, np. metodę w której będę oznaczał zadania, których czas wykonania już minął?

Tak. To jest logika biznesowa, umieść ją w encjach/serwisach. Nie w repozytoriach, nie w kontrolerach.

Teraz mam w repozytorium dodaną taką metodę do konstruktora, dzięki czemu w dowolnej implementacji oznacza mi takie zadania jako wykonane.

Dlaczego repozytorium wykonuje logikę biznesową? Dlaczego konstruktor robi takie rzeczy?

Repozytorium to kolekcja. Jedyne, do czego służy do dodawanie, usuwanie, i pobieranie obiektów.
A konstruktory służą do ustawienia stanu pierwotnego obiektu, nie wykonywania jakiejś skomplikowanej logiki.

ad7. Czyli powinienem utworzyć serwisy, które będą korzystały z repozytorium, a kontroler z serwisów?

Jeśli chcesz mieć poprawną architekturę z poprawnym podziałem odpowiedzialności, to owszem. Ponadto, repozytorium implikuje podejście DDD, więc logika biznesowa ma się znajdować w serwisach i encjach, a serwisy aplikacyjne tylko szeregują jej wywołania i zwracają dane do kontrolerów.

Może powinienem całkowicie zrezygnować z repozytorium?

W tak małym projekcie, nie ma ono praktycznego sensu, ale zakładam, że to projekt edukacyjny i chcesz się nauczyć jak je poprawnie stosować.

Ciekawa lektura, wynika z niej, że oficjalna dokumentacja błędnie opisuje mvc oraz ideę repozytorium.

Ale jaka oficjalna dokumentacja? Która oficjalna dokumentacja każe wpychać logikę biznesową do konstruktora repozytorium?
MVC pojawiło się po raz pierwszy w materiałach z PARC, a do repozytorium chyba trudniej o bardziej oficjalne źródła niż blog Fowlera i książka Evansa o DDD. O wszystkim tym jest mowa na blogu.

Czy osoba stosująca się do zaleceń autora bloga nie będzie mimo wszystko odbierana jako osoba wprowadzająca zamieszanie do projektu? A tym samym z taką osobą pozostali programiści nie koniecznie będą chcieli współpracować.

Większość projektów jest skopana, większość zastosowań MVC i repozytoriów niepoprawna, a większość programistów nie rozumie tych wzorców. Jeśli pojawisz się wśród nich i zaczniesz głosić prawdę, to mogą dostać ataku paniki czy innej pomroczności i zacząć przeczyć faktom.
To Ty musisz sobie odpowiedzieć na pytanie, czy chcesz być ignorantem, bo inni tacy są, czy nie.

0

@somekind Poprawiłem mój program. Jeśli mógłbyś sprawdzić i skrytykować co się da byłbym ci bardzo wdzięczny.
Jednej rzeczy tylko nie zmieniłem. Napisałeś, żeby nie ustawiać viewmodelu w kontrolerze. W takim razie gdzie to powinienem zrobić?

2

@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.

0

W przykładach, które przerabiałem encje to było coś co odwzorowywało tabele z bazy danych. Miejsce gdzie nic się nie powinno dziać. Rozumiem, że tak nie trzeba robić. Mogę spokojnie dodawać do nich logikę, zmieniającą wewnętrzny stan, w sposób podobny jak pokazałeś w przykładzie. Wykorzystanie takiej encji w widoku jest niedopuszczalne i zawsze muszę tworzyć dodatkowe viewmodele.

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?

Nie bardzo wiem czym są odłączane obiekty. Sposób, o który pytasz był w przykładzie, który przerabiałem. Uznałem, że tak należy robić.

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 problem

Rozwiązałem to w ten sposób, ponieważ przed usunięciem danego zadania, muszę sprawdzić czy istnieje. Pomyślę nad tym jak to inaczej rozwiązać.

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.

Czyli powinienem przeprojektować projekt i jego zależności. Teraz w Domain znajdują się encje i serwisy. WebUI ma referencję do Domain, a nie powinien mieć możliwości dostępu do encji. Utworzę w takim razie dodatkowy projekt, w którym umieszczę interfejsy serwisów i viewmodele. Nazwę go Contracts, wzorując się na twojej konfiguracji z innego tematu.

0

@msk, wybacz opóźnienie.

msk napisał(a):

W przykładach, które przerabiałem encje to było coś co odwzorowywało tabele z bazy danych. Miejsce gdzie nic się nie powinno dziać. Rozumiem, że tak nie trzeba robić.

Takie podejście zwane jest anemicznym modelem domeny (anemic domain model) i powszechnie uważane za antywzorzec.
Ja na to tak nie patrzę, sam nie mam problemu ze stosowaniem takich struktur danych, tylko po prostu nie nazywam ich encjami. Bo encje kojarzą się z encjami z Domain Driven Design, czyli właśnie obiektami, które posiadają stan i zachowanie.

Mogę spokojnie dodawać do nich logikę, zmieniającą wewnętrzny stan, w sposób podobny jak pokazałeś w przykładzie.

Generalnie tak, wtedy kod będzie bardziej obiektowy - bo dane i operacje na nich umieścisz w jednym tworze - obiekcie.

Wykorzystanie takiej encji w widoku jest niedopuszczalne i zawsze muszę tworzyć dodatkowe viewmodele.

No ja zawsze tworzę viewmodele, z przyczyn wielokrotnie opisywanych - po co wysyłać do GUI danych, których ono nie potrzebuje.

Nie bardzo wiem czym są odłączane obiekty. Sposób, o który pytasz był w przykładzie, który przerabiałem. Uznałem, że tak należy robić.

ORM potrafi śledzić obiekty, które wczytał z bazy i sam wykrywać w nich zmiany. Obiekt odłączony, to obiekt, który nie pochodzi z ORMa (czyli nie został wczytany z bazy). Aby ORM go wykrywał, trzeba go najpierw "podłączyć" do kontekstu. Zdaje się, że służy do tego metoda Merge.
No, ale przy podejściu, które stosujesz, nie masz wyjścia, skoro Twój obiekt pochodzi z danych wpisanych przez użytkownika.

Rozwiązałem to w ten sposób, ponieważ przed usunięciem danego zadania, muszę sprawdzić czy istnieje. Pomyślę nad tym jak to inaczej rozwiązać.

Ale czy to ma jakiś sens? Przecież jeśli nie istnieje, to go na pewno nie usuniesz. EF pewno rzuci wyjątkiem, Ty możesz wtedy użytkownikowi pokazać jakiś komunikat, i to wszystko.

1 użytkowników online, w tym zalogowanych: 0, gości: 1