kritik an einer mysql klasse

[iberican]

Hallo,
ici habe eine Klasse geschrieben, mit welcher ich simple Abfragen mache. Nun würde ich gerne eure Kritik zu diesem pattern hören, was euch gefällt oder auch nicht. Freue mich über Anregungen

<?php  class dbWork{      
public function query($sql) {         
$this->sql = $sql;         
$this->res = mysql_query($this->sql);     

if(!$this->res){ 
throw new Exception(mysql_error() . mysql_errno());     
}         
return $this->res;     
}          

public function fetch(){         

if(mysql_num_rows($this->res) === 0){         
    
throw new Exception('Query results 0 rows');         

} 
return $this->res;     }      

    
public function getField($fName){     

$this->fName = $fName;         
$this->tmp = mysql_fetch_assoc($this->fetch());     
    
if(!array_key_exists($this->fName, $this->tmp)){         
    
throw new Exception("array key doest not exist");         }         
return $this->tmp[$this->fName];               
}          

public function getRows(){         
return mysql_fetch_assoc($this->fetch());     
}           
    
public function getFieldNames(){     
    
$this->res = $this->fetch();         
$this->assoc = mysql_fetch_array($this->res);     
$this->numR = mysql_num_fields($this->res);     
$this->fieldNames = array();                      
for($i=0; $i<$this->numR; $i++){     
            
$this->fieldName[$i] =mysql_field_name($this->res,$i);        
                     array_push($this->fieldNames,$this->fieldName[$i]);     

}     
return $this->fieldNames;         
}      
}
 ?>

[Koala]

wo ist denn der Konstruktor ?
Wo sind host, username, passwort ?
Findet der connect außerhalb der Klasse statt ??

[Griecherus]

Hier mal ein paar spontane, sehr grundlegende Punkte:

- Der Klassenname ist IMO ungünstig Schrägstrich nicht besonders aussagekräftig (besser: Database, DatabaseConnection bzw. DbConnection, DatabaseResult bzw. DbResult)
- Fehlende Deklaration der Klassenvariablen
- Manche der Klassenvariablen scheinen nutzlos (wozu $this->sql, wo es nirgends sonst gebraucht wird?)
- die Klasse bietet keine Möglichkeit, eine Verbindung zur Datenbank herzustellen
- deine Klasse könnte sich um SQL-Injection Prävention kümmern
- Fehlende Einrückung des Codes

P.S. Ich habe bewusst nicht auf etablierte Alternativen wie PDO hingewiesen, die das, was du versuchst, besser können. Ich gehe davon aus, dass die Klasse für Lernzwecke gedacht ist.

Grüße

[h3ll]

Ich finds nutzlos. Die Funktionen würde ich nie brauchen.

Außerdem gibts schon eine fertige MySQL-Klasse in PHP: mysqli

[Griecherus]

Original geschrieben von h3ll
Ich finds nutzlos. Die Funktionen würde ich nie brauchen.

Außerdem gibts schon eine fertige MySQL-Klasse in PHP: mysqli

Ich habe irgendwo mal gelesen, dass es mit Mysqli's derzeitiger(?) Implementierung Probleme mit Prepared Statements geben soll. Angabe ist natürlich ohne Gewähr. Ich würde ohnehin eher zu PDO raten.

Grüße

[iberican]

danke für die kritiken. auf die punkte bgzl. sinhhafitgkeit zwecks alternativen gehe ich nicht weiter ein. wie Griecherus schon richtig erkannt hat, es handelt sich um eine übung.

@Koala
die connection wird in einer anderen klasse behandelt, daher kein user etc.

die klasse soll nur dazu dienen angeforderte daten beritzustellen.

[martin_duisburg]

guten morgen

1. Warum benutzt du die mysql Funktionen?

2. du schreibst die Connection wird in einer anderen Klasse gesetzt, aber ich sehe nirgendwo eine Abfrage die verhindert das die Klasse ohne geöffente Mysql Verbindung auf gerufe wird.

3. Benutzt die Exceptions inflationär, ich bin nicht sicher ob es Sinn macht z.B.

public function fetch(){         

if(mysql_num_rows($this->res) === 0){         
    
throw new Exception('Query results 0 rows');         

} 
return $this->res;     } 

eine Exception zu werfen, da es durchaus auch Ergebnisse mit keinem 0 results geben kann oder? dann Liegt aber keine Ausnahme sondern ein korrektes Verhalten vor.

4. wie bereits angemerkt findet keine initalisierung von member Variablen statt, ist zwar nicht nötig, wirft dir aber städig notice warnungen und sollte schon aus übersichtsgründen so gemacht werden.

schönen Sonntag noch

[iberican]

@martin_duisburg

sehr schön die punkte gefallen mir und sind auch allesamt korrekt.
nur verstehe ich nicht ganz was du bei punkt 1 meinst?
zu 2). eindeutig nicht gut gelöst
zu 3) auch korrekt. hier ist die exception nicht sinnvoll

da ich ahand der aufrufe nun sehe, dass der beitrag wohl auf größeres interesse stößt fände ich es toll, wenn viel. der ein oder andere der sich animiert fühlt mal seinen ansatz zeigt....

[Griecherus]

Ich denke er meint mit 1., dass die mysql-Funktionen längst übeholt sind (MySql Improved - mysqli).

Wenn du nach Ansätzen suchst, dann schau dich doch mal bei den ganzen Database-Abstraction-Layern um, die's wie Sand am Meer gibt (Pear DB, Pear MDB2, Creole, etc.). Bei Creole wird es übrigens auch so gehandhabt, dass Connection und Resultset in verschiedene Klassen gekapselt sind.
Hier mal das Prinzip in sehr simpler Ausführung (keine abstrakte Klassen, Interfaces und dergleichen):

class DatabaseConnectionMysql
{
    protected $_connection = null;

    public function connect()
    {
        $this->_connection = mysql_connect();

        if (!$this->_connection)
        {
             throw new Exception('Failed to establish connection to database: '
                . mysql_error() . ':' . mysql_errno());
        }
    }

    public function query($query_string)
    {
        $result = mysql_query($query_string);
       
        if (!$result)
        {
             throw new Exception('Failed to execute query: ' . mysql_error() . ':' . mysql_errno());
        }

        return new DatabaseResultMysql($result, $this->_connection);
    }
}

class DatabaseResultMysql
{
    protected $_result = null;
    protected $_connection = null;

    public function __construct($result, $connection)
    {
        $this->_result = $result;
        $this->_connection = $connection;
    }

    public function __get($name)
    {
        switch ($name)
        {
            case 'num_rows':
                return mysql_num_rows($this->_result);
                break;
        }
    }
} 

$database = new DatabaseConnectionMysql();

try
{
    $database->connect();

    $result = $database->query("SELECT something");

    if ($result->num_rows == 0)
    {
        echo 'Sorry, nothing found.' . PHP_EOL;
    }
    else
    {

    }
}
catch (Exception $e)
{
    die($e->getMessage());
} 

Grüße

[combie]

mal seinen ansatz zeigt.

Naja, versprich dir aber nicht zuviel davon....
Hier dürfte evtl. das "subpackage DB" für dich von Interesse sein: http://www.combie.de/packages_doc/
Alles ein unfertiges Experiment...