Keskustelu: Nettisivujen teko: Olio-ohjelmointi: luokan idea

Jälleen on uuden mietiskelyn (kysymyksen) paikka. Olen miettinyt, että miten luokka pitäisi koostaa / miten sitä pitäisi käyttää. Eli, onko parempi tapa, että luokka on vain luokka, jossa on tietyn asian tekeviä, ei-staattisia metodeja, vai luokka, joka tavallaan on se asia, jota käsitellään, ja jos tarvitsee esim. hakea tietyn käyttäjän viestit, sen tekevät metodit olisivat staattisia?
Tässä esimerkit:

// Ensimmäinen tapa, luokassa on vain ei-staattisia metodeja tietyntyyppisten asioiden tekemiseen
class Post extends PDO {
	function findAll($query, $args){
		$q = $this->prepare($query);
		$stmt = $q->execute($args);

		$arr = array();
		while ($s = $stmt->fetchObject()){
			$arr[] = $s;
		}

		return $arr;
	}

	function create($content, $writer_id){
		// tallenna viesti tietokantaan
	}
}

// Toinen tapa, luokka käyttäytyy kuin viesti, metodit, joilla haetaan viestit,
// ovat staattisia
class Post extends PDO {
	function __construct(array $post){
		$this->content = (isset($post["content"])) ? $post["content"] : Null;
		$this->writer_id = (isset($post["writer_id"])) ? $post["writer_id"] : Null;

		parent::__construct(/* Connection string */);
	}

	static function findAll($query, $args){
		$pdo = new PDO(/* Jne */);
		$q = $pdo->prepare($query);
		$stmt = $q->execute($args);

		$arr = array();
		while ($s = $stmt->fetchObject()){
			$arr[] = $s;
		}

		return $arr;
	}

	function save(){
		$q = $this->prepare("INSERT INTO post (content, writer_id) VALUES (?, ?)");
		$q->execute(array($this->content, $this->writer_id));
	}
}

Ennen olen tehnyt kaiken aina ensimmäisellä tavalla, mutta kun katsoin koodivinkkiä salasanojen käsittelystä, aloin miettiä, onko oma tapani sittenkään niin hyvä. Mielestäni koodivinkissä esitelty tapa on enemmän olio-ohjelmoinnin ominaisuuksia hyödyntävä kuin oma tapani.

Ottamatta kantaa varsinaiseen kysymykseen, en ainakaan jatkaisi suoraan PDO luokkaa, luo mielummin PDO wrapperi joka palauttaa singleton tyyppisesti PDO objektin käytettäväksi.

qeijo kirjoitti:

luo mielummin PDO wrapperi joka palauttaa singleton tyyppisesti PDO objektin käytettäväksi.

Tuo on erittäin hyvä idea. Tämä koodini on hieman yksinkertaistettu versio oikeasta koodistani, oikeasti periytän Post-luokan Db-luokasta (jossa on kyselyä helpottavia metodeja), joka kylläkin periytyy PDO:sta. Olisiko järkevää periyttää tämä Post-luokka Db-luokasta ja sitten Db-luokkaa ei periyttäisikään PDO:sta vaan laittaisi sen PDO-wrapperin palauttaman objektin sinne jäsenmuuttujaksi, vai niin, että Db-luokka olisi se wrapperi (kyselyä helpottavien metodiensa kanssa), koska en halua käyttää esimerkiksi Post-luokassa suoraan PDO:n metodeja vaan omia välimetodejani (tyyliin Db->findAll() ja Db->insert(array('content' => "Moro")) )ja sitten Db:n instanssi laitettaisiin Post-luokalle jäsenmuuttujaksi?

On täysin selvää, että ensimmäinen tapasi on ihan kummallinen. Olion pitää kuvata yhtä jonkinlaista yksilöä, ja kaikkien ei-staattisten jäsenten pitää olla kuuri kyseiseen olioon liittyviä. Jos metodi käsittelee useaa oliota (kuten esimerkkisi findAll), sen kuuluu yleensä joko olla staattinen (Post::findAll) tai sijaita jossain muussa luokassa (esim. $tietokanta->findAllPosts, huono esimerkki mutten jaksa nyt keksiä muutakaan).

Myös toinen koodisi on väärin, koska luokan periyttäminen tietokantayhteydestä ei ole järkevää. Silloinhan jokainen ohjelmasi postaus on myös erillinen tietokantayhteys eli ohjelmassasi on hirveä määrä tietokantayhteyksiä. Myös ideologisella tasolla periytyminen tarkoittaa, että uusi luokka sisältää myös koko vanhan luokan (eli uuden luokan olio on myös vanhan luokan olio); siksi periytymistä pitää käyttää vain silloin, kun luodaan luokasta jokin erikoisempi versio (vaikka Tietokanta -> MySQLTietokanta). Jos Post on myös PDO, on mahdollista kirjoittaa vaikka $post->prepare("kysely"), jossa ei ole mitään järkeä. Tietokanta pitää antaa parametrina, esim. Post::lataa($kanta, $id) tai $post->tallenna($kanta).

Ajatellaanpa, että luokka on Ihminen, olioita ovat Pekka ja Matti ja funktio on LaskeMaailmanIhmiset. Nyt jos funktio on staattinen, se voi laskea kaikki maailman ihmiset eikä olioita tarvita. Jos funktio ei ole staattinen, sen toiminnan pitää olla jotenkin laskijasta riippuvainen: ehkä Pekka ei osaa laskea kovin hyvin tai Matti ei laske poliitikkoja ihmisiksi.

Tiivistetysti voi sanoa, että jos funktio ei sisällä $this-sanaa, sen pitää lähes poikkeuksetta olla staattinen.

(Edit: Haukuin myös toisen luokan.)

Metabolix kirjoitti:

On täysin selvää, että ensimmäinen tapasi on ihan kummallinen.

Sitähän minäkin, ja siksi sitä aloinkin miettiä. Ja kiitos selvästä selvennyksestä asiaan. Mutta kuten sanoin, ennen olen tehnyt kaiken ensimmäisellä tavallani, ennen kuin aloin miettiä, onko siinä järkeä.

Mitä mieltä olet edellisessä viestissäni esittämästäni kysymyksestä? Voi olla, että termit ovat vielä vähän hakusessa, en ole olio-ohjelmointia kovin paljoa harrastanut, mutta korjatkaa, jos löydätte vääriä termejä tai ilmaisutapoja.

Niin, siis tämähän riippuu myös rakennettavasta järjestelmästä ja sen muista tarpeista ja riippuvuuksista. Yksi vaihtoehto on antaa PDO objekti datamapper - tyyppiselle luokalle joka loisi (ylläpitäisi) ja palauttaisi Post olioita. Tietty ideaali olisi jos mapper - luokka ei olisi ollenkaan riippuvainen juuri PDO:sta, vaan sille annettaisi jokin abstraktimpi toteutus.

Edit: pientä korjailua..

<?php

/**
 * Ei testattu esimerkki
 */
class PostMapper {

    private $db;

    public function __construct(PDO $db) {

        $this->db = $db;
    }

