Witaj Gościu! ( Zaloguj | Rejestruj )

Forum PHP.pl

 
Reply to this topicStart new topic
> Ocena kodu OOP
JacekJagiello
post 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ć?
  1. interface ProductRepositoryInterface{
  2.  
  3. /**
  4. * Find all products
  5. *
  6. * @return array
  7. */
  8. public function findAll();
  9.  
  10.  
  11. /**
  12. * Find specific product by id
  13. *
  14. * @param int id
  15. *
  16. * @return array
  17. */
  18. public function findById($id);
  19.  
  20.  
  21. /**
  22. * Save single product
  23. *
  24. * @param array item
  25. *
  26. * @return bool
  27. */
  28. public function save($item);
  29. }
  30.  
  31.  
  32. class JsonProductReposiotry implements ProductRepositoryInterface
  33. {
  34. /**
  35. * Path to json file, witch contains data
  36. */
  37. private $file = 'database.json';
  38.  
  39. /**
  40. * Set file property
  41. */
  42. function __construct($file = 'database.json') {
  43.  
  44. if (!file_exists($file)) throw new Exception("Can not load {$file} file.");
  45.  
  46. $this->file = $file;
  47. }
  48.  
  49. /**
  50. * Find all products
  51. *
  52. * @return array
  53. */
  54. public function findAll()
  55. {
  56. return json_decode(file_get_contents($this->file), true);
  57. }
  58.  
  59. /**
  60. * Find specific product by id
  61. *
  62. * @param int id
  63. *
  64. * @return array
  65. */
  66. public function findById($id)
  67. {
  68. $items = json_decode(file_get_contents($this->file), true);
  69.  
  70. foreach ($items as $item) {
  71. if ($item['id'] == $id) return $item;
  72. }
  73. }
  74.  
  75. /**
  76. * Save single product
  77. *
  78. * @param array item
  79. *
  80. * @return bool
  81. */
  82. public function save($item)
  83. {
  84. $items = json_decode(file_get_contents($this->file), true); //get all current stored products
  85. $this->items[] = $item; //append new product
  86.  
  87. $result = file_put_contents($this->file, json_encode($this->items)); //save to file
  88.  
  89. return ($result) ? true : false;
  90. }
  91.  
  92. }
  93.  
  94. class Product
  95. {
  96. /**
  97. * Products properties that can be set
  98. */
  99. private $fillable = ['id', 'name', 'price'];
  100.  
  101.  
  102. /**
  103. * Automatic sets properties
  104. */
  105. public function __set($property, $value)
  106. {
  107. if (in_array($property, $this->fillable)) {
  108. $this->$property = $value;
  109. }
  110. }
  111.  
  112. }
  113.  
  114.  
  115. class ProductsManager
  116. {
  117. /**
  118. * Sotres all producst obiented from respository
  119. */
  120. private $products = [];
  121.  
  122.  
  123. /**
  124. * Set ProductsManager dependencies
  125. *
  126. * @param ProductRepositoryInterface
  127. */
  128. function __construct(ProductRepositoryInterface $repository) {
  129.  
  130. $this->repository = $repository;
  131. }
  132.  
  133.  
  134. /**
  135. * Add single Product instacne to products property
  136. */
  137. public function addProduct(Product $item)
  138. {
  139. $this->products[] = $item;
  140. }
  141.  
  142. /**
  143. * Get all products from respository
  144. *
  145. * @return array
  146. */
  147. public function findAll()
  148. {
  149. return $this->repository->findAll();
  150. }
  151.  
  152. /**
  153. * Save all products
  154. *
  155. * @return all products
  156. */
  157. public function saveAll()
  158. {
  159. foreach ($this->products as $product) {
  160.  
  161. $this->repository->save([
  162. 'id' => $product->id,
  163. 'name'=> $product->name,
  164. 'price' => $product->price
  165. ]);
  166.  
  167. }
  168.  
  169. return $this->findAll();
  170. }
  171. }


Poniżej proste zastosowanie tych klas
  1. $manager = new ProductsManager(new JsonProductReposiotry('database.json'));
  2.  
  3. $dress = new Product();
  4. $dress->id = '791';
  5. $dress->name = 'dress';
  6. $dress->price = '150';
  7.  
  8. $socks = new Product();
  9. $socks->id = '21';
  10. $socks->name = 'socks';
  11. $socks->price = '15';
  12.  
  13. $manager->addProduct($dress);
  14. $manager->addProduct($socks);
  15.  
  16.  
  17. $manager->saveAll();


Ten post edytował JacekJagiello 12.04.2014, 20:34:59
Go to the top of the page
+Quote Post
PrinceOfPersia
post 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ć:
  1. $this->items = json_decode(file_get_contents($this->file), true);

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:
  1. $this->items[] = $item; //append new product
  2. $result = file_put_contents($this->file, json_encode($this->items)); //save to fi


Cytat
JsonProductReposiotry

literówka "Reposiotry"


--------------------
Go to the top of the page
+Quote Post
destroyerr
post 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ść.
Go to the top of the page
+Quote Post
JacekJagiello
post 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 smile.gif
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?
  1. class Product
  2. {
  3. private $id;
  4. private $name;
  5. private $price;
  6.  
  7. /**
  8. * Products properties that can be set
  9. */
  10. private $setable = ['id', 'name', 'price'];
  11.  
  12.  
  13. /**
  14. * Products properties that can be get
  15. */
  16. private $getable = ['id', 'name', 'price'];
  17.  
  18.  
  19. /**
  20. * Automatic sets properties
  21. */
  22. public function __set($property, $value)
  23. {
  24. if (in_array($property, $this->setable)) {
  25. $this->$property = $value;
  26. }
  27.  
  28.  
  29. }
  30.  
  31. /**
  32. * Automatic get properties
  33. */
  34. public function __get($property)
  35. {
  36. if (in_array($property, $this->getable)) {
  37. return $this->$property;
  38. }
  39. }
  40.  
  41. }

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
Go to the top of the page
+Quote Post
destroyerr
post 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.
Go to the top of the page
+Quote Post
JacekJagiello
post 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?
Go to the top of the page
+Quote Post
destroyerr
post 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.
Go to the top of the page
+Quote Post
JacekJagiello
post 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 smile.gif
Go to the top of the page
+Quote Post

Reply to this topicStart new topic
1 Użytkowników czyta ten temat (1 Gości i 0 Anonimowych użytkowników)
0 Zarejestrowanych:

 



RSS Wersja Lo-Fi Aktualny czas: 28.03.2024 - 14:23