Bitte einmal meinen Codestil kritisieren! :)

[carapau]

Hey Leute!

Ich möchte meinen Codestil "perfektionieren". Ich kann PHP soweit, dass alles funktioniert usw, aber ich möchte optimale Lösungen bei minimalem Code. Hab mir PHP selbst (Bücher, Inet) beigebracht und deshalb nicht wirklich nach einer Norm gelernt.

Hier ein abgespecktes Beispiel (News), welches ihr gerne zerfleischen könnt!

// news.class.php
class News {
    var $id, $headline, $content;

    // füllt die vars 
    function News($id = "") {
        if(!empty($id) $ext = " WHERE id='$id'";

        $qry = mysql_query("SELECT id, headline, content FROM news".$ext);

        while($row = mysql_fetch_object($qry)) {
            // Frage 1: Geht die Zuweisung nicht schöner/eleganter?  Es könnten ja 100 Werte statt 3 sein.            
            $this->id    = $row->id;                
            $this->headline = $row->headline;
            $this->content  = $row->content;
        }
        
    }

    // gibt die letzten x news ids zurück 
    function getLatestNews($start, $num) {
        $qry = mysql_query("SELECT id FROM news LIMIT $start, $num");
    while($row = mysql_fetch_assoc($qry))
         $items[] = $row[id];

    return $items;
    }
} 

// news.php
require_once('news.class.php');
global Smarty;
$News = new News();

$start = 0;
$newsPerPage = 30;

$template = 'default.htm';

$newsIds = $News->getlatestNews($start, $newsPerPage);
foreach($newsIds as $newsId) {
    $newsItems[] = (array) new News($newsId);
}

$Smarty->assign('newsItems', $newsItems);
$Smarty->display($template); 

// default.htm (Template)
{foreach item=news from=$newsItems}
    <h2>{$news.headline}</h2>
    {$news.content}<br>
    [<a href="index.php?m=news&id={$news.id}">mehr</a>]
    <hr>
{/foreach}

Was haltet ihr von dem Stil? Falls euch sonst einige typische Fehler einfallen, immer raus damit!

Danke
carapau

[Hopka]

Vorneweg: Du benutzt noch PHP 4 Syntax. Solltest du nicht machen, es sei denn du wirst dazu gezwungen.

Du liest erst alle IDs aus, die du brauchst und holst dann danach für jede ID mit je einer zusätzlichen Abfrage die zusätzlichen Infos, also 31 Datenbankabfragen für 30 News. Das ist vermutlich die zweitschlechteste Lösung, die es überhaupt gibt. Du solltest mit einer Abfrage alle benötigten Infos holen.

Die Schleife im Konstruktor ist auch unnötig, da sowieso immer nur höchstens 1 Wert zurückkommt (bzw. zurückkommen sollte).

Was völlig fehlt, ist das Abfangen von Fehlern (z.B. können Datenbankabfragen auch mal "einfach so" fehlschlagen).

Die ID wird ungeprüft an die Datenbank geschickt. Es ist zwar aus diesem Code hier nicht direkt ersichtlich, aber möglicherweise setzt du die Klasse noch woanders ein und übergibst dort User-Eingaben (Stichwort SQL-Injection).


Und noch ein paar nicht ganz so schlimme Dinge:

Du solltest dir mal phpDocumentor ansehen, wenn du deine Klassen / Methoden kommentieren willst. Das hat sich mehr oder weniger als Standard etabliert.

Viele Menschen (so auch ich) bevorzugen es, nach Kontrollstrukturen immer geschweifte Klammern zu setzen, auch wenn nur eine Anweisung folgt. Aber das ist Geschmackssache.

Was auch noch nicht so optimal ist, ist dein Einsatz von Objekten. Du scheinst die Variablen der News-Klasse nur zu benötigen, weil du das Objekt später in ein Array umwandelst. Ich würde hier z.B. nur ein News-Objekt erzeugen und mir von dem dann alle einzelnen News zurückgeben lassen. Damit ließe Sich auch Frage 1 beantworten, nämlich gar nicht mehr für alle Spalten einzelne Variablen anlegen sondern die Zeilen aus der DB als Arrays zu behalten.

Die Einrückung bei getLatestNews ist etwas schief, aber das könnte auch am Forum liegen. Außerdem fehlt bei global Smarty; ein $. Warum benutzt du da überhaupt global?

[tontechniker]

[list=1][*]PHP 5 benutzen (Gültigkeitsbereich von Variablen usw.)[*]Hässlich:

global Smarty; 

Globale Variablen (und Smarty, aber das ist Geschmackssache)

while($row = mysql_fetch_object($qry)) {       
            $this->id    = $row->id; 

Zuweisung und Schleife (Daten als Array holen und über foreach zuweisen, bei nur einem gewünschten Wert keine Schleife)

if(!empty($id) 

Hier fehlt eine Klammer

$newsItems[] = (array) new News($newsId); 

Was soll das denn? Dezidierte Methoden zu Verfügung stellen[/list=1]Ansonsten: ack@Hopka

[IchBinIch]

$qry = mysql_query("SELECT id, headline, content FROM news".$ext);

Fehlende Fehlerprüfung wurde schon angesprochen. Darüber hinaus ist auch sonstiges Debuggen (z.B. Ausgabe der Query) unmöglich. Außerdem trägt die Variable $qry den falschen oder einen missverständlichen Namen. Sie enthält nämlich keinesfalls eine Query, sondern ein Result.

Require ist eigentlich keine Funktion, daher sind Klammern überflüssig.

[frankburian]

:O) noch ein tipp, binde die Datentypen vor den Variablenname an, so dass du immer weißt welche Variable welchen Typ enthält. so mach ich es zu min.

$arrArray
$strText
$intZahl
$boolWahrOderFalsch
$objObjekt

und ich finde es wesentlichen übersichlicher, wenn Du

function ...
{
...
}

schreibst, anstatt

function ... {
...
}

[carapau]

Original geschrieben von tontechniker
[list=1][*]PHP 5 benutzen (Gültigkeitsbereich von Variablen usw.)[*]Hässlich:

global Smarty; 

Globale Variablen (und Smarty, aber das ist Geschmackssache)[PHP][/list=1]

Werd mich mal um PHP5 kümmern - Buch hab ich hier.

Warum ist "global Smarty;" häßlich? Es geht leider nicht anders, da in einer anderen Datei Einstellungen für Smarty festgelegt werden, welche übernommen werden sollen.

[carapau]

Erstmal vielen Dank für Eure Antworten! Auch wenn ich jetzt nicht auf jede Antwort eingehe, hab ich mir jede aufmerksam durchgelesen. Hab das Beispiel mal überarbeitet, was haltet ihr davon?

Grundsätzlich: Hab Sicherheitsabfragen usw aus Übersichtsgründen rausgenommen. Geht in erster Linie um Performance und "den Weg".

# news.class.php

// news.class.php
class News {
    
    function getNews($start, $num, $id = "") 
    {
        $qry = "SELECT * FROM _tbl_news WHERE status = '$status'";        
        if(!empty($id)) 
        {
            $ext .= " AND id='$id'";            
        } 
        
        $result = mysql_query($qry.$ext);
        for($i = 0; $i < mysql_num_rows($result); $i++) {
            foreach(mysql_fetch_assoc($result) as $field => $key) {
                $newsItem[$field] = $key;
            }
            $newsItems[] = $newsItem;    
        }

        return $newsItems;
    }
} 

# news.php

// news.php
require_once 'news.class.php';
global $Smarty;
$News     = new News();

$template = 'bla.htm';
$newsItems = $News->getNews(0, 30, $_GET['id']);
$Smarty->assign("newsItems", $newsItems);
$Smarty->display($template); 

Gruß
carapau

[3DMax]

Original geschrieben von carapau

        for($i = 0; $i < mysql_num_rows($result); $i++) {
            foreach(mysql_fetch_assoc($result) as $field => $key) {
                $newsItem[$field] = $key;
            }
            $newsItems[] = $newsItem;    
        } 

die innere foreach-schleife ist irgendwie unsinnig. das array kannst du auch gleich direkt zuweisen.

$newsItems = array();

while(is_array($newsItem = mysql_fetch_assoc($result)))
{
 $newsItems[] = $newsItem;
} 

die db-funktionalität würde ich in eine separate datenbank-klasse auslagern. $db->fetchRow oder $db->fetchAllRows brauchst du doch auch an anderen stellen immer wieder.

[tontechniker]

Warum ist "global Smarty;" häßlich? Es geht leider nicht anders, da in einer anderen Datei Einstellungen für Smarty festgelegt werden, welche übernommen werden sollen.

Globale Variablen sind unschön da du nie weißt wo sie noch geändert wurden und sie undefinierte Gültigkeitsbereiche haben. Es gibt eigentlich immer eine andere Lösung.

        $qry = "SELECT * FROM _tbl_news WHERE status = '$status'"; 

Trennung von Strings und Variablen und SQL Injections ...

Werd mich mal um PHP5 kümmern - Buch hab ich hier.

Funktionen dürfen auch Gültigkeitsbereiche bekommen (siehe public/protected/private).

[carapau]

Original geschrieben von 3DMax
die db-funktionalität würde ich in eine separate datenbank-klasse auslagern. $db->fetchRow oder $db->fetchAllRows brauchst du doch auch an anderen stellen immer wieder. [/B]

Hatte angefangen mir eine MySQL Klasse zu schreiben, aber irgendwie nicht wirklich Unterschiede gemerkt. Kannst du vielleicht eine posten, die mich inspirieren könnte? Danke

Original geschrieben von tontechniker
Globale Variablen sind unschön da du nie weißt wo sie noch geändert wurden und sie undefinierte Gültigkeitsbereiche haben. Es gibt eigentlich immer eine andere Lösung.[/B]

Naja, bei dieser Variable weiss ich 100% wo sie gebraucht wird.

Danke für die Links.

Neue "Version" poste ich nachher oder morgen. Danke nochmal für die Hilfe

[combie]

Kannst du vielleicht eine posten, die mich inspirieren könnte?

Hier: http://www.php-resource.de/forum/sho...&postid=557961 habe ich mal eine rudimentäre Klasse gepostet...

Aber du könntest besser was moderneres/fertiges benutzen, z.B. PDO, oder gar mit doctrine