[php OOP] Prosba o opinie nt. pierwszej klasy Opcje

[phpion]

Chcialbym was prosic o opinie i sugestie nt. mojej pierwszej klasy podczsa nauki OOP. Co mozna w niej poprawic, co nalezaloby napisac inaczej. Poczatek czyli laczenie z baza i ustawianie sesji jest tylko tymczasowo. Prosze o informacje co i jak. Do czego moglby sluzyc destruktor w tej klasie? Do zerowania atrybutow? Wogole nie widze wiekszego sensu destruktora w tym przypadku - moze sie myle, prosze mnie wiec oswiecic.

[PHP] pobierz, plaintext 
<?php
$sql = mysql_connect("localhost", "root", "");
mysql_select_db("logowanie");
 
session_start();
 
$_SESSION['login'] = "phpion";
$_SESSION['haslo'] = "haslo";
 
class Uzytkownik
{
	private $login;
	private $haslo;
	private $grupa;
	private $zalogowany;
	private $dostep;
 
	function __construct()
	{
		$this->login = (isset($_SESSION['login'])) ? $_SESSION['login'] : NULL;
		$this->haslo = (isset($_SESSION['haslo'])) ? $_SESSION['haslo'] : NULL;
		$this->grupa = 0;
		$this->zalogowany = $this->dostep = FALSE;
	}
 
	function sprawdzZalogowanie()
	{
		if ($this->login != NULL && $this->haslo != NULL)
		{
			$q = "SELECT grupa FROM uzytkownicy WHERE BINARY login='".$this->login."' AND BINARY haslo='".$this->haslo."' AND stan='1'";
			$q = mysql_query($q);
 
			if (mysql_num_rows($q) > 0)
			{
				$r = mysql_fetch_row($q);
 
				$this->grupa = $r[0];
				$this->zalogowany = TRUE;
			}
		}
	}
 
	function sprawdzDostep($typ, $lista)
	{
		$this->sprawdzZalogowanie();
 
		if ($this->zalogowany == TRUE && $this->grupa != 0)
		{
			$tablica = explode(" ", $lista);
 
			if (($typ == 1 && in_array($this->grupa, $tablica) || ($typ == 0 && !in_array($this->grupa, $tablica))))
				$this->dostep = TRUE;
		}
 
		if ($this->dostep != TRUE)
		{
			header("Location: accessDenied.php");
			exit;
		}
	}
}
 
$uzytkownik = new Uzytkownik();
$uzytkownik->sprawdzDostep(0, "1 2");
?>
Tajna tresc
[PHP] pobierz, plaintext 

PS: w metodzie sprawdzDostep $typ moze byc 0 lub 1 (0 dla wylaczania dostepu dla grup, 1 dla udostepnianiu dla wskazanych grup) i liste grup podajemy jako ciag liczb oddzielonych spacja.

Ten post edytował phpion.com 17.07.2006, 20:18:36

[Cysiaczek]

PIerwsz klasa

No to jedziemy :

1. Używasz globalnej tablicy $_SESSION wewnatrz klasy, co jest "be".
2. Oznaczyłeś jako prywatne $login i $haslo. No dobra. to po co Ci te składowe, skoro nie zamierzasz z nich korzystać? Brakuje takich metod jak getUserName() oraz getUserPass(), których możesz użyć do pozyskania informacji o użytkowniku. Domyślam się, że potem korzystasz po prostu z $_SESSION... po co? Masz przeciez klasę i tam masz dane uzytkownika. Wykorzystaj to.

Btw. Pierwsza klasa - Gratz

Pozdrawiam.

[-Gość-]

Masz na mysli takie cos?

[PHP] pobierz, plaintext 
<?php
function getUserName()
{
	$this->login = $_SESSION['login'];
}
?>
[PHP] pobierz, plaintext 

W sumie to robie w konstruktorze... uzywanie $_SESSION w srodku klasy jest 'be' to jak mozna inaczej (lepiej) to rozwiazac? Zastosowanie klasy (klaski ;-) ) podalem w 2 ostatnich linijkach skryptu. Public, private, protected - jak najlepiej ustawic var'y klasy - nie do konca jeszcze czuje roznice zastosowac (teorie znam).

[phpion]

To bylem ja - nie wiem dlaczego mnie wylogowalo...

[Ludvik]

1. Używasz globalnej tablicy $_SESSION wewnatrz klasy, co jest "be".

To nie jest tablica globalna... To jest tablica superglobalna, a to nie jest takie bardzo "be"...

Ja bym zmienił modyfikatory private na protected. Czasami przydaje się dziedziczenie przy takich klasach. Tak jak napisał Cysiaczek, napisz funkcje dostępu. Poza tym sesje są bezpieczne i nie musisz sprawdzać czy użytkownik istnieje co wywołanie strony. Co innego, gdybyś dane pozyskiwał z ciastka, które użytkownik może modyfikować.

Inna uwaga. Zapomnij o exitach w klasach! Nigdy czegoś takiego się nie robi. Możesz wyrzucić wyjątek, który sobie wyłapiesz i zrobisz wtedy co chcesz.

Tyle ode mnie. Nie jest źle

[Cysiaczek]

@Ludvik - no dobra - nie takie bardzo "be", a jesli zechcę sobie (nie pytaj dlaczego) przenosić login i hasełko przez $_GET ? Czy to tak wiele kosztuje przekazanie $_SESSION["user"] i "pass" do konstruktora?

[Ludvik]

