Witam

Postanowiłem spróbować swoich sił w OOP, na przykładzie skryptu rejestracji użytkowników. Napisałem coś takiego i o dziwo działa Gdy dane są poprawne dodaje użytkownika, gdy nie to nie dodaje, ale za to również nie wyświetla komunikatu. Moja prośba do Was, doświadczonych w temacie osób, to zobaczenie czy ten kod jest napisany poprawnie (zgodnie ze sztuką), co można w nim ulepszyć i jak najlepiej rozwiązać wyświetlanie komunikatów, które w tej formie nie działają.

Za wszelkie opinie, porady serdecznie dziękuję

[PHP] pobierz, plaintext 
class new_user
{
	public $login;
	public $pass;
	public $re_pass;
	public $email;
 
	function validate($dane)
	{
		$dane = htmlspecialchars($dane, ENT_QUOTES);
		$dane = mysql_real_escape_string($dane);
    		if (!get_magic_quotes_gpc())
    		{
			$dane = addslashes ($dane);
    		}
        	return $dane;
	}
 
	function check_login ()
	{
		if ($this->login==$this->validate($this->login))
		{
			$query = mysql_query('SELECT id FROM test WHERE login = "'.$this->login.'"') or die (mysql_error()); 
 			if (mysql_num_rows($query) != 0)
			{
 				// login zajęty
				$komunikat = 'Wybrany login jest zajęty';
				return false;
			}
			else
			{
 				// login wolny
				return $this->login;
			}
		}
		else
		{
			$komunikat = 'Login zawiera niedozwolone znaki';
			return false;
		}
	}
 
	function check_pass ()
	{
		if ($this->pass==$this->validate($this->pass))
		{
			if ($this->pass==$this->re_pass)
			{
 				if (strlen($this->pass)>5)
				{
					return md5($this->pass);
				}
				else 
				{
					$komunikat = 'Podane hasło jest za krótkie (min. 6 znaków)';
					return false;
				}
			}
			else
			{
				$komunikat = 'Podane hasła są różne';
				return false;
			}
		}
		else
		{
			$komunikat = 'Hasło zawiera niedozwolone znaki';
			return false;
		}
 
	}
 
	function check_email ()
	{
		if (ereg("^[a-z0-9_\\.-]+@([a-z0-9_-]+\\.)+[a-z]{2,}$", $this->email))
		{
			return $this->email;
		}
		else
		{
			$komunikat = 'Podaj poprawny adres e-mail';
			return false;
		}
	}
 
	function add_user ()
	{
		if($this->check_login() && $this->check_pass() && $this->check_email())
		{
			mysql_query("INSERT INTO test(login,pass,email) VALUES('".$this->check_login()."','".$this->check_pass()."','".$this->check_email()."')");
			if (mysql_affected_rows())
			{
				return true;
			}
			else
			{
				return false;
			}
		}
	}
 
}
 
$new = new new_user;
$new->login = 'ssssss';
$new->pass = 'aaaaaaaaa';
$new->re_pass = 'aaaaaaaaa';
$new->email = 'ttttt@gggggg.pl';
$nowy = $new->add_user($new);
echo $komunikat;
[PHP] pobierz, plaintext 

z jakiego powodu nie uzywasz PDO ?

Oczywiscie ze nie
1) poczytaj o zasięgu zmiennych
2) poczytaj o czyms takim jak return, albo wlasciwosci klasy

3)Temat OOP nie do konca powstał na takie tematy. przenosze

podejscie z adduser jest dla mnie troche dziwne. Moim zdaniem powinno byc jakies "save" ktore zapamietywaloby identyfikator uzytkownika, albo w ogole powinienes oddzielic warstwe z tym userem, od warstwy zapisujacej dane do bazy

z save wygladaloby to jakos tak

[PHP] pobierz, plaintext 
$user = new new_user();
$user->jakiesdane = $dane;
try {
  $user->save();
  // dodamy mu jeszzce jakies dane
  $user->innedane = $dane;
  $user->save();
} (..)
[PHP] pobierz, plaintext 