    public function fetchAll($status = 1) {

        $query = $this->db->prepare('SELECT * FROM posts WHERE status = ?');
        $query->execute(array($status));

        $result = $query->fetchAll();

        $posts = array();

        foreach ($result as $row) {

            $post = new Post();

            $post->setId($row['id'])
                    ->setPost($row['post'])
                    ->setSubject($row['subject'])
                    ->setDatetime($row['datetime']);

            $posts[] = $post;
        }

        return $posts;
    }

    public function fetchRow($id) {

        $query = $this->db->prepare('SELECT * FROM posts WHERE id = ?');
        $query->execute(array($id));

        $row = $query->fetch();

        $post = new Post();

        $post->setId($row['id'])
                ->setPost($row['post'])
                ->setSubject($row['subject'])
                ->setDatetime($row['datetime']);

        return $post;
    }

    public function insert(Post $post) {}

    public function update(Post $post) {}

    public function delete(Post $post) {}
}

class Post {

    private $id, $datetime, $subject, $post;

    public function __get($name) {

        return $this->$name;
    }

    public function setId($value) {

        $this->id = $value;
        return $this;
    }

    public function setDatetime($value) {

        $this->datetime = $value;
        return $this;
    }

    public function setSubject($value) {

        $this->subject = $value;
        return $this;
    }

    public function setPost($value) {

        $this->post = $value;
        return $this;
    }
}

try {

     $postMapper = new PostMapper(DbPdo::getInstance());

     $posts = $postMapper->fetchAll();

     foreach($posts as $post) {

         print $post->id; //tjn.
     }

} catch(PDOException $e) {

     print $e->getMessage();
}

Ripe kirjoitti:

qeijo kirjoitti:

luo mielummin PDO wrapperi joka palauttaa singleton tyyppisesti PDO objektin käytettäväksi.

Tuo on erittäin hyvä idea.

Ei se ole erittäin hyvä idea, vaan korkeintaan kohtuullinen, mutta silti huomattavasti parempi kuin nykyinen tapasi periä Post-luokka PDO:sta (tai Db:sta). Singletonien lähtökohtaiselle huonoudelle löytyy paljon hyviä perusteluja mm. ne piilottavat riippuvuudet ja hankaloittavat yksikkötestaamista (tai tekevät se jopa mahdottomaksi).

Ripe kirjoitti:

Tämä koodini on hieman yksinkertaistettu versio oikeasta koodistani, oikeasti periytän Post-luokan Db-luokasta (jossa on kyselyä helpottavia metodeja), joka kylläkin periytyy PDO:sta. Olisiko järkevää periyttää tämä Post-luokka Db-luokasta ja sitten Db-luokkaa ei periyttäisikään PDO:sta vaan laittaisi sen PDO-wrapperin palauttaman objektin sinne jäsenmuuttujaksi, vai niin, että Db-luokka olisi se wrapperi (kyselyä helpottavien metodiensa kanssa), koska en halua käyttää esimerkiksi Post-luokassa suoraan PDO:n metodeja vaan omia välimetodejani (tyyliin Db->findAll() ja Db->insert(array('content' => "Moro")) )ja sitten Db:n instanssi laitettaisiin Post-luokalle jäsenmuuttujaksi?

Post on selkeästi tässä model-luokka eikä sen tulisi periytyä PDO:sta, Db:stä tai mistään muustakaan mikä kuva ajollain tapaa tietokantayhteyttä tms. koska silloin rikottaisiin SoC-periaatetta. Pidä postit posteina ja tietokantayhteydet tietokantayhteyksinä. Jos jossain Post-luokan metodissa tarvitset tietokantaa, anna se sinne parametrina tai laita konstruktorissa jäsenmuuttujaksi kuten itse ehdotit.

Kiitos oikein paljon kaikille, jotka joko haukuitte koodiani, lähestymistapaani tai annoitte parempia vaihtoehtoja (Tuo ei ole sarkasmia, vaan olen teille oikeasti kiitollinen). Kuten olen sanonut, olen php:n kanssa aloittelija, ja vielä enemmän sitä olen olio-ohjelmoinnin kanssa. Jos ette olisi haukkuneet koodiani, jatkaisin edelleen vanhan, huonon koodin kirjoittamista ja vanhoja koodaustyyliäni ja ajatustapaani. Täytyy sitten pistää lähes kaikki koodi uusiksi, mutta tällä kertaa siitä tulee parempaa, loogisempaa ja järkevämpää. Ja tuo qeijon koodi näyttää mainiolta pohjalta/esimerkiltä jatkoa varten.

qeijo kirjoitti:

Tietty ideaali olisi jos mapper - luokka ei olisi ollenkaan riippuvainen juuri PDO:sta, vaan sille annettaisi jokin abstrakti toteutus.

Olisiko tämän tyylinen Db-luokka, joka periytyy PDO:sta, edes jonkinlainen abstrakti toteutus?

class Db extends PDO{
	function findAll(array $query = NULL, array $columns = NULL){
		$str = "SELECT ";

		if ($columns === NULL){
			$str .= "* ";
		} else {
			$str .= implode(",", $columns);
			$str .= " ";
		}

		$str .= "FROM {$this->table} ";

		if (isset($query)){
			$str .= "WHERE ";

			$arr = array();
			$values = array();

			foreach ($query as $key => $value){
				$arr[] = $key . "=?";
				$values[] = $value;
			}

			if (count($query) > 1){
				$str .= implode(" AND ", $arr);
			} else {
				$str .= $arr[0];
			}
		}

		$q = $this->prepare($str);
		if (!empty($values)){
			$stmt = $q->execute($values);
		} else {
			$stmt = $q->execute();
		}

		$arr = array();
		if (is_object($stmt)){
			while ($s = $stmt->fetchObject()){
				$arr[] = $s;
			}

			return $arr;
		}

		return array();
	}
}

Vielä kerran, olen erittäin kiitollinen kaikille, jotka auttoivat tässä.

Ripe kirjoitti:

qeijo kirjoitti:

Tietty ideaali olisi jos mapper - luokka ei olisi ollenkaan riippuvainen juuri PDO:sta, vaan sille annettaisi jokin abstrakti toteutus.

Olisiko tämän tyylinen Db-luokka, joka periytyy PDO:sta, edes jonkinlainen abstrakti toteutus:

Ei. Koska Db-luokkasi periytyy PDO:sta, se ei ole yhtään sen enempää abstrakti tai vähemmän riippuvainen PDO:sta kuin PDO itse. PDO on jo itsessään tietokantaabstraktiototeutus eli se mahdollistaa useiden erilaisten tietokantatoteutusten käyttämisen saman rajapinnan kautta. Koska tuon PostMapper-luokan tarkoituksena on nimenomaan hakea ja poistaa Post-olioita tietokannasta, niin minusta on ihan ok, ja jopa välttämätöntä, että se riippuu tietokannasta eli tässä tapauksessa PDO:sta, kunhan riippuvuus toteutetaan läpinäkyvästi, niinkuin tässä on tehty. (eli niin että PDO annetaan konstruktoriparametrina eikä esim niin että konstruktorissa olisi jotain tällaista $this->db = DbPdo::getInstance();

Jos haluat lisätä omia ominaisuuksiasi PDO:hon niin tuollainen perivä DB-luokka on ihan ok. Näissä esimerkeissä tehdyt lisäykset näyttävät minusta kuitenkin menevä jo aika paljon sivuun siitä mikä on PDO:n varsinainen tehtävä, joten itse en ehkä käyttäisi perintää vaan tekisin lisäykset omiin erilliisiin luokkiinsa, jotka sitten antaisin riippuvuuksina tuolle mapperille. Itseasiassa tässä alkaa jo vaikuttamaan siltä että ollaan keksimässä pyörää (ORM:ia) uudelleen. Kannattaisi varmaan vilkaista valmiita toteutuksia olisiko niistä iloa (esim. Propel ja Doctrine).

Okei, en siis lähde laajentamaan PDO:ta Db-luokalla. Kiitos tuostakin selvennyksestä.

Nimimerkki kirjoitti:

PDO on jo itsessään tietokantaabstraktiototeutus eli se mahdollistaa useiden erilaisten tietokantatoteutusten käyttämisen saman rajapinnan kautta. Koska tuon PostMapper-luokan tarkoituksena on nimenomaan hakea ja poistaa Post-olioita tietokannasta, niin minusta on ihan ok, ja jopa välttämätöntä, että se riippuu tietokannasta eli tässä tapauksessa PDO:sta

Olisihan se hyvä jos PostMapper luokan ei tarvitsisi välittää tiedon alkuperästä. Toki hieman yliampuvaa tässä tapauksessa...

Jos on tarkoitus tehdä kaikki kyselyt hienomman rajapinnan kautta, PDO-olio pitää laittaa Db:n jäseneksi, jotta Db:stä voi tarvittaessa tehdä myös muunlaisen toteutuksen eikä tule vahingossakaan käytettyä PDO:n funktioita muualla koodissa. (Tällöin kylläkin Db:n pitäisi olla rajapinta ja toteutuksen pitäisi olla vaikka luokassa DbPDO.) (Esimerkiksi C++ toimii hieman eri tavalla, siinä kantaluokan voi määritellä yksityiseksi, jolloin ulkopuolisen silmin Db ei olekaan PDO suorasta periytymisestä huolimatta.)

Jos SQL:n kirjoittaminen käsin tai enintään Ripen edellisessä koodissa esitetyllä tasolla ei sovi tai kiinnosta, kannattaa saman tien käyttää jotain valmista ratkaisua eikä ruveta itse virittelemään mitään ihmeellisiäa QueryBuilder-luokkia.

qeijo kirjoitti:

Olisihan se hyvä jos PostMapper luokan ei tarvitsisi välittää tiedon alkuperästä. Hieman ehkä yliampuvaa tässä tapauksessa...

Näkisin tuon järkevänä lähinnä silloin jos haluttaisiin mahdollistaa objektien persistointi muuallekin kuin relaatiotietokantoihin (esim flat-tiedotoihin tai oliokantoihin). Silloin noiden erilaisten tietovarastojen käsittelyyn pitäisi olla kuitenkin yhtenäinen rajapinta, jonka toteutus voitaisiin sitten antaa mapperille parametrina. Mapperi voisi sitten käsitellä dataa välittämättä siitä mistä se tulee. Tuo esimerkki sisältää nyt jo kuitenkin kovakoodattua SQL:ää ja olisi aika kaukaa haettua että esim. jonkin flat-tiedostoihin objekteja tallentavan systeemin pitäisi osata SQL:n perusteella hakea niitä haluttuja tietoja.

Periaatteessa koodaus rajapintoja vasten on aina kauaskatseisempaa kuin koodaus toteutuksia vasten, mutta käytännön toteutuksissa abstrahoinnin raja pitää kuitenkin vetää johonkin kohtaan. Se mikä tuo kohta on, on toki tapauskohtaista. Jos data on nyt persistoitu relaatiokantaan, niin on tosiaan aika yliampuvaa koodata jo etukäteen tuki myös muille tallennustaoville ihan vaan siltä varalta, että jossain vaiheessa halutaisiinkin vaihtaa johonkin sellaiseen tallennusmuotoon, jota PDO ei tue. Todennäköisyys sille että noin käy, on aika pieni, ja kannattaa muistaa että koodia voi muuttaa jälkeenpäinkin.

Tukki kirjoitti:

Tuo esimerkki sisältää nyt jo kuitenkin kovakoodattua SQL:ää ja olisi aika kaukaa haettua että esim. jonkin flat-tiedostoihin objekteja tallentavan systeemin pitäisi osata SQL:n perusteella hakea niitä haluttuja tietoja.

En väittänytkään etta ehdottamani ratkaisu pitäisi sisällään juuri kyseistä toteutusta.!

Tukki kirjoitti:

Jos data on nyt persistoitu relaatiokantaan, niin on tosiaan aika yliampuvaa koodata jo etukäteen tuki myös muille tallennustaoville ihan vaan siltä varalta, että jossain vaiheessa halutaisiinkin vaihtaa johonkin sellaiseen tallennusmuotoon, jota PDO ei tue.

Ei tietenkään tarvitse tukea etukäteen, mutta kyllä abstrahoinnin kautta voidaan toteuttaa helposti myös alkuunsa geneerisiä ratkaisuja, jotka mahdollistavat myöhemmässä vaiheessa esimerkiksi XML - tietolähteiden hyödyntämisen.

Mutta, kuten jo sanoin aikasemmin, tämä on hieman yliampuvaa tässä tapauksessa...

Pitäisikö tämä koodin palanen upottaa controller-luokan constructoriin vai siihen metodiin, jossa tuota $postMapper-muuttujaa käytetään? Eli kumpi tapa on järkevämpi?

$postMapper = new PostMapper(DbPdo::getInstance());

Edit:

Tukki kirjoitti:

Se mikä tuo kohta on, on toki tapauskohtaista. Jos data on nyt persistoitu relaatiokantaan, niin on tosiaan aika yliampuvaa koodata jo etukäteen tuki myös muille tallennustaoville ihan vaan siltä varalta, että jossain vaiheessa halutaisiinkin vaihtaa johonkin sellaiseen tallennusmuotoon, jota PDO ei tue.

Tällä hetkellä käytän MySQL:llää projektissani, ja tarkoituksenani on saada tämä joskus jonnekin webhotelliin, joten kovin kummallisia tietokantoja, esim. MongoDB:tä, en tule tämän kanssa käyttämään, korkeintaan PostgreSQL:llää.

Suosittelisin kokeilemaan Doctrinea, kun tässä kerta orm:ää yritetään käyttää. Käsin itse tekemällä saa aikaan vain paskaa, varsinkin jossei edes ymmärrä, mitä on tekemässä.

http://www.doctrine-project.org/

Doctrinea käyttäessä ei tarvitse itse kirjoittaa sql-kyselyitä oikeastaan ensimmäistäkään. Sen kuin mättää koodiinsa luokat, lisää annotaatiot jäsenmuuttujiin, ja jatkaa itse sovelluksen koodaamista.

Käytän nyt tuota qeijon koodia mallina, mutta mietityttää se, että mihin luokkaan tässä koodivinkissä esitellyt metodit asetaSalasana ja tarkistaSalasana voisi laittaa? Mielestäni ne eivät kuulu User-luokkaan (samaa tyyliä kuin Post-luokka (qeijon koodissa)), vaan enneminkin UserMapper-luokkaan, mutta ei UserMapper:kaan tunnu täysin oikealta vaihtoehdolta.

Juurikin qeijon esimerkin mukaisesti ne kuuluisivat kyseiseen User-luokkaan, koska jäsenmuuttujat ovat yksityisiä, eikä ole hyvän tavan mukaista käpistellä niitä suoraan luokan ulkopuolelta vaikka setterit kyseisessä esimerkissä siihen mahdollisuuden antavatkin. Eri asia, jos kyseinen luokka olisi POD tyylinen niin silloinhan nuo metodit voisi toteuttaa ihan puhtaasti funktioina ja toteutuksen voit valita mieltymyksesi mukaan joko

$kayttaja->tiiviste = muunnaTiivisteeksi($salasana);

tai

muunnaTiivisteeksi($kayttaja, $salasana);

joista itse käyttäisin ensimmäistä tyyliä.

Jos nyt oikein ymmärsin, niin tuo muunnaTiivisteeksi pitäisi siis laittaa User-luokan metodiksi.

EDIT: Aion siis laittaa nuo metodit User-luokkaan. Onko se hyvä tyyli?

Lisää edittiä:

class User {
	// Muut metodit

	public function setPasswordHash($value){
		$this->password_hash = password_hash($value, PASSWORD_DEFAULT);
	}

	public function verifyPassword($password){
		if (!password_verify($password, $this->password_hash)){
			return false;
		} else {
			if (password_needs_rehash($this->password_hash, PASSWORD_DEFAULT)){
				$this->setPasswordHash($password);
			}
			return true;
		}
	}
}

Miten tuohon luokkaani/toteutukseeni voi toteuttaa tuon käyttäjän tallennuksen, joka koodivinkissä esitellään?

Nuo koodivinkissä esitetyt tallennus ja hae-metodit kuuluisivat mielestäni sitten tuonne qeijon esittelemään mapper-luokkaan.

Okei, kysymykseni oli hiukan epäselvä, mutta kiitos Othnos selvennyksistä, sain kaipaamani vastauksen, ja osan ongelmasta selvitin itse.

Viimeisessä koodissasi setPasswordHash on hämäävä nimi funktiolle, kun parametrina on kuitenkin salasana. Muutenkin funktion nimi velvoittaa luokan tallentamaan salasanat tiivisteinä, vaikka käyttäjän kannalta sen voisi yhtä hyvin tallentaa selkokielisenä. Niinpä funktion looginen nimi olisi setPassword, koska käyttäjän näkökulmasta se vain asettaa salasanan, joka myöhemmin tarkistetaan. Eihän tarkistuskaan ole verifyPasswordHash.

Tulipa tietoturvan kannalta arveluttava esimerkki, mutta ymmärrät kai idean. Tässä oikeastaan tuleekin esiin yksi olio-ohjelmoinnin peruspilari: luokalla on rajapinta, joka on helppo ymmärtää, ja toteutus, josta muiden ei tarvitse välittää.

Kun vaihdat nimen, koodisi on oikein hieno. Tosin sehän on sitten melkein suoraan vinkistä. ;)

