php-resource



Zurück   PHP-Scripte PHP-Tutorials PHP-Jobs und vieles mehr > Entwicklung > PHP Developer Forum
 

Login

 
eingeloggt bleiben
star Jetzt registrieren   star Passwort vergessen
 

 

 


PHP Developer Forum Hier habt ihr die Möglichkeit, eure Skriptprobleme mit anderen Anwendern zu diskutieren. Seid so fair und beantwortet auch Fragen von anderen Anwendern. Dieses Forum ist sowohl für ANFÄNGER als auch für PHP-Profis! Fragen zu Laravel, YII oder anderen PHP-Frameworks.

Antwort
 
LinkBack Themen-Optionen Thema bewerten
  #1 (permalink)  
Alt 19-01-2008, 17:38
carapau
 Registrierter Benutzer
Links : Onlinestatus : carapau ist offline
Registriert seit: Aug 2005
Ort: Bielefeld
Beiträge: 347
carapau ist zur Zeit noch ein unbeschriebenes Blatt
Standard Bitte einmal meinen Codestil kritisieren! :)

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!

PHP-Code:
// 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;
    }

PHP-Code:
// 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); 
Code:
// 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

__________________
Lasst euch nicht lumpen, hoch den Humpen!
Mit Zitat antworten
  #2 (permalink)  
Alt 19-01-2008, 18:23
Hopka
 PHP Expert
Links : Onlinestatus : Hopka ist offline
Registriert seit: May 2003
Ort: Köln
Beiträge: 2.172
Hopka ist zur Zeit noch ein unbeschriebenes Blatt
Hopka eine Nachricht über ICQ schicken
Standard

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?
__________________
hopka.net!
Mit Zitat antworten
  #3 (permalink)  
Alt 19-01-2008, 18:29
tontechniker
 PHP Senior
Links : Onlinestatus : tontechniker ist offline
Registriert seit: Jul 2005
Beiträge: 1.972
tontechniker ist zur Zeit noch ein unbeschriebenes Blatt
Standard

