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ć?

[PHP] pobierz, plaintext 
interface ProductRepositoryInterface{
 
	/**
	 * Find all products
	 * 
	 * @return array
	 */
	public function findAll();
 
 
	/**
	 * Find specific product by id
	 *
	 * @param int id
	 * 
	 * @return array
	 */
	public function findById($id);
 
 
	/**
	 * Save single product 
	 *
	 * @param array item
	 * 
	 * @return bool
	 */
	public function save($item);
}
 
 
class JsonProductReposiotry implements ProductRepositoryInterface
{
	/**
	 * Path to json file, witch contains data
	 */
	private $file = 'database.json';
 
	/**
	 * Set file property
	 */
	function __construct($file = 'database.json') {
 
		if (!file_exists($file)) throw new Exception("Can not load {$file} file.");
 
		$this->file = $file;
	}
 
	/**
	 * Find all products
	 * 
	 * @return array
	 */
	public function findAll()
	{
		return json_decode(file_get_contents($this->file), true);
	}
 
	/**
	 * Find specific product by id
	 *
	 * @param int id
	 * 
	 * @return array
	 */
	public function findById($id)
	{
		$items = json_decode(file_get_contents($this->file), true);
 
		foreach ($items as $item) {
			if ($item['id'] == $id) return $item;
		}
	}
 
	/**
	 * Save single product 
	 *
	 * @param array item
	 * 
	 * @return bool
	 */
	public function save($item)
	{
		$items = json_decode(file_get_contents($this->file), true); //get all current stored products
	 	$this->items[] = $item; //append new product
 
		$result = file_put_contents($this->file, json_encode($this->items)); //save to file
 
		return ($result) ? true : false;
	}
 
}
 
class Product
{
	/**
	 * Products properties that can be set
	 */
	private $fillable = ['id', 'name', 'price'];
 
 
	/**
	 * Automatic sets properties
	 */
	public function __set($property, $value)
	{
		if (in_array($property, $this->fillable)) {
			$this->$property = $value;
		}
	}
 
}
 
 
class ProductsManager
{
	/**
	 * Sotres all producst obiented from respository
	 */
	private $products = [];
 
 
	/**
	 * Set ProductsManager dependencies
	 *
	 * @param ProductRepositoryInterface
	 */
	function __construct(ProductRepositoryInterface $repository) {
 
		$this->repository = $repository;
	}
 
 
	/**
	 * Add single Product instacne to products property
	 */
	public function addProduct(Product $item)
	{
		$this->products[] = $item;
	}
 
	/**
	 * Get all products from respository
	 *
	 * @return array
	 */
	public function findAll()
	{
		return $this->repository->findAll();
	}
 
	/**
	 * Save all products 
	 *
	 * @return all products
	 */
	public function saveAll()
	{
		foreach ($this->products as $product) {
 
			$this->repository->save([
				'id' => $product->id,
				'name'=> $product->name,
				'price' => $product->price
			]);
 
		}
 
		return $this->findAll();
	}
}
[PHP] pobierz, plaintext 

Poniżej proste zastosowanie tych klas

[PHP] pobierz, plaintext 
$manager = new ProductsManager(new JsonProductReposiotry('database.json'));
 
$dress        = new Product();
$dress->id    = '791';
$dress->name  = 'dress';
$dress->price = '150';
 
$socks        = new Product();
$socks->id    = '21';
$socks->name  = 'socks';
$socks->price = '15';
 
$manager->addProduct($dress);
$manager->addProduct($socks);
 
 
$manager->saveAll();
[PHP] pobierz, plaintext 

ł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ć:

[PHP] pobierz, plaintext 
$this->items = json_decode(file_get_contents($this->file), true);
[PHP] pobierz, plaintext 

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:

[PHP] pobierz, plaintext 
$this->items[] = $item; //append new product
$result = file_put_contents($this->file, json_encode($this->items)); //save to fi
[PHP] pobierz, plaintext 

literówka "Reposiotry"

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ść.

Dzięki wszystkim za uwagi

Rzeczywiście, masz rację, będę na to uważł w przyszłości.

Dzięki za radę, zastosuje się.


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ć?

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?

[PHP] pobierz, plaintext 
class Product
{
	private $id;
	private $name;
	private $price;
 
	/**
	 * Products properties that can be set
	 */
	private $setable = ['id', 'name', 'price'];
 
 
        /**
	 * Products properties that can be get
	 */
	private $getable = ['id', 'name', 'price'];
 
 
	/**
	 * Automatic sets properties
	 */
	public function __set($property, $value)
	{
		if (in_array($property, $this->setable)) {
			$this->$property = $value;
		}
 
 
	}
 
	/**
	 * Automatic get properties
	 */
	public function __get($property)
	{
		if (in_array($property, $this->getable)) {
			return $this->$property;
		}
	}
 
}
[PHP] pobierz, plaintext 

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.


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.

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.


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.


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.

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ę?


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?

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.

Okej, więc się zrozumieliśmy. Dzięki Destroyerr za pomoc