Järkeilin itsekin joskus viimeisimmän viestini jälkeen, että nimeksi ei oikein sovi tuo setPasswordHash, melkein samoilla perusteilla, kuin Metabolixin perustelut.

Metabolix kirjoitti:

Kun vaihdat nimen, koodisi on oikein hieno.

Mikään ei ole täydellistä, mutta hyvää voi olla, ja tässä tapauksessa koodini on hyvää vain ja ainoastaan teidän kaikkien, jotka autoitte minua, ansiosta.

Metabolix kirjoitti:

Tosin sehän on sitten melkein suoraan vinkistä. ;)

Ai, en tiennyt, ettei saisi olla... :D Miksei hyvää esimerkkiä voisi käyttää, siihen kai se on tarkoitettu, tosin ehkä vähän epäsuorempi kopiointi voisi olla parempi tapa, mutta nyt näin.

Voisi sanoa, että koko koodini periaate muuttui teidän ansiostanne paljon parempaa suuntaan. Tämän projektini alun (ja näiden kolmen keskustelun) aikana olen oppinut paljon, miten tehdä asioita järkevämmin, ja myös olio-ohjelmoinnin idean olen sisäistänyt jotenkuten.

Joten, kiitos kaikille auttajilleni.

Salasanan hashaaminen pitäisi tehdä jossain muualla kuin User-luokassa. Olion tulee sisältää vain salasanan tiiviste, joten getteri palauttaa tiivisteen. Täten setterin täytyy, ollakseen looginen, vastaanottaa tiiviste. Tai kenties voidaan lisätä toinen setteri, jolle annetaan raaka salasana ja luokka muuttaa sen itse tiivisteeksi.Tällöin kuitenkin naitetaan salasanan tiivistämisen toteutus käyttäjäluokkaan vailla mitään perusteita.

Salasanojen tiivistämistä voidaan tarvita monessa muussakin yhteydessä täysin irrallaan User-luokasta, joten ei ole mitään järkeä upottaa sitä kyseiseen luokkaan.

Eihän salasanan tiivistystä ole upotettukkaan User-luokkaan. Voit edelleen käyttää muualla koodissasi suoraan password_hash($salasana, PASSWORD_DEFAULT)-funktiota. Kyllä itse pidän järkevämpänä, huomattavasti siistimpänä ja enemmän OO-tapana käyttää kyseistä koodivinkin tyyliä. Jos luokka sisältää vain gettereitä ja settereitä niin silloin on sama jo mennä tuohon POD-tyyliseen ohjelmointiin.

Vrt.

$kayttaja->asetaSalasana($salasana);

ja

$kayttaja->asetaSalasana(password_hash($salasana, PASSWORD_DEFAULT));

The Alchemist, montako luokkaa tarvitset, jotta saat tehtyä tämän yksinkertaisen asian mielestäsi oikein?

Olipa luokkahierarkia mikä hyvänsä, pääasia on, että jossain luokassa on tämä äsken mainittu rajapinta ja että missään tapauksessa ei sirotella mitään hash-kikkailuja ympäriinsä koodissa. Jos nyt jonkun ideologia vaatii, että User sisältää vain dataa ja kirjautuminen on vaikka jossain UserLoginManager-luokassa, olkoon sitten niin. Minusta kuitenkin siinä mennään sikäli pahasti metsään, että User sisältääkin tietoa, jota ei ole mahdollista hyödyntää ilman juuri oikeanlaista UserLoginManageria. On ihan turha vaahdota ensin jostain tietokantaobjektin singleton-riippuvuuksista, jos heti perään tyrkyttää muuta koodia, jossa jokainen luokka vaatii kymmenen muuta toimiakseen.

