Ocena podejścia do Dependency Injection, PDO + OOP + Dependency injection Opcje

[marcinq123]

Witam, uczę się OOP, zacząłem dla testów pisać klasę obsługująca bazę na podstawie PDO ( Trudno to nazwać DIC ). I proszę Was o ocenę czy w dobrą stronę to zmierza czy może podejście mam złe i później sobie skomplikuje życie ?

"Klasa DIC"

[PHP] pobierz, plaintext 
class DataBaseConnection
{
	protected $pdo;
	protected $mysqlHost = 'Host';
	protected $mysqlLogin = 'Login';
	protected $mysqlPassword = 'Haslo do bazy';
	protected $mysqlDatabase = 'Baza';
	public function __construct()
	{
 
				try
	{
		$this->pdo = new PDO('mysql:dbname='.$this->mysqlDatabase.';host='.$this->mysqlHost.';', $this->mysqlLogin, $this->mysqlPassword);
		$this->pdo->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);
 
	}
	catch(PDOException $exception) 
	{
		echo $exception->getMessage();
	}
	}
 
	public function prepare($sql)
	{
		return $this->pdo->prepare($sql);
	}
 
}
[PHP] pobierz, plaintext 

Pomijając kwestię iż trzymam dane do połączenia w klasie, czy ma mniej więcej tak to wyglądać ? Ciekawi mnie dlaczego musiałem zdefiniować funkcję prepare ... jak jej nie dopiszę to wywala mi później że nie istnieje ;x

Klasa User:

[PHP] pobierz, plaintext 
class User
{	
	private $login;
	private $query;
	public function __construct($baza)
	{
 
		$this->pdo = $baza;
 
	}
 
	public function selectUser($user)
	{
		$this->login = $user;
 
		$this->query = $this->pdo->prepare('SELECT * FROM users WHERE login =:login');
		$this->query->bindParam(':login', $this->login, PDO::PARAM_STR);
		$this->query->execute();
 
		if($this->query->rowCount() > 0)
		{
 
		$row=$this->query->fetch();
		$this->nazwa = $row['data'];
		}else{echo ' dupa ';}
	}
 
 
 
	public function getUserData()
	{
 
		return $this->nazwa;
 
	}
 
} 
[PHP] pobierz, plaintext 

Kod napisany tylko żeby sprawdzić czy się uruchamia.

A wywołuje metody tak:

[PHP] pobierz, plaintext 
$user = new User(new DataBaseConnection());
 
$user->selectUser('jakisUser');
echo $user->getUserData();
[PHP] pobierz, plaintext 

Ogólnie działa, no chyba że skasuje metode prepare w klasie DataBaseConnection to wtedy już nie ;p Tylko pytanie jak już wcześniej napisałem czy dobrą drogą idę ?

[Turson]

Pokaż var_dump($baza) w konstruktorze klasy User. Siłą rzeczy nie powinieneś mieć dostępu do własności $pdo klasy DataBaseConnection bo jest ona chroniona

[Damonsson]

Takie trochę na siłę to DI. Do każdego obiektu będziesz musiał przekazywać to połączenie, wg mnie bez sensu. A to raczej sam model sobie powinien dziedziczyć po DataBaseConnection i tyle.

[marcinq123]

Pokaż var_dump($baza) w konstruktorze klasy User. Siłą rzeczy nie powinieneś mieć dostępu do własności $pdo klasy DataBaseConnection bo jest ona chroniona

[PHP] pobierz, plaintext 
object(DataBaseConnection)#2 (5) { ["pdo":protected]=> object(PDO)#3 (0) { } ["mysqlHost":protected]=> string(13) "Host" ["mysqlLogin":protected]=> string(8) "Login" ["mysqlPassword":protected]=> string(16) "Haslo do bazy" ["mysqlDatabase":protected]=> string(8) "Baza" }
[PHP] pobierz, plaintext 

Takie trochę na siłę to DI. Do każdego obiektu będziesz musiał przekazywać to połączenie, wg mnie bez sensu. A to raczej sam model sobie powinien dziedziczyć po DataBaseConnection i tyle.

Chciałem sobie uprościć... W tym przypadku extend rozumiem. Ale dajmy na to, chciałbym stworzyć klasę rejestracyjną, logowanie, obsługa profilu klienta, panel admina ... etc. Miałbym je po prostu dziedziczyć po klasie obsługującej bazę ?
Bo chyba nie Sigleton'em miałbym to rozwiązać ?