generalnie logiczniej bedzie jesli obiekt uzytkownika bedzie reprezentowal jednego konkretnego uzytkownika. Jesli chcesz masowo dodawac uzytkownikow to moim zdaniem powinienes zrobic inna klase - fabryke - ktora bedzie tworzyla obiekty klasy user

[PHP] pobierz, plaintext 
$fabryka = new fabryka();
$user = $fabryka->nowyuser();
$user->jakiesdane = $dane;
$user->save(); // insert
 
$user2 = $fabryka->nowyuser();
$user2->jakiesdane = $jakiesdane dla innego usera;
$user2->save(); //insert
 
$user3 = $fabryka->ladujusera($id);
$user3->jakiesdane = $jakiesdane dla innego usera;
$user3->save(); //update
 
[PHP] pobierz, plaintext 

moim zdaniem cos w ten desen, ale sie opisalem..

Gdyż to tylko testowy kod, szybciej pisze się bez PDO. Nacodzień jednak korzystam z dobrodziejstw PDO.


Nie bardzo rozumiem jak mam oddzielić warstwę z userem od zapisywania? Rozumiem, że proponujesz, aby w inny sposób przekazać zvalidowane dane do zapytania SQL, tak? Masz jakąś konkretną propozycję?

Napisany przeze mnie kod miałby służyć do rejestracji użytkowników w serwisie poprzez formularz i miałby operować na otrzymanych danych, więc nie będzie to na masową skalę. Nie mniej jednak nie bardzo rozumiem idee "fabryki". Co ona powinna zawiera/robić?

Wyświetlanie komunikatów udało mi się rozwiązać po wskazówkach Nospor'a:) Czy ten sposób jest OK? Czy jest lepsze rozwiązanie?

Natomiast nadal nie bardzo wiem, jak udoskonalić resztę kodu. Bardzo proszę o porady, pomoc. Teraz wygląda on tak:

[PHP] pobierz, plaintext 
 
class new_user
{
    public $login;
    public $pass;
    public $re_pass;
    public $email;
    public $status;
 
    function validate($dane)
    {
        $dane = htmlspecialchars($dane, ENT_QUOTES);
        $dane = mysql_real_escape_string($dane);
            if (!get_magic_quotes_gpc())
            {
            $dane = addslashes ($dane);
            }
            return $dane;
    }
 
    function check_login ()
    {
        if ($this->login==$this->validate($this->login))
        {
            $query = mysql_query('SELECT id FROM test WHERE login = "'.$this->login.'"') or die (mysql_error()); 
             if (mysql_num_rows($query) != 0)
            {
                 // login zajęty
                $this->status = 'Wybrany login jest zajęty';
                return false;
            }
            else
            {
                 // login wolny
                return $this->login;
            }
        }
        else
        {
            $this->status = 'Login zawiera niedozwolone znaki';
            return false;
        }
    }
 
    function check_pass ()
    {
        if ($this->pass==$this->validate($this->pass))
        {
            if ($this->pass==$this->re_pass)
            {
                 if (strlen($this->pass)>5)
                {
                    return md5($this->pass);
                }
                else 
                {
                    $this->status = 'Podane hasło jest za krótkie (min. 6 znaków)';
                    return false;
                }
            }
            else
            {
                $this->status = 'Podane hasła są różne';
                return false;
            }
        }
        else
        {
            $this->status = 'Hasło zawiera niedozwolone znaki';
            return false;
        }
 
    }
 
    function check_email ()
    {
        if (ereg("^[a-z0-9_\\.-]+@([a-z0-9_-]+\\.)+[a-z]{2,}$", $this->email))
        {
            return $this->email;
        }
        else
        {
            $this->status = 'Podaj poprawny adres e-mail';
            return false;
        }
    }
 