Alchemist varmaan tarkoitti että tuo tapa miten salasanat tiivistetään pitäisi olla vian yhdessä kohtaa koodissa, mutta kuitenkin niin että sitä voisi käyttää tarvittaessa muuallakin kui User-luokassa. Tässä muutama erilainen lähestymistapa asiaan:

Jos User-oliot on todellakin ainoita paikkoja missä sovelluksessa tarvitaan salasanoja niin tämä on varmaan ihan ok:

class User {
  public function setPassword($plain_password){
    $this->password_hash = password_hash($plain_password, PASSWORD_DEFAULT);
  }
  public function setPasswordHash($password_hash){
    $this->password_hash = $password_hash;
  }
}
// Käyttö
$user->setPassword($plain_password);

Jos salasanoja tarvitaan muuallakin, esim muissa luokissa, mutta ne halutaan hashata aina samalla tavalla kaikille olioille ja hashaus-tapa halutaa pitää ainoastaan yhdessä kohtaa koodia, niin voisi ajatella jotain tällaista:

class PasswordHasher {
  public function hashPassword($plain_password) {
    return password_hash($plain_password, PASSWORD_DEFAULT);
  }
}
// Käyttö
$password_hasher = new PasswordHasher();
$user->setPasswordHash($password_hasher->hashPassword($plain_password));

seuraava aste voisi sitten olla että mukaan halutaan oliokohtaiset suolat, jolloin toteutus voisi olla vaikka tällainen:

interface SaltAwareInterface {
  public function getSalt();
}

class SaltedUser extends User implements SaltAwareInterface {
  private $salt;
  public function getSalt() {
    // suolan asettaminen (tai laskeminen) ei kuulu esimerkkiin
    return $this->salt;
  }
}

class SaltedPasswordHasher {
  public function hashPassword(SaltAwareInterface $object, $plain_password) {
    return password_hash($plain_password, PASSWORD_DEFAULT, array('salt' => $object->getSalt()));
  }
}
// Käyttö
$password_hasher = new SaltedPasswordHasher();
$user->setPasswordHash($password_hasher->hashPassword($user, $plain_password));

Lopulta, jos halutaan että jokaiselle oliolle voidaan hashata salasanan täysin yksilöllisellä tavalla, niin voidaan tehdä jotain tällaista:

interface PasswordAwareInterface { /* tarvittavat metodit, eli ne joiden perusteella oliokohtainen hasher tulisi valita ja joita tarvitaan hashin luomiseen */ }
interface PasswordHasherInterface {
  public function hashPassword(PasswordAwareInterface $object, $plain_password);
}
class User implements PasswordAwareInterface {
  // rajapinnan totetus tähän...
}

class PasswordHasherFactory {
  /**
   * @return PasswordHasherInterface $password_hasher
   **/
  public static function getPasswordHasher(PasswordAwareInterface $object) {
    // Tänne annetun olion ominaisuuksista riippuva logiikka, jolla luodaan sille tarkoitettu PasswordHasher
    return $password_hasher;
  }
}
// Käyttö
$password_hasher = PasswordHasherFactory::getPasswordHasher($user);
$user->setPasswordHash($password_hasher->hashPassword($user, $plain_password));

Näistä voi sitten valita tarpeita vastaavan lähestymistavan. Veikkaan että esim viimeinen on vähän yliampuva useimmissa koti-, tai harjoitteluprojekteissa.

Lisäys:

Metabolix kirjoitti:

The Alchemist, montako luokkaa tarvitset, jotta saat tehtyä tämän yksinkertaisen asian mielestäsi oikein?

Tuossahan noita oli jo aika monta. Keksiikö joku vielä lisää? Ei luokkien määrä itsessään ole hyvä tai huono asia. Niitä pitää olla tarpeen mukaan.

Metabolix kirjoitti:

On ihan turha vaahdota ensin jostain tietokantaobjektin singleton-riippuvuuksista, jos heti perään tyrkyttää muuta koodia, jossa jokainen luokka vaatii kymmenen muuta toimiakseen.

Ei riippuvuudet ole pahasta vaan piilotetut riippuvuudet. Singletonien kanssa tulee helposti koodattua noita piilotettuja riippuvuuksia.

Miksi User luokan pitäisi ottaa millään laillla kantaa hashin toteutukseen? Hashaus muualla ja välitys User:ille parametrina, KISS.

Toinen vaihtoehto olisi antaa User luokan setPassword metodille PasswordHasherInterface tyyppisen rajapinnan toteutus parametrina, tällöinkin teetetään hieman turhaa työtä Userilla, mutta mahdollistaa erillaisten PasswordHasherInterface toteutusten hyödyntämisen.

Edit: Korjailua..

<?php

interface PasswordHasherInterface {

    public function hashPassword($plaintextPassword);
}

class PasswordHasherPhp implements PasswordHasherInterface {

    public function hashPassword($plaintextPassword) {}
}

class User {

    private $id, $name, $emial, $passwordHash;

    public function setPassword(PasswordHasherInterface $passwordHasher, $plaintextPassword) {

        $this->passwordHash = $passwordHasher->hashPassword($plaintextPassword);
    }

    public function setPasswordHash($passwordHash) {

        $this->passwordHash = $passwordHash;
    }

    //etc
}

$user = new User();

$user->setPassword(new PasswordHasherPhp, 'Rambo2000');

Metabolix kirjoitti:

The Alchemist, montako luokkaa tarvitset, jotta saat tehtyä tämän yksinkertaisen asian mielestäsi oikein?

Kyllähän niitä varmasti aika monta tulee, jos lähtee puristisesti väsäämään.

Katson tässä ZF2:lle tehtyä ZfcUser-moduulia, joka sisältää 46 luokkaa. Kyseinen moduuli toteuttaa käyttäjien rekisteröitymisen, sisään- ja uloskirjautumisen sekä yksinkertaisen profiilisivun, johon ei kuulu tukea tietojen muokkaamiselle. Siihen vielä päälle 9 kpl jaettuja riippuvuuksia ZfcBase-moduulista ja 3 kpl Doctrine-integraation tuottavasta lisämoduulista.

Itse käytin aikoinaan puhtaalta pöydältä kirjoittamassani kehyksessä erillistä Password-luokkaa, jonka vastuulla oli selkokielisen salasanan tiivistäminen ja validointi annetun olemassaolevan tiivisteen kanssa. Kyseisen luokan instanssi palautti __toString()-taikametodissa tiivistetyn muodon salasanasta, joten sitä pystyi periaatteessa käyttämään stringinä.

ZF2:ssa käytetään myös samalla tavalla erillistä salasanaluokkaa (Zend\Crypt\Password\Bcrypt). ZfcUser-moduulissa salasanaluokkaa käytetään kahdessa eri paikassa: ZfcUser\Service\User-luokassa (rekisteröitymisvaihe) sekä ZfcUser\Authentication\Adapter\Db-luokassa (autentikointi).

Qeijon esittämässä ratkaisussa salasanan tiivistämisen toteutuksen injektoimisessa User-luokkaan on se vika, että edelleenkään setteri ei tee sitä mitä pitäisi vaan yrittää kikkailla liikaa.