Oczywiście, że można. Tak nawet lepiej będzie. Ale to tylko koncepcja autora. Jeżeli chodzi o głębszą interpretację tej klasy to bym podzielił ją na dwie, albo nawet trzy, bo teraz za dużo ma "na głowie". Zajmuje się przenoszeniem danych użytkownika, pobieraniem ich oraz autoryzacją. Nie tędy droga. Jedna klasa musi przechowywać same dane i udostępniać interfejs, dzięki któremu można na nich operować. Druga klasa odpowiada za pobranie danych użytkownika z bazy i stworzenie obiektu. Trzecia klasa operuje na obiekcie użytkownik i porównuje jego prawa dostępu do wymagań. Tak wszystko będzie bardziej elastyczne. Ale nie można oczekiwać elastyczności po pierwszych rozwiązaniach...

Do autora wątku: Atrybuty klasy wypada izolować, aby zachować nad nimi kotrolę. Dostęp najlepiej zapewnić przez publiczne metody. Radzę zapoznać się z lekturą związaną z OOP. Np. Headfirst Design Patterns - dziwnie napisana książka, ale łatwo przyswajalna. Nie jest zła...

[TomASS]

1.

Brakuje takich metod jak getUserName() oraz getUserPass()

Ja bym to załatwił metodami magicznymi __get i __set.

2.
co to będzie jak metodę zawierającą ten kod:

[PHP] pobierz, plaintext 
<?php
if ($this->dostep != TRUE){
			header("Location: accessDenied.php");
			exit;
 }
?>
[PHP] pobierz, plaintext 

wywoła się po wysłaniu nagłówka do przeglądarki? Spowoduje błąd. Tak jak podpowiadają koledzy, lepszym rozwiązaniem w tym wypadku będą wyjątki.

3.
metody też mogą być public/protected/private

4.

[PHP] pobierz, plaintext 
<?php
$q = "SELECT grupa FROM uzytkownicy WHERE BINARY login='".$this->login."'
?>
[PHP] pobierz, plaintext 

A co będzie jeśli w $this->login będzie login' OR 1 ? Mam nadzieję, że to gdzieś sprawdzasz (np. przy pomocy mysql_escape_string)
Zamiast w sesji przekazywać zmienne do klasy - przekazuj je poprzez konstruktora.

5.
Troszkę dla mnie dziwne jest pisanie metody, która ma za zadnie sprawdzenie czy użytkownik ma dostęp i jeśli nie to przładowuje się na inną stronę, a jeśli ma dostęp to "puszcza" skrypt.

Masz teraz tak:

[PHP] pobierz, plaintext 
<?php
$uzytkownik = new Uzytkownik();  //1
$uzytkownik->sprawdzDostep(0, "1 2");  //2
?>
Tajna tresc
[PHP] pobierz, plaintext 

Jeśli brak dostępu to w //2 przejdzie do innej strony.
Jeśli jest dostęp to "puści"

Ja bym to zrobił raczej tak, że metoda sprawdzDostep() zwraca czy jest dostęp czy brak dostępu:

[PHP] pobierz, plaintext 
<?php
$uzytkownik = new Uzytkownik();
if($uzytkownik->sprawdzDostep(0, "1 2")){
   echo 'Tajna treść';
}
else{
  echo 'Brak dostępu';
}
?>
[PHP] pobierz, plaintext 

Na pierwszą funkcję jest na prawdę dobrze Czekamy na jeszcze

Ten post edytował TomASS 17.07.2006, 22:49:42

[-Gość-]

Ok, wszystko dokladnie przeczytalem. Wlasnie chodzilo mi o to jak to wszystko zgrabnie napsiac (ten podzial na 3 klasy - wlasnie o takie rady mi chodzilo!). Tak jak pisalem - jest to 1 klasa i w zasadzie wszystko bym do niej wpakowal a wiem ze nie tedy droga.

Czy to tak wiele kosztuje przekazanie $_SESSION["user"] i "pass" do konstruktora

hmmm a to tak nie jest teraz?
Co do tego ze exit nie sa zbyt mile widziale w klasach - nie wiedzialem, dziekuje za info. Filtrowanie danych itd. na razie nie ma bo jak widac to klasa testowa z danymi umieszczonymi przeze mnie w kodzie wiec tym sie na razie zajmowal nie bede. Chodzi mi o sama klase. Natomiast co to samego dzialania skryptu:

[PHP] pobierz, plaintext 
<?php
 
$uzytkownik = new Uzytkownik();
 
if($uzytkownik->sprawdzDostep(0, "1 2")){
 
   echo 'Tajna treść';
 
}
 
else{
 
  echo 'Brak dostępu';
 
}
 
?>
[PHP] pobierz, plaintext 

Chcialem wlasnie to zrobic w jak najkrotszy sposob, zeby nie trzeba bylo wstawiac duzo kodu na kazdej chronionej podstronie. Stad takie rozwiazanie z exitem w srodku...

[phpion]

Znowu mnie kurAW wylogowalo


---
Jak się kulturalnie odezwać nie potrafisz to nic nie pisz.
To jest forum publiczne a nie ławka pod blokiem.
Następnym razem dostaniesz ostrzeżenie i zakaz postowania na jakiś czas.
~mike_mech

[Cysiaczek]

@phpion - Nie - teraz $_SESSION nie jest przekazana, tylko użyta w konstruktorze. Przekazać trzeba jako argument konstruktora.

[phpion]

No racja

[acztery]

ja bym popracował nad nazewnictwem zmiennych , klass itp itd

[phpion]

Tzn? wszystko mam jechac po angielsku?

[Cysiaczek]

Niekonieczne, choć lepiej , bo ktoś z zagranicy będzie mógł odczytać twój kod. Poza tym szlifujesz w ten sposób angielski, co samo w sobie jest dobre

[nasty]

i ja dozuce swoje 5 groszy:

Skorzystaj z okazji ze jeszcze nie masz zlych przyzwyczajen i nabiez dobrych, jak np komentowanie wszystkiego w formacie phpDoc
pozdrawian