    function add_user ()
    {
        if($this->check_login() && $this->check_pass() && $this->check_email())
        {
            mysql_query("INSERT INTO test(login,pass,email) VALUES('".$this->check_login()."','".$this->check_pass()."','".$this->check_email()."')");
            if (mysql_affected_rows())
            {
                $this->status = 'Użytkownik dodany poprawnie.';
                return true;
            }
            else
            {
                $this->status = 'Wystąpił błąd podczas dodawania użytkownika.';
                return false;
            }
        }
    }
 
}
 
$new = new new_user;
$new->login = 'sssssgs';
$new->pass = 'aaaaaaaaa';
$new->re_pass = 'aaaaaaaaa';
$new->email = 'tttttg@ggggg.pl';
$nowy = $new->add_user($new);
echo $new->status;
 
[PHP] pobierz, plaintext 

1. new new_user();. Czy taki zapis ma dla Ciebie sens? Wywal ten śmieszny przedrostek "new_" z nazwy.
2. Metoda validate - przecież ona niczego nie sprawdza - jedyne co robi to przygotowuje dane do zapisu do bazy.
2.1. Tym powinno zajmować się PDO (czy coś takiego).
2.2. Usuwanie "dobrodziejstw" magic quotes powinno być raczej gdzie indziej.
2.3. htmlspecialchars powinno używać się przed wyświetleniem danych, a nie ich zapisywaniem.
3. Status nowego użytkownika to może być "zarejestrowany", "autoryzowany" itp., a nie "login jest zajęty" - od czegoś takiego są wyjątki.
4. Od sprawdzania poprawności emaila masz filter_var, a nie eregi (notabene oznaczone już jako deprecated)
5. Twój obiekt to: reprezentant użytkownika, walidator danych z formularza, warstwa zapisująca dane po rejestracji, jakiś ogólny filtr dla danych trafiających do bazy - a jedna z podstawowych zasad OOP brzmi: jeden obiekt - jedno zadanie.

Metoda ta usuwa, zastępuje niedozwolone znaki, następnie w ifie porównuję login wprowadzony przez użytkownika z tym, który został przepuszczony przez validację. Jeśli oba są jednakowe to login uznany jest za poprawny.


PDO będzie, nie użyłem go tylko w tym kodzie "szkoleniowym"


Chodzi Ci o to, że powinno być uaktywnione na serwerze?


Nie bardzo rozumiem, co i jak mam jeszcze podzielić. Możesz to napisać łopatologiczniej?

Poniżej kod po poprawkach, proszę o kolejne uwagi i wskazówki:)

[PHP] pobierz, plaintext 
 
try {
	$pdo = new PDO('mysql:host=localhost;dbname=baza', 'log', 'pass');
	$pdo -> query("SET NAMES 'utf8'");
	//echo 'Połączenie nawiązane!';
}
catch(PDOException $e)
{
	echo 'Połączenie nie mogło zostać utworzone: ' . $e->getMessage();
	die;
}
 
class user
{
	public $login;
	public $pass;
	public $re_pass;
	public $email;
 
	private $pdo; //obiekt klasy PDO
 
 
    public function __construct(&$pdo)
    {
        $this->pdo =& $pdo; //przekazanie obiektu PDO do klasy.
    }
 
 
	function validate($dane)
	{
		$dane = htmlspecialchars($dane, ENT_QUOTES);
		//$dane = mysql_real_escape_string($dane);
    		if (!get_magic_quotes_gpc())
    		{
			$dane = addslashes ($dane);
    		}
        	return $dane;
	}
 
	function check_login ()
	{
		if ($this->login != $this->validate($this->login))
		{
			throw new Exception('Login zawiera niedozwolone znaki');
		}
 
		$sql = "SELECT id FROM `test` WHERE login= :login"; 
		$start = $this->pdo->prepare($sql);
		$start->bindParam(':login' , $this->login , PDO::PARAM_STR);
		$start->execute(); 
		$result=$start->rowCount();
		$start->closeCursor();
 
		//$query = mysql_query('SELECT id FROM test WHERE login = "'.$this->login.'"') or die (mysql_error()); 
 
		if ($result != 0)
		{
			throw new Exception('Wybrany login jest zajęty');
		}
		return $this->login;			
	}
 