Hyvin usein kannasta haettu tai lomakkeelta tuleva data luetaan olioon sisälle automaattisesti käyttämällä ns. hydrator-luokkia. Hydrator voi toimia monella eri tavalla, mutta perinteisiä tapoja on kaksi:

1. Gettereiden ja settereiden käyttö.
2. Datan kirjoittaminen suoraan muuttujiin.

Automaattisen, geneerisen hydraattorin käyttö on täysin mahdotonta, jos salasanan setteri ei toimi kuten setterin pitäisi toimia, eli että sille annetaan arvo ja tämän jälkeen olio sisältää kyseisen arvon.

Periaatteessa pitäisi olla mahdollista tehdä näin ilman korruptoitumista: $object->setValue($object->getValue())

Mikäli annettu merkkijono hashataan aina setteriä kutsuttaessa, tiedot korruptoituvat, tai sitten pitää keksiä poikkeussääntöjä tämän yhden tietyn setterin kutsumiseen; täysin turhaa.

The Alchemist kirjoitti:

Qeijon esittämässä ratkaisussa salasanan tiivistämisen toteutuksen injektoimisessa User-luokkaan on se vika, että edelleenkään setteri ei tee sitä mitä pitäisi vaan yrittää kikkailla liikaa.

Ratkaisuni oli geneerinen vaihtoehto Tukin esittämään vastaavanlaiseen ratkaisuun:

Tukki kirjoitti:

Jos User-oliot on todellakin ainoita paikkoja missä sovelluksessa tarvitaan salasanoja niin tämä on varmaan ihan ok:

class User {
  public function setPassword($plain_password){
    $this->password_hash = password_hash($plain_password, PASSWORD_DEFAULT);
  }
  public function setPasswordHash($password_hash){
    $this->password_hash = $password_hash;
  }
}
// Käyttö
$user->setPassword($plain_password);

The Alchemist kirjoitti:

Mikäli annettu merkkijono hashataan aina setteriä kutsuttaessa, tiedot korruptoituvat, tai sitten pitää keksiä poikkeussääntöjä tämän yhden tietyn setterin kutsumiseen; täysin turhaa.

En tiedä missä vaiheessa koodia olet kommentoinnut, mutta kyllä kuten siihen pohjautuvassa Tukin esimerkkissäkin oli, ajatuksena on että voidaan asetta myös jo hashattu merkkijono.

Itse olen edelleen samalla linjalla itseni kanssa, että antaa Userin olla Useri.

qeijo kirjoitti:

Miksi User luokan pitäisi ottaa millään laillla kantaa hashin toteutukseen? Hashaus muualla ja välitys User:ille parametrina, KISS.

Tukki kirjoitti:

seuraava aste voisi sitten olla että mukaan halutaan oliokohtaiset suolat

Aika ironinen keksintö sikäli, että password_hash luo automaattisesti yksilöllisen suolan joka kutsukerralla (ellei erikseen muuta määrätä), joten kaikki omatekoiset räpellykset todennäköisesti vain heikentävät tiivisteen tietoturvaa.

The Alchemist kirjoitti:

Periaatteessa pitäisi olla mahdollista tehdä näin ilman korruptoitumista: $object->setValue($object->getValue())

Tietenkin – jos luokassa on getteri. Olio-ohjelmoinnin idean mukaista on tarjota järkeviä kokonaisuuksia, mikä salasanan kohdalla tarkoittaa funktioita setPassword ja verifyPassword. Onneksi en ole vielä nähnyt käyttäjäluokkaa, jossa olisi funktio getPassword.

Jos getPassword-funktio todella jossain onkin, minusta ei ole mikään ongelma, että setPassword(getPassword()) tuottaa eri tiivisteen kuin ennen. Herää kuitenkin kysymys, mitä varten tiivisteitä edes lasketaan, jos salasana on saatavilla myös selväkielisenä.

Sanoinhan jo, että olion pitäisi sisältää vain tiiviste. Siispä funktiot getPassword() ja setPassword() ovat vain lyhenteitä nimille getPasswordHash() ja setPasswordHash(). On varsin tavallista, että kannassa on sarake 'password' eikä 'password_hash' ja että oliolla on jäsenmuuttuja 'password' eikä 'password_hash'. En näe mitään ihmeellistä siinä, että samaa logiikkaa jatketaan myös julkisessa rajapinnassa.

Sanoinhan jo, että olio voi laskea sen tiivisteensä itse eikä ulkopuolisten tarvitse tietää siitä mitään. Kyllä funktion nimen kannattaa myös vastata sen toimintaa, vaikka jotkut käyttävätkin väärää nimeä. Setterit ja getterit eivät myöskään ole hyvän koodin merkki, päinvastoin; kuten sanoin, luokan pitäisi toteuttaa toimintoja eikä vain siirrellä dataa 20 luokan läpi ilman järkevää päämäärää.

PHP:ssä on nyt tiivisteitä varten niin triviaalit funktiot, että toiminnallisuuden "upottaminen" omaan luokkaan ei aiheuta koodin duplikointia. Kun User-luokka ei anna tiivistettä koskaan ulos, on turha vaahdota myöskään yhteensopivan koodin käytöstä kaikissa projektin osissa. PHP:n funktioiden laittaminen omatekoiseen luokkaan, joka ei edes tee muuta kuin kutsuu suoraan noita funktioita, on myös ihmeellistä pelleilyä, jossa tehdään luokkia ihan vain luokkien takia ilman mitään realistista käytännön syytä. Ei myöskään ole kovin yleistä, että tuollaista salasanoihin erikoistunutta tiivistefunktiota tarvittaisiin juuri missään muussa yhteydessä.

Käytäntö on ihan hyvä. Olio ei tiedä mitään alkuperäisestä salasanasta, vain tiivisteestä. Tiiviste on salasana. Alkuperäistä salasanaa ei edes tarvita mihinkään, riittää kun keksii jonkin merkkijonon, joka tuottaa saman tiivisteen.

Olen melkolailla eri mieltä tuosta, ettei enää ole mitään toteutusta, jonka upottamista User-luokan koodiin pitäisi välttää. Jokainen tiiviste on sidottu tiettyyn toteutukseen. Tiivisteen laskemiseen käytettyä algoritmia joudutaan vaihtamaan, jos kyseisestä algoritmista löydetään vakava haavoittuvuus tai jos siitä tulee liian kevyt laskettava ja sitä kautta turvallisuus heikkenee.

Tietysti tässä vaiheessa voidaan alkaa miettiä, millä strategialla lähdetään kannassa olevia tiivisteitä päivittämään. Joskus voi olla pakko vain vetää kanta tyhjäksi ja pakottaa käyttäjät luomaan uusi salasana, mutta monesti voi olla parempi jättää vanhat tiivisteet kantaan ja päivittää ne sitä mukaa, kun käyttäjät kirjautuvat palveluun (eli siis kirjautumisen yhteydessä). Tällöin pitäisi User-luokkaan tunkea täysin käyttäjän ominaisuuksista riippumatonta koodia, joka tunnistaisi vanhentuneen tiivisteen ja päivittäisi sen uuteen ja sitä rataa. Turhaa bloattia aivan väärässä paikassa.

