Witam,

Chciałem prosić o opinię na temat takiej klasy. Klasa zapisuje dane o kursach walut NBP z pliku xml i odczytuje je z bazy danych. Nie wiem czy samo połącznie do bazy powinno być tak jak to zrobiłem czy może w osobnej klasie? Poza tym czy tej klasie coś brakuje lub jest w niej coś nie potrzebne?

[PHP] pobierz, plaintext 
<?php
 
	class kursy_walut
	{
		private $host;
		private $user;
		private $password;
		private $db;
		private $statement;
		private $count;
 
		function __construct($host,$user,$password,$db){
			$this->host = $host;
			$this->user = $user;
			$this->password = $password;
			$this->db = $db;
 
			$this->db = new PDO('mysql:host='.$host.';dbname='.$db.';',$user,$password);
		}
 
		function saveXMLToTheDatabase(){
 
			$walutyKursy = simplexml_load_file('http://rss.nbp.pl/kursy/xml2/2012/a/12a127.xml');
 
			$nrTabeli = $walutyKursy->numer_tabeli;
 
			foreach($walutyKursy->pozycja as $poz){
				$naz_wal = $poz->nazwa_waluty;
				$przel = $poz->przelicznik;
				$kod_wal = $poz->kod_waluty;
				$kurs_sr = $poz->kurs_sredni;
 
				$this->db->exec('INSERT INTO kursy_walut_nbp (numer,nazwa_wal,przel,kod_w,kurs_s) VALUES (\''.$nrTabeli.'\',\''.$naz_wal.'\',\''.$przel.'\',\''.$kod_wal.'\',\''.$kurs_sr.'\')');
 
			}
 
 
		}
 
		function readDataFromTheDatabase($val,$dat,$number){
 
			$statement = $this->db->query('SELECT nazwa_wal,przel,kod_w,kurs_s FROM kursy_walut_nbp');
 
      echo '<ul>';
      foreach($statement as $row){
				$count = $row['kurs_s'] * $val;
				echo '<li>'.$row['nazwa_wal'].' - '.$row['przel'].' - '.$row['kod_w'].' - '.$row['kurs_s'].' - '.$count.'</li>';
      }
			echo '</ul>';
		}
 
	}
 
 
	$db_con = new kursy_walut('localhost','root','ddd','baza1');
	$db_con -> saveXMLToTheDatabase();
	$db_con -> readDataFromTheDatabase(10.5,'2012-06-04','HNG/DMSD/436');
 
 
 
?>
 
[PHP] pobierz, plaintext 

proszę o podpowiedź.
dzięki

1. powienieneś przekazywać obiekt PDO w konstruktorze, a nie tworzyć go wewnątrz klasy
2. Bez sensu jest INSERT w foreach() to możesz dodać jednym zapytaniem podając zestawy danych po przecinku -> manual mysql
3. wewnątrz klasy nie powinno być echo, print itd, lepiej zwracaj to sobie w postaci tablicy i potem wyświetlaj
4. I najlepiej to używać prepared statements

co masz na myśli pisząc:

"1. powienieneś przekazywać obiekt PDO w konstruktorze, a nie tworzyć go wewnątrz klasy"

możesz napisać przykład jak to powinienem zrobić ?

dzięki

Po prostu przekazujesz do klasy zmienną $db, która jest już obiektem PDO, tym sposobem musiał byś w każdej klasie jaką użyjesz tworzyć oddzielną instancję klasy, a to jest mało wydajne i nie elastyczne

[PHP] pobierz, plaintext 
class kursy_walut
{
    private $db;
 
    function __construct(PDO $db)
    {
        $this->db = $db;
    }
}
[PHP] pobierz, plaintext 

nie widzę też potrzeby, żeby zmienna $statement była prywatną zmienną klasy, przeciesz nie potrzebujesz mieć do niej dostępu z innych metod funkcji

ok, dzięki ...

rozumiem, że takie wywołanie zostaje i jest dobrze :

[PHP] pobierz, plaintext 
$db_con = new kursy_walut('localhost','root','ddd','baza1');
[PHP] pobierz, plaintext 

Nie.. po co klasie kursy_walut Twoje hasła do bazy danych? Przecież ona zajmuje się walutami, a nie łączeniem z bazą, Instancje PDO masz stworzyć poza klasą i tylko przekazać obiekt, żeby można było wykonwać na bazie potrzebne operację.

[PHP] pobierz, plaintext 
$db = new PDO('mysql:host='.$host.';dbname='.$db.';',$user,$password);
 
$kursy = new kursy_walut($db);
 
[PHP] pobierz, plaintext 

Przyjęło się że nazwy klas piszemy z dużej litery w notacji CamelCase

@toffiak - z dużej - ok ale z tym camel-casem to przesadziłeś - gdzie się tak przyjęło? Jest wiele frameworków, bibliotek gdzie używa się "podłogi" a nie wielbłądka

Od siebie dodam, że jedyna własność klasy, która jest potrzebna to $db. Resztę można spokojnie wywalić.

Pętla while i masa zapytań po sobie też jest zbędna. Najlepiej zastąp to konstrukcją:

[SQL] pobierz, plaintext 
INSERT INTO ....... VALUES (...), (...), (...) ....... (...)
[SQL] pobierz, plaintext 

Metoda saveXMLToTheDatabase() powinna przyjmować w argumencie jaki plik XML ma pójść do bazy (jego adres) w razie nie podania można użyć domyślnego - staje się to bardziej uniwersalne.