Na samym początku wypada się przywitać co też czynię, jako że jestem świeżo po rejestracji na forum. Mam też nadzieję, że nie umknął mi żaden punkt regulaminu i na starcie nie popełniłem jakiegoś "wykroczenia" zamieszczając pierwszy post.

A teraz do rzeczy:

Od dzisiaj studiuję rozszerzenie PDO. Napisałem prostą klasę licznika odwiedzin odpornego na odświeżanie strony. Dane licznika przechowywane są w bazie danych. Konstruktor klasy tworzy nową tabelę jeśli takowa nie istnieje. Więcej szczegółów w komentarzach do metod.

Proszę doświadczonych programistów o:
1. ocenę kodu;
2. wskazówki na co zwracać uwagę;
3. ocenę pod kątem bezpieczeństwa (czy takie wykorzystanie PDO utrudnia SQL Injection?)
4. co można zoptymalizować;
5. opinie czy takie wykorzystanie funkcji unset jest korzystne/dobre, czy też zbędne/złe?
6. Co sądzicie o PDO? Czy pod względem wygody/wydajności/bezpieczeństwa jest lepsze od innych metod? Co polecacie?

Każda konstruktywna krytyka jest mile widziana.

Oto kod:

[PHP] pobierz, plaintext 
class Counter {
	private $db; //obiekt bazy danych
	private $counter; //zmienna zawierająca wartość licznika
 
	//Funkcja inicjuje połączenie z bazą danych, wrazie potrzeby tworzy tabelę i ustawia wartość
	//licznika. Ponadto ustawia cookie o nazwie visited na wartość true i czas życia 3h.
	function __construct($dbName, $user, $password, $host="localhost", $dbType="mysql") {
		//w tym miejscu powinny znajdować się instrukcję obsługi pozostałych typów baz danych
		$dsn = "$dbType:dbname=$dbName;host=$host;";
		try {
			$this->db = new PDO($dsn, $user, $password);
		} catch (PDOException $e) {
			echo "Nieudana próba połączenia: " . $e->getMessage();
		}	
 
		$checkIfTableExists = $this->db->prepare("SELECT * FROM licznik");
		if(!$checkIfTableExists->execute()) {
			$create = $this->db->prepare("CREATE TABLE licznik(Hits INT)");
			$create->execute();
			unSet($create);
 
			$insert = $this->db->prepare("INSERT INTO licznik VALUES(0)");
			$insert->execute();
			unSet($insert);
		}		
		unSet($checkIfTableExists);
 
		$select = $this->db->prepare("SELECT Hits FROM licznik");
		$select->execute();
 
		$result = $select->fetch(PDO::FETCH_ASSOC);
		unSet($select);
 
		if (isSet($_COOKIE['visited'])) { 
			$this->counter = $result['Hits'];
		} else {
			$this->counter = $result['Hits'] + 1;
 
			$update = $this->db->prepare("UPDATE licznik SET Hits=$this->counter");
			$update->execute();
			unSet($update);
 
			setCookie("visited", "true", time()+3600*3);
		}
		unSet($result);
	}
 
	//Funkcja umożliwia ustawienie licznia na dowolną wartość.
	function setCounter($val) {
		$this->counter = $val;
		$update = $this->db->prepare("UPDATE licznik SET Hits=$val");
		$update->execute();
		unSet($update);
	}
 
	//Funckja zwraca wartość licznika.
	function getCounter() {
		return $this->counter;
	}
 
	//Funkcja ustawia wartość licznika na 0.
	function resetCounter() {
		$reset = $this->db->prepare("UPDATE licznik SET Hits=0");
		$reset->execute();
		$this->counter = 0;
		unSet($reset);
	}
 
	//Destruktor klasy Counter.
	function __destruct() {
		unSet($this->db);
		unSet($this->counter);
	}
}
[PHP] pobierz, plaintext 

Sposób użycia:

[PHP] pobierz, plaintext 
$licznik = new Counter("nazwa_bazy", "user", "hasło", "host", "typ_bazy");
	echo "Liczba odwiedzin: " . $licznik->getCounter();
	unset($licznik);
[PHP] pobierz, plaintext 

Po co korzystasz z unset? Php ma gc, więc jeśli zmienna wyjdzie poza zakres to pamięć zostanie zwolniona - niepotrzebne zaciemnianie kodu.

Korzystasz z prepared statements, lecz w ogóle nie wykorzystujesz ich zalet. Masz na krzyż 4 zapytania, które różnią się tylko parametrem, przygotuj sobie 4 obiekty prepared statement dla każdego z zapytań i te obiekty wykorzystuj do wykonywania zapytań, a nie twórz ich non stop za każdym razem. Będziesz musiał wykorzystać placeholdery (np. w postaci znaku zapytania), a wartosci przekazywać do metody execute - tak też powinno się robić nawet gdy docelowo obiekt PDOStatement ma wykonać tylko jedno zapytanie.

Klasa counter nie powinna nawiązywać połączenia z bazą danych, a dostać już gotowe połączenie.

Dzięki za spostrzeżenia!



Czyli w tym konkretnym przypadku powinienem przygotować po jednym obiekcie dla zapytań select, update, insert, create? Czy tak?



W tym miejscu nie byłem pewien, czy Counter powinien sam łączyć się z bazą. Dzięki za wyjaśnienie! Czy powinienem przekazywać do niego gotowe połączenie w postaci obiektu PDO?


Jeszcze raz dzięki za odpowiedź i miłej niedzieli.

Chodzi o zapytania UPDATE:



Po co Ci tutaj prepare(), kiedy dane wklejasz bezpośrednio w zapytanie?

blad na bledzie

1 - dlaczego tworzysz nowe polaczenie w konstruktorze? nie lepiej wystawic set/get connection aby mozna bylo to zlaczyc z innym kodem?
2 - calkowity brak tranzakcji
3 - tworzenie tabeli w locie!?
4 - duplikacje kodu (brak DRY)
5 - nie robisz lockow na tabelach

ogolnie, licznik nadaje sie jedynie do prostych stronek o niklej ogladalnosci, przy wiekszym ruchu bedzie waskim gardlem a przy duzej konkurencyjnosci bedzie dawal zle wyniki

Nie wykorzystujesz tego co oferuje PDO, a mam tutaj na myśli dokładnie to co pisał Zyx.
Destruktor możesz wywalić.
resetCounter mógłby najwyżej wywoływać $this->setCounter(0), nie widzę sensu powielania kodu.
getCounter nie zwróci odpowiedniej wartości, jeżeli w czasie wykonywania skryptu kilku innych w tym samym czasie wejdzie na stronie i każdy z nich przesunie licznik o +1 ;]