Bitte einmal meinen Codestil kritisieren! :)

Einklappen
X
 
  • Filter
  • Zeit
  • Anzeigen
Alles löschen
neue Beiträge

  • 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!

  • #2
    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!

    Kommentar


    • #3
      [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 | [COLOR=silver][[/COLOR][COLOR=royalblue]–[/COLOR][COLOR=silver]][/COLOR]

      Kommentar


      • #4
        $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!!!

        Kommentar


        • #5
          :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 ... {
          ...
          }

          Kommentar


          • #6
            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!

            Kommentar


            • #7
              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!

              Kommentar


              • #8
                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.

                Kommentar


                • #9
                  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 ...
                  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 | [COLOR=silver][[/COLOR][COLOR=royalblue]–[/COLOR][COLOR=silver]][/COLOR]

                  Kommentar


                  • #10
                    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
                    Lasst euch nicht lumpen, hoch den Humpen!

                    Kommentar


                    • #11
                      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
                      Zuletzt geändert von combie; 21.01.2008, 03:08.
                      Wir werden alle sterben

                      Kommentar

                      Lädt...
                      X