Salasanan varmentaminen liittyy muutenkin ainoastaan aktiiviseen käyttäjään eikä yleisesti kaikkiin käyttäjäolioihin. Sellaista tilannetta ei ole, missä käyttäjä Siika yrittäisi varmentaa Pekan tai Matin salasanoja. Se ei siis oikeastaan edes paperilla kuulu millään tavoin User-luokkaan.

The Alchemist kirjoitti:

Turhaa bloattia aivan väärässä paikassa.

Sanoo herra, jonka toteutuksessa on enemmän luokkia kuin tässä luokassa koodirivejä. ^^

Ohjelmoinnissa pitää osata ajatella vähän muutakin kuin sitä, mikä nyt on jossain tietyssä skenaariossa UML-kaavioiden kannalta tyylikkäin ratkaisu.

The Alchemist kirjoitti:

Sellaista tilannetta ei ole, missä käyttäjä Siika yrittäisi varmentaa Pekan tai Matin salasanoja.

Siksi ei olekaan funktiota $siika->verifyPasswordOf($pekka, $password).

Kannattaisi ehkä opetella vähän ohjelmistosuunnittelun alkeita, jos bloattiuden määritelmä on se, kuinka monta komponenttia järjestelmässä on.

Lisäys:

Metabolix kirjoitti:

The Alchemist kirjoitti:

Sellaista tilannetta ei ole, missä käyttäjä Siika yrittäisi varmentaa Pekan tai Matin salasanoja.

Siksi ei olekaan funktiota $siika->verifyPasswordOf($pekka, $password).

Ymmärsit väärin. User-luokan instanssilla ei ole sellaista toiminnallisuutta kuin "varmista salasana". Se on järjestelmän ominaisuus. Järjestelmä autentikoi käyttäjän, joka pyrkii sisälle järjestelmään. Järjestelmä ei autentikoi User-luokan instansseja vaan järjestelmän ulkopuolisen agentin. User-luokka on yhtä geneerinen tietovarasto kuin BlogPost- tai ForumThread-luokatkin.

Voisitko perustella, miksi mielestäsi User-luokka on juuri tuollainen eikä missään tapauksessa muunlainen? Se ei takuulla ole olio-ohjelmoinnin perimmäinen totuus vaan vain sinun näkemyksesi tai käyttämisi frameworkin tapa. Minkälainen sinusta tämän salasanaketjun pitäisi ihan konkreettisesti olla ($_POST-taulukosta User-luokkaan asti), ja mitä etuja mielestäsi saavutat sillä rakenteella?

Metabolix kirjoitti:

Voisitko perustella, miksi mielestäsi User-luokka on juuri tuollainen eikä missään tapauksessa muunlainen?

Mielestäni User luokka on tässä tapauksessa mallin roolissa oleva "domain objekti" joka kuuluu sisältää ainoastaan kyseisen objektiin liittyviä jäseniä ja toiminnallisuutta.

Eli käytännössä muusta riippumaton objekti, jota voidaan hallinnoida esim. datamappereilla.

interface UserMapperAdapterInterface  {

    public function fetchAll();
    //etc
}

class XmlUserMapperAdapter implements UserMapperAdapterInterface  {

    private $xml;

    public function __construct($xmlFile) {

        $this->xml = simplexml_load_file($xmlFile);
    }

    public function fetchAll() {

        $users = array();

        foreach ($this->xml->user as $user)
            $users[] = (array)$user;

        return $users;
    }
}

class PdoMysqlUserMapperAdapter implements UserMapperAdapterInterface  {

    private $db;

    public function __construct(PDO $db) {

        $this->db = $db;
    }

    public function fetchAll() {

        $query = $this->db->prepare('SELECT * FROM users');
        $query->execute();

        return $query->fetchAll();
    }
}

class UserMapper {

    protected $mapperAdapter;

    public function __construct(UserMapperAdapterInterface  $mapperAdapter) {

        $this->mapperAdapter = $mapperAdapter;
    }

    public function fetchAll() {

        $result = $this->mapperAdapter->fetchAll();

        $users = array();

        foreach($result as $entry) {

            $user = new UserModel();

            $user->setId($entry['id'])
                    ->setName($entry['name'])
                    ->setEmail($entry['email'])
                    ->setPasswordHash($entry['passwordhash']);

            $users[] = $user;
        }

        return $users;
    }
}

class UserModel {

    private $id, $name, $email, $passwordHash;

    public function __get($name) {

        return $this->$name;
    }

    public function setId($id) {

        $this->id = $id;
        return $this;
    }

    public function setName($name) {

        $this->name = $name;
        return $this;
    }

    public function setEmail($email) {

        $this->email = $email;
        return $this;
    }

    public function setPasswordHash($passwordhash) {

        $this->passwordHash = $passwordhash;
        return $this;
    }
}

// XML

$userMapper = new UserMapper(new XmlUserMapperAdapter('users.xml'));
$users = $userMapper->fetchAll();

// Mysli

$userMapper = new UserMapper(new PdoMysqlUserMapperAdapter(DbPdo::getInstance()));
$users = $userMapper->fetchAll();

The Alchemist kirjoitti:

Sanoinhan jo, että olion pitäisi sisältää vain tiiviste.

Valitettavasti olioni täytyy sisältää tiivisteen ohella myös selväkielinen salasana, mutta vain siinä kohtaa, kun uusi käyttäjä rekisteröityy, eli uusi User-olion instanssi luodaan, koska muuten en voi varmistaa, että käyttäjän antama salasana täsmää käyttäjän antamaan salasanan varmennukseen rekisteröintivaiheessa.

// kuuluu UserMapper-luokkaan
public function validate(User $user) {
	if (!$user->username || !$user->fullname || !$user->email || !$user->password || !$user->password_again){
		return array(false, "Puuttuvia tietoja");
	}
	if ($this->fetch(array('username' => $user->username))->username == $user->username){
		return array(false, "Käyttäjänimi on virheellinen");
	}
	if ($user->password_text != $user->password_again){
		return array(false, "Salasanat eivät täsmää");
	}
	return array(true);
}

// kuuluu userController-luokkaan
public function create_action(){
	$user = new User();
	$user->setUsername($_POST['username'])
		->setFullname($_POST['fullname'])
		->setEmail($_POST['email'])
		->setPassword($_POST['password'])
		->setPlainPassword($_POST['password'])
		->setPasswordAgain($_POST['password_again'])
		->setPhoto();

	$validate_result = $this->userMapper->validate($user);
	if ($validate_result[0]){
		$this->userMapper->insert($user);
		$this->login_action();
	} else {
		die("Käyttäjän rekisteröinti epäonnistui: " . $validate_result[1]);
	}
}

// seuraavat metodit kuuluvat User-luokkaan
public function setPassword($value = NULL){
	$this->password = ($value === NULL) ? NULL : password_hash($value, PASSWORD_DEFAULT);
	return $this;
}

public function setPasswordAgain($value = NULL){
	$this->password_again = $value;
	return $this;
}

public function setPlainPassword($value = NULL){
	$this->plain_password = $value;
	return $this;
}