Broń Boże się nie wymądrzam, po prostu nie jestem pewien jak najlepiej rozwiązać operacje na bazie ... Albo inaczej, jak najszybciej załapać.

Ten post edytował marcinq123 5.01.2015, 22:01:49

[Turson]

Właśnie Singleton się do tego nadaje.

[marcinq123]

No właśnie, ogólnie sobie poczytałem ... i w sumie zdania są podzielone ... sporo osób odradza singletona ... albo odradza używania źle napisanego singletona ;p
Pomyślałem nad właśnie DI ...

[Damonsson]

Jeżeli to jest po prostu zwykły model, to musisz mieć połączenie z bazą, więc po wymuszać przekazywanie czegoś, bez czego jak nie przekażesz to nie będzie działać? A skoro musisz to przekazać to takie użycie DI, jest wg mnie średnim pomysłem.
Dokładnie, miałbyś je po prostu dziedziczyć, użyć Singleton i tyle. Czasami ludzie próbują na siłę doszukiwać się problemów, z tymi wadami Singleton.

Ten post edytował Damonsson 5.01.2015, 22:23:45

[Crozin]

Z Twoim kodem wszystko jest źle i nie ma to wiele wspólnego z Dependency Injection - niestety podobnie jest z późniejszymi postami w tym wątku.

1. Po co istnieje klasa DataBaseConnection skoro nie robi ona absolutnie nic poza obcięciem możliwości "surowego" PDO? Przecież możesz bezpośrednio korzystać z obiektu PDO w swoim kodzie - nie musisz tego robić przez obiekt pośredniczący.
2. Jak już, to konstruktor klasy DataBaseConnection powinien przyjąć obiekt PDO w swoim argumencie, a nie tworzyć go samemu - wtedy mielibyśmy do czynienia właśnie z DI.
3. Jeżeli nie jesteś wstanie zrobić nic z wyjątkiem, to go nie wyłapuj.
4. Musiałeś zdefiniować dla klasy DataBaseConnection metodę prepare bo dlaczego niby miałaby ona istnieć sama z siebie? Obiekt PDO udostępnia prepare(), query(), beginTransaction() i masę innych, nie DataBaseConnection.
5. W klasie User już poprawnie przekazałeś obiekt połączenia z zewnątrz. Jedynie niepotrzebnie na siłę wykorzystujesz właściwości login czy query bo tam możesz skorzystać ze zwykłych zmiennych lokalnych.

Czyli generalnie powinno to wyglądać to mniej-więcej tak:

[PHP] pobierz, plaintext 
// Klasa DataBaseConnection w ogóle do kosza.
 
class User
{
    private $pdo;
 
    private $name;
 
    public function __construct(PDO $pdo)
    {
        $this->pdo = $pdo;
    }
 
    public function select($login)
    {
        $stmt = $this->pdo->prepare('SELECT ... ');
        $stmt->bind(..., $login);
        $stmt->execute();
 
        if ($stmt->rowCount() === 0) {
            throw new ...Exception('...');
        }
 
        $data = $stmt->fetch();
 
        $this->name = $data['name'];
    }
 
    public function getName()
    {
        return $this->name;
    }
}
[PHP] pobierz, plaintext 

Pomijam tutaj już fakt, że wybieranie danych z bazy i ich reprezentacja w formie jednego obiektu klasy User jest bardzo złym podejściem. Powinieneś mieć raczej jeden obiekt przeznaczony do reprezentowania użytkownika (User) i drugi do wybierania takowych np. z bazy danych, czyli tzw. "model" (UserRepository).

