Klasa: koszyk, prosba o spojrzenie doswiadczonym okiem Opcje

[phpion]

Chcialbym dac wam pod ostrzal moja druga klasunie. Tym razem ma to byc koszyk zakupow.
Dane koszyka trzymane sa w tablicy z $_SESSION['koszyk'][ID_ELEMENTU] a wartosc poszczegolnej komorki okresla ZAMOWIONA_ILOSC. Np.

[PHP] pobierz, plaintext 
<?php
$_SESSION['koszyk'][1] = 20;
$_SESSION['koszyk'][2] = 10;
?>
[PHP] pobierz, plaintext 

Nie wiedzialem za bardzo jak rozwiazac kwestie pobierania danych o towarach bedacych w koszyku wiec getData() i polaczenie ze SQL w niej jest tymczasowe .
Oto moja klasa:

[PHP] pobierz, plaintext 
<?php
class Basket
{
	public $basket;
	public $sum;
	public $countItems;
	public $basketHandler;
 
	public function Basket()
	{
		$this->__construct();
	}
 
	public function __construct()
	{
		$this->basket = Array();
		$this->basketHandler = &$_SESSION['koszyk'];
	}
 
	public function addItem($itemID, $itemAmount)
	{
		if (isset($this->basketHandler[$itemID]))
			$this->basketHandler[$itemID] += $itemAmount;
		else
			$this->basketHandler[$itemID] = $itemAmount;
	}
 
	public function getData()
	{
		$arrayTemp = Array();
		$this->sum = 0;
 
		if (count($this->basketHandler) > 0)
			foreach ($this->basketHandler as $index => $value)
				$arrayTemp[] = $index;
 
		if (count($arrayTemp) > 0)
		{
			$sql = mysql_connect("localhost", "root", "");
			mysql_select_db("sklep", $sql);
 
			$query = "SELECT id, nazwa, cena, vat FROM produkty WHERE id IN (".implode(", ", $arrayTemp).")";
			$query = mysql_query($query);
 
			if (mysql_num_rows($query) > 0)
			{
				$i = 0;
 
				while ($record = mysql_fetch_row($query))
				{
					$this->basket[$i]['id'] = $record[0];
					$this->basket[$i]['name'] = $record[1];
					$this->basket[$i]['amount'] = $this->basketHandler[$record[0]];
					$this->basket[$i]['vat'] = $record[3];
					$this->basket[$i]['netPrice'] = number_format($record[2], 2);
 
					$this->basket[$i]['grossPrice'] = number_format(($this->basket[$i]['netPrice'] + $this->basket[$i]['netPrice'] * $this->basket[$i]['vat'] / 100), 2);
 
					$this->basket[$i]['netValue'] = number_format(($this->basket[$i]['netPrice'] * $this->basket[$i]['amount']), 2);
 
					$this->basket[$i]['grossValue'] = number_format(($this->basket[$i]['grossPrice'] * $this->basket[$i]['amount']), 2);
 
					$this->sum += $this->basket[$i]['grossValue'];
 
					$i++;
				}
 
				$this->countItems = $i;
			}
		}
	}
 
	public function showBasket()
	{
		if ($this->countItems() > 0)
		{
			$return = "<table border="1">n";
			$return .= "<tr><td>Nazwa:</td><td>Cena netto:</td><td>VAT:</td><td>Cena brutto:</td><td>Ilosc:</td><td>Wartosc netto:</td><td>Wartosc brutto:</td></tr>n";
 
			foreach ($this->basket as $index => $value)
				$return .= "<tr><td>".$this->basket[$index]['name']."</td><td>".$this->basket[$index]['netPrice']."</td><td>".$this->basket[$index]['vat']."</td><td>".$this->basket[$index]['grossPrice']."</td><td>".$this->basket[$index]['amount']."</td><td>".$this->basket[$index]['netValue']."</td><td>".$this->basket[$index]['grossValue']."</td></tr>n";
 
			$return .= "<tr><td colspan="6" style="text-align: right;">Do zaplaty:</td><td>".$this->sum."</td></tr>n";
			$return .= "</table>";
 
			return $return;
		}
	}
 
	public function countItems()
	{
		$return = (isset($this->countItems)) ? $this->countItems : count($this->basketHandler);
 
		return $return;
	}
 
	public function getSum()
	{
		if (!isset($this->sum))
			$this->getData();
 
		return $this->sum;
	}
 
	public function emptyBasket()
	{
		foreach ($this->basketHandler as $index => $value)
			unset($this->basketHandler[$index]);
	}
}
?>
[PHP] pobierz, plaintext 

Prosze o porady i sugestie co w niej zmienic, co dopisac itp. Z gory serdeczne dzieki!

[dr_bonzo]

1. koszyk nie powinien sie wyswietlac tylko zwracac swoja zawartosc (mieszasz wyglad z logika biznesowa)
2. "public function Basket()" jest niepotrzebne, skoro kod ruszy tylko na php5 to pierwszy zostanie wywolany konstruktor __construkt (Basket() bylby uruchamiany gdy ten pierwszy nie istnial)
3. "getData()" ma zla nazwe, metoda nic nie zwraca a zajmuje sie (jak pobierznie patrzylem) tylko obliczaniem sumy (czegostam -- no wlasnie czego, nazwa $sum nic nie mowi)
4. emptyBasket()
nie prosciej
$this->basketHandler = array()?

5. patrz:
dodajesz itemy do koszyka
pobierasz (i liczysz) sume
dodajesz jednego itema
pobierasz sume -- ktora sie NIE ZMIENILA!!

[phpion]

1. No wlasnie koszyk 'zwracany' jest przez getData, a wyswietlany dopiero przez showBasket(). Jakos inaczej to zrobic? Bo w sumie chyba jest rozbite na wczytanie danych oraz ich wyswietlenie.
2. W sumie racja
3. getData() zajmuje sie pobraniem wszelkich danych dla koszyka, tj. nazw produktow, ich cen, obliczania cen brutto, wartosci netto i brutto oraz sumy zamowienia. Dane przypisuje do obiektu koszyka.
4. W zasadzie prosciej
5. No ok, tu sie zgodze, ale przeciez i tak w praktyce dodanie kazdego itemu laczy sie z przeladowaniem strony. Dlatego jest jak jest. Racja: przy recznym wykonywanie metod w kodzie suma sie nie zmieni. No ale jak juz wspomnialem - chyba nie ma to znaczenia?

[dr_bonzo]

5. Ale nie zmienia to faktu ze jest blad w kodzie i tak nie powinno byc, wykorzystujesz tutaj hacka : takiego ze tylko raz bedziesz dodawal przedmiot i sprawdzal ceny.
3. (getData())To rozbij ja na poszczegolne metody zajmujace sie pojedynczym problemem

[PHP] pobierz, plaintext 
<?php
if (count($this->basketHandler) > 0)
			foreach ($this->basketHandler as $index => $value)
				$arrayTemp[] = $index;
?>
[PHP] pobierz, plaintext 

if (count()) jest niepotrzebne, bo gdy tablica jest pusta to foreach nie wykona sie (wykona sie zero razy)


6*. a co jest w $record[0]; ? lepiej uzyc mysql_fetch_assoc i odwoywac sie $record[ 'id' ] itd.

7*. $this->basketHandler lespiej zmieniec nazwe na $products, $items -- bardziej odpowiada temu co przechowuje; chociarz juz sie gubie -- czym jest $basket a czym $basketHandler

[acztery]

zrob tak zeby koszyk zwracał tablice array wtedy mozesz w zawsze zmienic sam wyglad koszyka np w jakims systamie szablonow