Ocena kodu OOP |
Ocena kodu OOP |
12.04.2014, 20:22:59
Post
#1
|
|
Grupa: Zarejestrowani Postów: 22 Pomógł: 0 Dołączył: 21.11.2012 Ostrzeżenie: (0%) |
Cześć, chciałbym prosić o ocenę mojego kodu.
Ćwiczyczę sobie programowanie obiektowe, staram się trzymać zasad SOLID, ale najlepiej będzie jeśli ktoś z zewnątrz oceni to co napisałem. Moje pytanie jest - czy jest to po prostu dobry kod? Czy jest on dobrze zaprojektowany(czy jest elastyczny)? Czy jest zgodny z zasadami SOLID? I najważniejsze - co można w nim poprawić?
Poniżej proste zastosowanie tych klas
Ten post edytował JacekJagiello 12.04.2014, 20:34:59 |
|
|
12.04.2014, 20:45:29
Post
#2
|
|
Grupa: Zarejestrowani Postów: 717 Pomógł: 120 Dołączył: 18.04.2009 Ostrzeżenie: (0%) |
Cytat $items = json_decode(file_get_contents($this->file), true); .... $items = json_decode(file_get_contents($this->file), true); .... $items = json_decode(file_get_contents($this->file), true); ... $items = json_decode(file_get_contents($this->file), true); łamiesz zasadę DRY (Don't repeat yourself), a co za tym idzie nie dość że niepotrzebnie przeklejasz ten sam kod, to w przypadku zmiany formy wczytywania danych (jeśli powiedzmy zapragniesz kiedyś umożliwić odczytywanie plików *.json wyciąganych z archiwum *.zip, to będziesz musiał w każdym takim miejscu dokleić kod odpowiedzialny za dekompresję zip), więc raczej powinieneś stworzyć dodatkową metodę "getItems()" , która by się zajmowała wczytywaniem danych. Po drugie to rozwiązanie jest niewydajne, bo za każdym razem odczytujesz z dysku dane, mógłbyś to jakoś zcacheować w pamięci (zrobić z $items, właściwość klasy, i przy pierwszym odczytaniu z dysku ją zainicjalizować: a przy zapisywaniu danych równolegle zapisywać je do $items (czyli w pamięci RAM) i na dysk (w celu trwałości zapisu). Chociaż zasadniczo to już tak chyba robisz:
Cytat JsonProductReposiotry literówka "Reposiotry" -------------------- |
|
|
12.04.2014, 20:48:49
Post
#3
|
|
Grupa: Zarejestrowani Postów: 879 Pomógł: 189 Dołączył: 14.06.2006 Skąd: Bytom Ostrzeżenie: (0%) |
Dla repozytorium stworzyłeś interfejs ale dla encji już nie? Nie masz interfejsu i dlatego opierasz tą encję o dziwną konstrukcję dynamicznie tworzonych pól danego obiektu (masz włączone raportowanie błędów?). Gdybyś to przynajmniej do tablicy wrzucał. Uważam, że trzeba to zrobić standardowo, dodać odpowiednie pola do klasy i dorobić im gettery i settery.
Powtarzanie kodu w repozytorium json, odczytywanie pliku i jego zapisywanie możesz wyrzucić do osobnych metod. Nazwa klasy ProductsManager jest sporna, ale moim zdaniem niewłaściwa. To właśnie klasa ProductsManager powinna się nazywać ProductsRepository, a Twoja klasa JsonProductReposiotry powinna się nazywać JsonProductsDAO. Rzucanie wyjątku \Exception też jest niewłaściwe, powinieneś doprecyzować co poszło nie tak poprzez użycie właściwego typu wyjątku. Może być to nawet Twój wyjątek. Przy trzech klasach na krzyż moim zdaniem ciężko jest ocenić elastyczność. |
|
|
12.04.2014, 22:11:21
Post
#4
|
|
Grupa: Zarejestrowani Postów: 22 Pomógł: 0 Dołączył: 21.11.2012 Ostrzeżenie: (0%) |
Dzięki wszystkim za uwagi
Cytat łamiesz zasadę DRY Rzeczywiście, masz rację, będę na to uważł w przyszłości. Cytat za każdym razem odczytujesz z dysku dane, mógłbyś to jakoś zcacheować w pamięci Dzięki za radę, zastosuje się. Cytat Dla repozytorium stworzyłeś interfejs ale dla encji już nie? Pisząc encję masz na myśli klasę Product? Nie zrobiłem dla tej klasy interfejsu, bo niewidzę jego zastosowania... mógłbyś przedstawić taki interfejs i powiedzieć dlaczego warto go wykorzystać? Cytat Nie masz interfejsu i dlatego opierasz tą encję o dziwną konstrukcję dynamicznie tworzonych pól danego obiektu Tą dziwną konstrukcję zrobiłem po to by nie pisać w przyszłości dla każdej właściwości klasy setera i getera... problem w tym, że zrobiłem to niewłaściwie Ale racja powinienem był stworzyć te właściwości. A co myślisz o takiej, poprawione konstrukcji?
Ta taka dziwna konstrukcja, to niezły pomysł których zauważyłem u niektórych programistów na Githubie. Tablice setable i getable, określają które właściwości klas mogą być ustawione lub które mogą być odczytane. W ten sposób nie trzeba pisać dużej ilości seterów i geterów. Cytat To właśnie klasa ProductsManager powinna się nazywać ProductsRepository, a Twoja klasa JsonProductReposiotry powinna się nazywać JsonProductsDAO. U mnie klasa Product z założenia jest modelem(czy też, jak wspomniałeś obiektem DAO). Repozytoria stworzyłem właśnie z myślą o elastyczności. Mogę mieć np. klasę JsonProductReposiotry, która zajmuję się pobieraniem danych z plików Json, ale mogę także stworzyć nową klasę np. DbProductRepository(która także będzie implementować ProductRepositoryInterface) która z koleji zajmuje się pobieraniem danych z MySQL. Teraz mam klasę ProductsManager, i może ona korzystać z dowolnej implementacji ProductRepositoryInterface, czyli z JsonProductReposiotry lub DbProductRepository. Dzięki temu łatwo mogę sobie zmenić sposób w jaki ProductsManager otrzymuje dane. Gdyby tak jak mówisz(albo tak jak ja zrozumiałem), JsonProductReposiotry był obiektem DAO, trudno było by zmienić sposób w jaki pobieram dane. ProductsManager, jest u mnie obiektem DAO, ale korzysta z repozytoriów po to by łatwo zmienić źródło pobierania daych. Ten post edytował JacekJagiello 12.04.2014, 22:15:20 |
|
|
13.04.2014, 06:35:02
Post
#5
|
|
Grupa: Zarejestrowani Postów: 879 Pomógł: 189 Dołączył: 14.06.2006 Skąd: Bytom Ostrzeżenie: (0%) |
Cytat Pisząc encję masz na myśli klasę Product? Nie zrobiłem dla tej klasy interfejsu, bo niewidzę jego zastosowania... mógłbyś przedstawić taki interfejs i powiedzieć dlaczego warto go wykorzystać? Wykorzystać go tak samo jak dla repozytorium. Istotniejsze dla mnie było to, żebyś nie używał magicznych metod tylko stworzył gettery i settery, co zostałoby wymuszone przez użycie interfejsu. Cytat U mnie klasa Product z założenia jest modelem(czy też, jak wspomniałeś obiektem DAO). Repozytoria stworzyłem właśnie z myślą o elastyczności. Mogę mieć np. klasę JsonProductReposiotry, która zajmuję się pobieraniem danych z plików Json, ale mogę także stworzyć nową klasę np. DbProductRepository(która także będzie implementować ProductRepositoryInterface) która z koleji zajmuje się pobieraniem danych z MySQL. Teraz mam klasę ProductsManager, i może ona korzystać z dowolnej implementacji ProductRepositoryInterface, czyli z JsonProductReposiotry lub DbProductRepository. Dzięki temu łatwo mogę sobie zmenić sposób w jaki ProductsManager otrzymuje dane. Gdyby tak jak mówisz(albo tak jak ja zrozumiałem), JsonProductReposiotry był obiektem DAO, trudno było by zmienić sposób w jaki pobieram dane. ProductsManager, jest u mnie obiektem DAO, ale korzysta z repozytoriów po to by łatwo zmienić źródło pobierania daych. Zadania przypisane warstwie DAO wykonuje u Ciebie właśnie repozytorium i to jest problemem. DAO powinno być jeszcze niżej w warstwach abstrakcji od repozytorium, dlatego napisałem, że masz to zamienione. Jeśli chcesz zmieniać sposób utrwalania to właśnie od tego jest Data Access Object i to ta warstwa wie jak utrwalane są dane czy w bazie danych czy w pliku. Moim zdaniem jesteś w błędzie i to klasa ProductsManager jest zbędna. Klasa Product nie jest modelem, jest tylko jego częścią. Model jest dużą i złożoną warstwą nie ma sensu spłaszczać go do jednej klasy. Cytat Gdyby tak jak mówisz(albo tak jak ja zrozumiałem), JsonProductReposiotry był obiektem DAO, trudno było by zmienić sposób w jaki pobieram dane. ProductsManager, jest u mnie obiektem DAO, ale korzysta z repozytoriów po to by łatwo zmienić źródło pobierania daych. ProductsManager nie jest u Ciebie DAO. Miałem na myśli, że kod zawarty w klasie JsonProductRepository powinien stanowić ciało klasy JsonProductsDAO. Kod z ProductsManager powinien stanowić klasę ProductsRepository. |
|
|
13.04.2014, 12:23:11
Post
#6
|
|
Grupa: Zarejestrowani Postów: 22 Pomógł: 0 Dołączył: 21.11.2012 Ostrzeżenie: (0%) |
Cytat Wykorzystać go tak samo jak dla repozytorium. Istotniejsze dla mnie było to, żebyś nie używał magicznych metod tylko stworzył gettery i settery, co zostałoby wymuszone przez użycie interfejsu. Rozumiem. Tylko... dla repozytoriów tworzę interfejs dlatego, że może być wiele rodzajów klas które implementują ten interfejs. Owe implementacje pobierają dane z różnych źródeł(JsonProductRepository, XmlProdcutRepository, DbProdcutRepository itd.). Mówisz żeby stworzyć interfejs dla Product, który zmuszał by klasę product do stworzenia seterów i getterów ale... skoro tylko klasa Product będzie miała takie metody jak setPrice() czy getPrice() to czy konieczny jest tu specjalny interfejs? Przecież żadna inna klasa nie będzie go implementowała... Chyba, że ma to związek z zasadą segregacji interfejsów. Ale w takim razie jak taki interfejs klasy Product realizuje tę zasadę? Cytat Zadania przypisane warstwie DAO wykonuje u Ciebie właśnie repozytorium i to jest problemem. DAO powinno być jeszcze niżej w warstwach abstrakcji od repozytorium, dlatego napisałem, że masz to zamienione. Jeśli chcesz zmieniać sposób utrwalania to właśnie od tego jest Data Access Object i to ta warstwa wie jak utrwalane są dane czy w bazie danych czy w pliku. Moim zdaniem jesteś w błędzie i to klasa ProductsManager jest zbędna. Klasa Product nie jest modelem, jest tylko jego częścią. Model jest dużą i złożoną warstwą nie ma sensu spłaszczać go do jednej klasy. Chyba powoli rozumiem o co Ci chodzi, i masz rację. U mnie klasa PorductManager wykorzystuje repozytorium JsonProductRepository() do wyszukiwania, zapisywania danych. Jednak to raczej repozytorium powinno dostarczać dane, bez pośredników(bez klasy PorductManager). Wytłumaczyłem sobie to odnosząc się do architektury MVC. W takiej architekturze, to repozytorium mogłoby(ale nie koniecznie) korzystać z modelu by pobrać dane(np. DbProductRepository, korzystało by z modelu Product który pobiera dane za pomocą np. ORM-a). A nie, tak jak to błędnie zaprojekowałem, że klasa Manager pośredniczy w całym procesie. Odnosząc się do MVC, to kontroler, a nie model używałby repozytorium. U mnie, gdyby kod był w architektórze MVC, wyglądało by to tak: Kontroler -> używa -> Model(DAO) -> używa -> Repozytorium Co jest złe. Tobie chodzi o to, by struktura wyglądała tak: Kontroler -> używa -> Repozytorium -> używa(jeśli jest potrzeba) -> Model(DAO) Wtedy rzeczywiście, tak jak mówisz, JsonProductRepository byłby odpowiedzialny za pobieranie z danych(byłby DAO). Mam nadzieję, że o coś podobnego Ci chodziło? |
|
|
18.04.2014, 13:26:31
Post
#7
|
|
Grupa: Zarejestrowani Postów: 879 Pomógł: 189 Dołączył: 14.06.2006 Skąd: Bytom Ostrzeżenie: (0%) |
Dokładnie to miałem na myśli.
Co do interfejsu dla encji to są różne podejścia do tego problemu. Moim głównym celem, kiedy o tym pisałem, było zwrócenie uwagi na (napiszę wprost) bezsensowny sposób implementacji encji. Encja (zresztą inne obiekty również) musi mieć jasno zdefiniowany interfejs (w znaczeniu API), można to wymusić poprzez interfejs (element składni). W PHP moim zdaniem służą do tego gettery i settery do chronionych właściwości, ewentualnie publiczne właściwości. Wykorzystanie do tego celu tablicy haszującej uważam za błąd. |
|
|
21.04.2014, 18:42:57
Post
#8
|
|
Grupa: Zarejestrowani Postów: 22 Pomógł: 0 Dołączył: 21.11.2012 Ostrzeżenie: (0%) |
Okej, więc się zrozumieliśmy. Dzięki Destroyerr za pomoc
|
|
|
Wersja Lo-Fi | Aktualny czas: 16.04.2024 - 13:59 |