Jeszcze co do bzdur wypisanych w postach:
1. W żadnym wypadku klasa typu user nie powinna dziedziczyć po połączeniu z bazą danych, bo: (a) nie jest takowym, ( dojdzie do tego że będziesz miał otwartych po kilka połączeń na raz - kompletnie niepotrzebnie.
2. Jeżeli w jakiejś klasie potrzebujesz mieć dostęp do bazy danych to obiekt PDO powinieneś właśnie przekazać w konstruktorze.
3. Użycie Singetonu dla "klasy od bazy danych" to idiotyzm: (a) przecież może istnieć wiele równoległych połączeń do różnych baz danych(!), ( niweczysz tym wszelkie zalety płynące z IoC (DI)
4. Model nie jest synonimem operacji na bazie danych. Ta ostatnia jest w ogóle nie potrzebna. Przecież dane mogą pochodzić z dziesiątek innych źródeł, a tak w ogóle to model nie oznacza tylko wybierania danych, ale również i ich przetwarzanie.

[pyro]

@Crozin, +1 . Miałem to samo wymienić, ale nie chciało mi się tyle pisać

[marcinq123]

Prawdę mówiąc, po cichu liczyłem na Twoją odpowiedź Crozin ;]

Z racji tego iż chcę nabrać dobrych nawyków, spróbuje podejść kompleksowo.

4. Musiałeś zdefiniować dla klasy DataBaseConnection metodę prepare bo dlaczego niby miałaby ona istnieć sama z siebie? Obiekt PDO udostępnia prepare(), query(), beginTransaction() i masę innych, nie DataBaseConnection.

No właśnie zdefiniowałem tylko prepare() a np. execute(), bindParam() już nie musiałem bo działało ... i dlatego nie rozumiem czemu.

Pomijam tutaj już fakt, że wybieranie danych z bazy i ich reprezentacja w formie jednego obiektu klasy User jest bardzo złym podejściem. Powinieneś mieć raczej jeden obiekt przeznaczony do reprezentowania użytkownika (User) i drugi do wybierania takowych np. z bazy danych, czyli tzw. "model" (UserRepository).

Cenna uwaga, ale napisałem przykład z pierwszego postu tylko żeby sprawdzić czy w ogóle to działa.

[PHP] pobierz, plaintext 
// Klasa DataBaseConnection w ogóle do kosza.
 
class User
{
    private $pdo;
 
    private $name;
 
    public function __construct(PDO $pdo)
    {
        $this->pdo = $pdo;
    }
 
    public function select($login)
    {
        $stmt = $this->pdo->prepare('SELECT ... ');
        $stmt->bind(..., $login);
        $stmt->execute();
 
        if ($stmt->rowCount() === 0) {
            throw new ...Exception('...');
        }
 
        $data = $stmt->fetch();
 
        $this->name = $data['name'];
    }
 
    public function getName()
    {
        return $this->name;
    }
 
//oraz inne metody
}
[PHP] pobierz, plaintext 

Czyli teraz odwołuje się tak ?:

[PHP] pobierz, plaintext 
$user = new User(new PDO('mysql:dbname=base;host=host;', 'user', 'password'));
$user->select('user');
echo $user->getName();
[PHP] pobierz, plaintext 

Moje pytanie brzmi, dajmy na to miałbym klasy User ... Admin, Auth, etc. ( nie wiem co tam jeszcze wymyśle )
Każdy obiekt generuje ( w razie potrzeby połączenia z bazą) poprzez

[PHP] pobierz, plaintext 
$admin= new Admin(new PDO('mysql:dbname=base;host=host;', 'user', 'password'));
//jakieś wywoływane metody
[PHP] pobierz, plaintext 

A czy takie podejście żeby się nie powtarzać ma sens?

[PHP] pobierz, plaintext 
$connection = new PDO('mysql:dbname=base;host=host;', 'user', 'password'));
$admin= new Admin($connection);
[PHP] pobierz, plaintext 

A do reszty się nie ustosunkuje bo ze wstydu nie wiem nawet co powiedzieć

[Crozin]

No właśnie zdefiniowałem tylko prepare() a np. execute(), bindParam() już nie musiałem bo działało ... i dlatego nie rozumiem czemu.

Twoja metoda DataBaseConnection::prepare() zwraca na dobrą sprawę to co zwraca PDO::prepare() czyli nowy obiekt, klasy PDOStatement - to on zawiera metody execute(), bindParam() i inne. Nie ma on już kompletnie nic wspólnego z Twoim kodem.

A czy takie podejście żeby się nie powtarzać ma sens?

Tak, ma to sens - tak właśnie powinno to wyglądać. Bo tutaj nie chodzi nawet o powtarzanie się, a o jednokrotne nawiązanie połączenia i wykorzystywanie go w wielu różnych miejscach, czyli:

[PHP] pobierz, plaintext 
$conn = new PDO(....);
 
$user = new User($conn);
$sth = new Sth($conn);
$userSth = new UserSth($conn, $user);
// itp.
[PHP] pobierz, plaintext 

[marcinq123]

Na tą chwile zostały rozwiane moje wątpliwości ;]