	function check_pass ()
	{
		if ($this->pass != $this->validate($this->pass))
		{
			throw new Exception('Hasło zawiera niedozwolone znaki');
		}
 
		if ($this->pass != $this->re_pass)
		{
 			throw new Exception('Podane hasła są różne');
		}
 
		if (strlen($this->pass)<6)
		{
			throw new Exception('Podane hasło jest za krótkie (min. 6 znaków)');
		}
		return md5($this->pass);
	}
 
	function check_email ()
	{
		if (filter_var($this->email, FILTER_VALIDATE_EMAIL))
		{
			return $this->email;
		}
		throw new Exception('Podaj poprawny adres e-mail');
	}
 
	function save()
	{
		if($this->check_login() && $this->check_pass() && $this->check_email())
		{
			//mysql_query("INSERT INTO test(login,pass,email) VALUES('".$this->check_login()."','".$this->check_pass()."','".$this->check_email()."')");
 
			$sql = "INSERT INTO `test` (login,pass,email) VALUES (:login,:pass,:email)"; 
			$start = $this->pdo->prepare($sql);
			$start->bindParam(':login' , $this->check_login() , PDO::PARAM_STR);
			$start->bindParam(':pass' , $this->check_pass() , PDO::PARAM_STR);
			$start->bindParam(':email' , $this->check_email() , PDO::PARAM_STR);
			$result=$start->execute(); 
			$start->closeCursor();
			echo $result;		
			if ($result == 1)
			{
				return true;
			}
			else
			{
				throw new Exception('Wystąpił błąd podczas dodawania użytkownika.');
			}
		}
	}
 
}
 
try
	{
		$new = new user($pdo);
		$new->login = 'ssss#%@#&;"sg/s0aa';
		$new->pass = 'aaaaaaaaa';
		$new->re_pass = 'aaaaaaaaa';
		$new->email = 'tttttg@ggggg.pl';
		$new->save();
		echo 'Użytkownik dodany pomyślnie';
	}
catch (Exception $error)
	{
		echo $error->getMessage();
	}
 
 
[PHP] pobierz, plaintext 

Za dalsze uwagi, będę bardzo wdzięczny

W kodzie, jak widać użyłem już PDO. Co jeszcze można/należałoby poprawić ?

Kolego według mnie nie potrzebnie pakujesz całą funkcjonalność do jednej klasy.
Model MVC się kłania.

Zrób osobno akcje, osobną klasę do walidacji danych, osobną do zapisu usera do bazy (tam metody zapisujace, a takze sprawdzajace dostepnosc jakiegos loginu). W ten sposób będzie to miało ręce i nogi bo to co przedstawiles to faktycznie klasy ale nijak ma sie to do sensu programowania obiektowego

Tzn mówisz, żeby zrobić osobno klasę do walidacji i zapisu, następnie wywoływać je po kolei?

[PHP] pobierz, plaintext 
$check = new validate;
$check ->login = 'ssss#%@#&;"sg/s0aa';
$check ->pass = 'aaaaaaaaa';
$check ->re_pass = 'aaaaaaaaa';
$check ->email = 'tttttg@ggggg.pl';
 
$new = new user($pdo);
$new->login = $check ->login;
$new->pass = $check ->pass;
$new->email = $check ->email;
$new->save();
[PHP] pobierz, plaintext 

Tak miałoby wyglądać ich wywołanie?

Raczej nie chodzi o to.
Powinieneś zakupić jakąś książkę omawiającą OOP - na pewno dużo Ci da i odpowie na wiele pytań. Zobaczysz jak to wszystko wygląda .