[list=1][*]PHP 5 benutzen (Gültigkeitsbereich von Variablen usw.)[*]Hässlich:
PHP-Code:
global Smarty
Globale Variablen (und Smarty, aber das ist Geschmackssache)
PHP-Code:
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)
PHP-Code:
if(!empty($id
Hier fehlt eine Klammer
PHP-Code:
$newsItems[] = (array) new News($newsId); 
Was soll das denn? Dezidierte Methoden zu Verfügung stellen[/list=1]Ansonsten: ack@Hopka
__________________
Die Regeln | rtfm | register_globals | strings | SQL-Injections | []
Mit Zitat antworten
  #4 (permalink)  
Alt 19-01-2008, 18:29
IchBinIch
 Registrierter Benutzer
Links : Onlinestatus : IchBinIch ist offline
Registriert seit: Apr 2003
Beiträge: 324
IchBinIch ist zur Zeit noch ein unbeschriebenes Blatt
Standard

Zitat:
$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.
__________________
ICH BIN ICH!!!
Mit Zitat antworten
  #5 (permalink)  
Alt 19-01-2008, 18:52
frankburian
 Guest
frankburian
Beiträge: n/a
Standard

: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 ... {
...
}
Mit Zitat antworten
  #6 (permalink)  
Alt 20-01-2008, 18:01
carapau
 Registrierter Benutzer
Links : Onlinestatus : carapau ist offline
Registriert seit: Aug 2005
Ort: Bielefeld
Beiträge: 347
carapau ist zur Zeit noch ein unbeschriebenes Blatt
Standard

Zitat:
Original geschrieben von tontechniker
[list=1][*]PHP 5 benutzen (Gültigkeitsbereich von Variablen usw.)[*]Hässlich:
PHP-Code:
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.
__________________
Lasst euch nicht lumpen, hoch den Humpen!
Mit Zitat antworten
  #7 (permalink)  
Alt 20-01-2008, 18:06
carapau
 Registrierter Benutzer
Links : Onlinestatus : carapau ist offline
Registriert seit: Aug 2005
Ort: Bielefeld
Beiträge: 347
carapau ist zur Zeit noch ein unbeschriebenes Blatt
Standard

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
PHP-Code:
// 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
PHP-Code:
// news.php
require_once 'news.class.php';
global 
$Smarty;
$News     = new News();

$template 'bla.htm';
$newsItems $News->getNews(030$_GET['id']);
$Smarty->assign("newsItems"$newsItems);
$Smarty->display($template); 
Gruß
carapau
__________________
Lasst euch nicht lumpen, hoch den Humpen!
Mit Zitat antworten
  #8 (permalink)  
Alt 20-01-2008, 18:42
3DMax
 PHP Senior
Links : Onlinestatus : 3DMax ist offline
Registriert seit: Jan 2004
Beiträge: 1.916
3DMax ist zur Zeit noch ein unbeschriebenes Blatt
Standard

Zitat:
Original geschrieben von carapau
PHP-Code:
        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.
PHP-Code:
$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.
Mit Zitat antworten
  #9 (permalink)  
Alt 20-01-2008, 20:03
tontechniker
 PHP Senior
Links : Onlinestatus : tontechniker ist offline
Registriert seit: Jul 2005
Beiträge: 1.972
tontechniker ist zur Zeit noch ein unbeschriebenes Blatt
Standard

Zitat:
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.
PHP-Code:
        $qry "SELECT * FROM _tbl_news WHERE status = '$status'"
Trennung von Strings und Variablen und SQL Injections ...
Zitat:
Werd mich mal um PHP5 kümmern - Buch hab ich hier.
Funktionen dürfen auch Gültigkeitsbereiche bekommen (siehe public/protected/private).
__________________
Die Regeln | rtfm | register_globals | strings | SQL-Injections | []
Mit Zitat antworten
  #10 (permalink)  
Alt 21-01-2008, 02:08
carapau
 Registrierter Benutzer
Links : Onlinestatus : carapau ist offline
Registriert seit: Aug 2005
Ort: Bielefeld
Beiträge: 347
carapau ist zur Zeit noch ein unbeschriebenes Blatt
Standard

Zitat:
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


Zitat:
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
__________________
Lasst euch nicht lumpen, hoch den Humpen!
Mit Zitat antworten
  #11 (permalink)  
Alt 21-01-2008, 04:05
combie
 PHP Expert
Links : Onlinestatus : combie ist offline
Registriert seit: May 2006
Beiträge: 3.296
combie wird schon bald berühmt werden
Standard

Zitat:
Kannst du vielleicht eine posten, die mich inspirieren könnte?
Hier: oop class extends vererbung / referenz habe ich mal eine rudimentäre Klasse gepostet...

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

Geändert von combie (21-01-2008 um 04:08 Uhr)
Mit Zitat antworten
Antwort

Lesezeichen


Aktive Benutzer in diesem Thema: 1 (Registrierte Benutzer: 0, Gäste: 1)
 

Themen-Optionen
Thema bewerten
Thema bewerten:

Forumregeln
Es ist Ihnen nicht erlaubt, neue Themen zu verfassen.
Es ist Ihnen nicht erlaubt, auf Beiträge zu antworten.
Es ist Ihnen nicht erlaubt, Anhänge hochzuladen.
Es ist Ihnen nicht erlaubt, Ihre Beiträge zu bearbeiten.

BB-Code ist an.
Smileys sind an.
[IMG] Code ist an.
HTML-Code ist aus.
Trackbacks are an
Pingbacks are an
Refbacks are an


PHP News

ebiz-trader 7.5.0 mit PHP7 Unterstützung veröffentlicht
ebiz-trader 7.5.0 mit PHP7 Unterstützung veröffentlichtDie bekannte Marktplatzsoftware ebiz-trader ist in der Version 7.5.0 veröffentlicht worden.

28.05.2018 | Berni

Wissensbestand in Unternehmen
Wissensbestand in UnternehmenLebenslanges Lernen und Weiterbilden sichert Wissensbestand in Unternehmen

25.05.2018 | Berni


 

Aktuelle PHP Scripte

ADSMAN V3 - Werbe-Manager ansehen ADSMAN V3 - Werbe-Manager

ADSMAN V3 - mehr als nur ein Bannermanager! Banner, Textanzeigen und PagePeel Manager! Mit ADSMAN PRO haben Sie die Marketinglösung für eine effektive und effiziente Werbeschaltung mit messbaren Ergebnissen. Unterstützt werden Bannerformate in beliebi

25.10.2018 virtualsystem | Kategorie: PHP/ Bannerverwaltung
PHP News und Artikel Script V2

News schreiben, verwalten, veröffentlichen. Dies ist jetzt mit dem neuen PHP News & Artikel System von virtualsystem.de noch einfacher. Die integrierte Multi-User-Funktion und der WYSIWYG-Editor (MS-Office ähnliche Bedienung) ermöglichen...

25.10.2018 virtualsystem | Kategorie: PHP/ News
Top-Side Guestbook

Gästebuch auf Textbasis (kein MySQL nötig) mit Smilies, Ip Sperre (Zeit selbst einstellbar), Spamschutz, Captcha (Code-Eingabe), BB-Code, Hitcounter, Löschfunktion, Editierfunktion, Kommentarfunktion, Kürzung langer Wörter, Seiten- bzw. Blätterfunktion, V

22.10.2018 webmaster10 | Kategorie: PHP/ Gaestebuch
 Alle PHP Scripte anzeigen

Alle Zeitangaben in WEZ +2. Es ist jetzt 03:48 Uhr.