qeijo kirjoitti:

Mielestäni User luokka on tässä tapauksessa mallin roolissa oleva "domain objekti" joka kuuluu sisältää ainoastaan kyseisen objektiin liittyviä jäseniä ja toiminnallisuutta.

OO:n perusperiaatteisiinhan kuuluu, että olio huolehtii itse siitä, että sen data on "käyttökelpoista". Kyseisessä tapauksessa käytät User-luokkaa POD omaisesti, joka on myös käyttökelpoinen ohjelmointi tapa, mutta itse määrittäisin suoraan kaikki User-luokan muuttujat julkisiksi, koska nuista gettereistä ja settereistä ei loppuen lopuksi tule kuin sotkua ja muutoksia tehdessä riittää tehdä ne kyseiseen UserDataMapperiin.

Jos halutaan ohjelmoida OO-tyylisesti niin piilotetaan ne muuttujat sitten oikeasti ja annetaan luokan itse huolehtia, että ne sisältävät "käyttökelpoista" dataa.

class User {
    private $id, $name, $email, $password;

    public function __construct($id, $name, $email, $password) {
        $this->id = $id;
        $this->name = $name;

        if (validateEmail($email)) {
            $this->email = $email;
        } else {
            throw new Exception('Email isn´t valid.');
        }

        $this->password = $password;
    }

    public function __get($name) {
        return $this->$name;
    }

    public function changeEmail($newEmail) {
        if (validateEmail($newEmail)) {
            $this->email = $newEmail;
        } else {
            throw new Exception('New email isn´t valid.');
        }
    }
}

Ripe kirjoitti:

Valitettavasti olioni täytyy sisältää tiivisteen ohella myös selväkielinen salasana, mutta vain siinä kohtaa, kun uusi käyttäjä rekisteröityy, – –

Siinä nyt ei ole mitään logiikkaa, että käyttäjällä olisi "salasana" ja "salasana uudestaan". Selvästi tuo tarkistus pitäisi hoitaa jossain kontrollerin puolella jo ennen salasanan syöttämistä käyttäjäoliolle.

Othnos kirjoitti:

Jos halutaan ohjelmoida OO-tyylisesti niin piilotetaan ne muuttujat sitten oikeasti ja annetaan luokan itse huolehtia, että ne sisältävät "käyttökelpoista" dataa.

Tottakai setterit voivat huolehtia tietojensa validoimisesta, ihan vain yksikertaistamisen takia jätin mahdolliset validoinnit ja mm. virhehallinta pois kokonaan. Oli itse asiassa aikasemmin settereissä kommentit "//Validointi?", mutta otin ne pois.

Metabolix kirjoitti:

Selvästi tuo tarkistus pitäisi hoitaa jossain kontrollerin puolella jo ennen salasanan syöttämistä käyttäjäoliolle.

No sinnehän sen tosiaan voisi siirtää, ja oikeastaan koko UserMapper::validate-metodin sisällön voisi siirtää kontrolleriin. Ja User::setPasswordin kai voisi nimetä setPasswordHash:iksi, koska en tarvitse selväkielistä salasanaa enää missään vaiheessa User-olion sisällä, jos siirrän validate-metodin sisällön kontrolleriin.

EDIT:

Othnos kirjoitti:

Jos halutaan ohjelmoida OO-tyylisesti niin piilotetaan ne muuttujat sitten oikeasti ja annetaan luokan itse huolehtia, että ne sisältävät "käyttökelpoista" dataa.

Voisiko tuon koodisi kostruktoriin siis laittaa käytännössä myös tuon oman koodini UserMapper::validate-metodin sisällön, se ainakin sopisi periaatteeseesi, enkä näe mitään syytä, miksi muitakin muuttujia ei voisi validoida siellä, kun kerran niin tehdään myös emailille esimerkissäsi?

Metabolix kirjoitti:

PHP:ssä on nyt tiivisteitä varten niin triviaalit funktiot, että toiminnallisuuden "upottaminen" omaan luokkaan ei aiheuta koodin duplikointia. Kun User-luokka ei anna tiivistettä koskaan ulos, on turha vaahdota myöskään yhteensopivan koodin käytöstä kaikissa projektin osissa. PHP:n funktioiden laittaminen omatekoiseen luokkaan, joka ei edes tee muuta kuin kutsuu suoraan noita funktioita, on myös ihmeellistä pelleilyä, jossa tehdään luokkia ihan vain luokkien takia ilman mitään realistista käytännön syytä.

Salasanojen käsittelyyn liittyen, ei ole ollenkaan huono idea kääriä PHP:n password_hash() -funktiota ohjelman omaan salasanoja hoitavaan osaan.

Perustarpeet tuolla hoitaa suoraan, mutta esimerkiksi reaalimaailmassa voi olla tarvetta käyttää salasanojen kanssa "local parameteriä", tai hoitaa tiivisteen laskennan painavin osa kokonaan muualla jne.

Siinä User alkaisi täyttyä täysin irrallisilla asioilla, no good. Ja vaikka ei käyttäisi muuta kuin password_hash():ia, niin ei tuo silti huonompi vaihtoehto ole vaikka se omaan kokonaisuuteen on kääritty. Mielestäni selkeämpi.

Ripe kirjoitti:

Voisiko tuon koodisi kostruktoriin siis laittaa käytännössä myös tuon oman koodini UserMapper::validate-metodin sisällön, se ainakin sopisi periaatteeseesi, enkä näe mitään syytä, miksi muitakin muuttujia ei voisi validoida siellä, kun kerran niin tehdään myös emailille esimerkissäsi?

Tuon validate-metodisi ensimmäinen ehto kuuluu User-luokkaan. Toisen ehdon voisit toteuttaa vaikka niin, että tekisit UserMapperiin metodin usernameExists($username), joka palauttaa true/false sen mukaan löytyykö käyttäjänimeä ja sen perusteella controllerissa sitten päätät mitä tehdään. Kolmanteen ehtoon Metabolix antoikin jo vastauksen.

Kiitos Othnokselle paljon, tuohan tosiaan on järkevä tyyli sijoitella asiat.
EDIT:

Othnos kirjoitti:

Tuon validate-metodisi ensimmäinen ehto kuuluu User-luokkaan.

Toisaalta, tietojen tarkistusta tarvitaan vain, kun käyttäjä luodaan ja syötetään tietokantaan, ja silloin, kun käyttäjän tietoja muutetaan. Kun käyttäjä mapataan tietokannasta User-olioon ja esimerkiksi käytetään käyttäjänimeä viestien hakemiseen, silloinhan käyttäjälle ei aseteta salasanaa, ja tarkistus heittää aivan turhaan poikkeuksen, kun salasanaa ei aseteta. Joten, miten tuon voisi tehdä vielä järkevämmin?
Itse ajattelisin, että ainakin melko hyvä tapa tehdä tuo olisi tarkistaa ainakin salasana kontrollerissa ja muut tiedot voisi tarkistella User-luokan change-metodeissa ja sitten tiedot voisi syöttää konstruktorin kautta, vähän niinkuin Othnoksen esimerkissä.