Dziękuje wielkie za pomoc panowie.

[Damonsson]

1. W żadnym wypadku klasa typu user nie powinna dziedziczyć po połączeniu z bazą danych, bo: (a) nie jest takowym, ( dojdzie do tego że będziesz miał otwartych po kilka połączeń na raz - kompletnie niepotrzebnie.
2. Jeżeli w jakiejś klasie potrzebujesz mieć dostęp do bazy danych to obiekt PDO powinieneś właśnie przekazać w konstruktorze.
3. Użycie Singetonu dla "klasy od bazy danych" to idiotyzm: (a) przecież może istnieć wiele równoległych połączeń do różnych baz danych(!), ( niweczysz tym wszelkie zalety płynące z IoC (DI)
4. Model nie jest synonimem operacji na bazie danych. Ta ostatnia jest w ogóle nie potrzebna. Przecież dane mogą pochodzić z dziesiątek innych źródeł, a tak w ogóle to model nie oznacza tylko wybierania danych, ale również i ich przetwarzanie.

1. Jeżeli to jest zwykły model do połączenia z bazą danych to jest takowym i w każdym obiekcie i tak będzie się łączył z bazą danych i duplikował przekazywanie połączenia w konstruktorze przecież. Po to ma użyć Singleton, żeby nie miał kilku otwartych połączeń.
2. Jasne.
3. Tak sobie przeglądam klasy Doctrine zawierające własnie Singleton np w getInstance i nie widzę idiotyzmu.
4. Jasne, jeżeli zakładamy, że to ma być uniwersalny model, zgadzam się. Tutaj było pytanie o PDO.

Nie piszę po złości, masz wiedzę większą ode mnie i chętnie poczytam, dlaczego się mylę.

Choć te różnice nie są duże, głównie chodzi o to, że moje założenie jest takie, że zawsze będzie się łączył z tą bazą danych, więc może dziedziczyć, a Ty dajesz większą uniwersalność, w zamian za duplikowanie kodu (przekazywanie połączenia za każdy razem, przy tworzeniu obiektu). Pytanie, co jest poprawniejsze?

[Crozin]

1. Jeżeli to jest zwykły model do połączenia z bazą danych to jest takowym i w każdym obiekcie i tak będzie się łączył z bazą danych i duplikował przekazywanie połączenia w konstruktorze przecież. Po to ma użyć Singleton, żeby nie miał kilku otwartych połączeń.

Co to jest "zwykły model"? Jeżeli przekazuje za każdym razem to samo połączenie to nie nawiąże mu się nagle kilka.

3. Tak sobie przeglądam klasy Doctrine zawierające własnie Singleton np w getInstance i nie widzę idiotyzmu.

A jakoś dokładniej? Bo na pewno nie jest to obiekt połączenia z bazą danych.

[...] w zamian za duplikowanie kodu (przekazywanie połączenia za każdy razem, przy tworzeniu obiektu)

Ale tu nie ma żadnego duplikowania kodu.

[Damonsson]

1. Metody, które i tak zawsze pobierają coś z bazy danych, nie wyobrażam tam sobie nagle czegoś niezwiązanego z bazą danych. Wtedy trzeba by to rozdzielić. No tak, a jeżeli będę dziedziczył (wg mojej teorii), to muszę sprawdzić czy już nie istnieje połączenie dlatego Singleton.

2. Dalej w związku z tym jest tworzenie połączenia itd. ale właśnie samo getInstance to zwykły Singleton.

[PHP] pobierz, plaintext 
/**
     * Returns an instance of this class
     * (this class uses the singleton pattern)
     *
     * @return Doctrine_Manager
     */
    public static function getInstance()
    {
        if ( ! isset(self::$_instance)) {
            self::$_instance = new self();
        }
        return self::$_instance;
    }
[PHP] pobierz, plaintext 

3. Za każdym razem musisz się martwić, żeby przekazać to połączenie do obiektu. Tworząc odpowiednio skonkretyzowany model, wiem, że będzie on korzystał z bazy danych i w konstruktorze, tworzę sobie to połączenie od razu. Tworząc obiekt nie muszę pamiętać, aby przekazać tam połączenie z bazą.

[Crozin]

Doctrine 1? Ale mamy 2015 rok, środowisko PHP już znormalniało...
O niczym nie musisz pamiętać, IDE/PHP samo Ci podpowie, że robisz coś nieprawidłowego.