PDA

Archiv verlassen und diese Seite im Standarddesign anzeigen : Guten Code schreiben



Andreas
09.08.2007, 17:29
Dies kann und soll kein Crashkurs in Sachen "guter" (PHP) Programmierung werden, aber auf einige elementare Dinge die einem das Leben leichter machen möchte ich doch eingehen.
Das A und O ist gut lesbarer Code!
Nichts ist schlimmer als "Spaghetti-Code" bei dem alles in der gleichen Spalte steht.
Am besten ist es, man hält sich an die vBulletin Code Standards, dies ist jedoch nicht zwingend erforderlich.

Hier einige wichtige Punkte der Code standards

Eirückung per Tab
AND und OR sollen verwendet werden, nicht && und ||
Funktions-/Methodennamen kleingeschrieben, ggf. mit unterStrich
Beispiel: print_my_foorow()
Keine Leerzeichen um Argumente/Klammern
Bei if-Abfragen die Blöcke immer in einer neuen Zeile beginnen lassen:
PHP:
----------
Der Inhalt dieses Abschnitts ist nur für Lizenznehmer sichtbar, Sie werden derzeit jedoch nicht als Lizenzinhaber erkannt.<br />
<br />
Bitte öffnen Sie den <a href="http://members.vbulletin-germany.com/membersupport_priority.php">Kundenbereich</a>, tragen Sie Ihre E-Mail-Adresse ein, mit der Sie sich hier registriert haben und aktivieren Sie die Lizenzüberprüfung für http://www.vbulletin-germany.org.
----------
Tabellen/Feldnamen ohne Unterstrich


Mindestens ebensowichtig wie das reine Struktur des Codes ist es sicher und möglichst effizient zu programmieren.
2 Punkte sind hierbei insbesondere bei PHP-Scripten sehr wichtig: Cross Site Scription (XSS) und SQL Injections.
Als Cross Site Scripting bezeichnet man die Möglichkeit, dass ein Angreifer eigenen HTML-Code einschleusen kann, bei einer SQL Injection ist es möglich direkt Datenbankabfragen zu manipulieren.

Hier ein (bewusst extrem schlechtes!) Beispiel-Script um beide Probleme zu demonstrieren.
PHP:
----------
Der Inhalt dieses Abschnitts ist nur für Lizenznehmer sichtbar, Sie werden derzeit jedoch nicht als Lizenzinhaber erkannt.<br />
<br />
Bitte öffnen Sie den <a href="http://members.vbulletin-germany.com/membersupport_priority.php">Kundenbereich</a>, tragen Sie Ihre E-Mail-Adresse ein, mit der Sie sich hier registriert haben und aktivieren Sie die Lizenzüberprüfung für http://www.vbulletin-germany.org.
----------
Eigentlich sieht das Script so aus als würde es das tun was es soll ...
Aber was passiert wenn man als Vorname z.B. <script type="text/javascript">alert('XSS');</script> eingibt?
Dann gibt das Script den Text wieder aus - das JavaScript wird ausgeführt und schon haben wir eine XSS-Lücke.

Noch besser: Eingabe ' OR vorname != '
Dann ergibt sich als Datenbankabfrage
CODE:
----------
Der Inhalt dieses Abschnitts ist nur für Lizenznehmer sichtbar, Sie werden derzeit jedoch nicht als Lizenzinhaber erkannt.<br />
<br />
Bitte öffnen Sie den <a href="http://members.vbulletin-germany.com/membersupport_priority.php">Kundenbereich</a>, tragen Sie Ihre E-Mail-Adresse ein, mit der Sie sich hier registriert haben und aktivieren Sie die Lizenzüberprüfung für http://www.vbulletin-germany.org.
----------
was dann mal gerade alle Datensätze löscht, und nicht nur den gewünschten.

Was kann man nun tun um beides zu verhindern?
Niemals Eingabedaten direkt zur Ausgabe oder für Datenbankabfragen verwenden!

Es sollten immer die Input-Cleaner ($vbulletin->input->clean_array_gpc()) verwendet werden bevor Eingabedaten weiter verrbeitet werden.
Diese Methode stellt sicher dass die Eingabedaten tatsächlich den erwarteten Typ haben.

Beispiel
PHP:
----------
Der Inhalt dieses Abschnitts ist nur für Lizenznehmer sichtbar, Sie werden derzeit jedoch nicht als Lizenzinhaber erkannt.<br />
<br />
Bitte öffnen Sie den <a href="http://members.vbulletin-germany.com/membersupport_priority.php">Kundenbereich</a>, tragen Sie Ihre E-Mail-Adresse ein, mit der Sie sich hier registriert haben und aktivieren Sie die Lizenzüberprüfung für http://www.vbulletin-germany.org.
----------

Durch die Verwendung von TYPE_NOHTML ist hierbei bereits sichergestellt dass diese Variable kein HTML enthält.

Wenn die Möglichkeit besteht dass ein auszugebender Text HTML enthält so muss dieser zuvor mit htmlspecialchars_uni() behandetl werden damit kein XSS möglich ist.

Beispiel
PHP:
----------
Der Inhalt dieses Abschnitts ist nur für Lizenznehmer sichtbar, Sie werden derzeit jedoch nicht als Lizenzinhaber erkannt.<br />
<br />
Bitte öffnen Sie den <a href="http://members.vbulletin-germany.com/membersupport_priority.php">Kundenbereich</a>, tragen Sie Ihre E-Mail-Adresse ein, mit der Sie sich hier registriert haben und aktivieren Sie die Lizenzüberprüfung für http://www.vbulletin-germany.org.
----------

In Datenbankabfragen sollten Strings immer mit $db->escape_string() behandelt werden damit keine SQL Injection möglich ist.

Beispiel
PHP:
----------
Der Inhalt dieses Abschnitts ist nur für Lizenznehmer sichtbar, Sie werden derzeit jedoch nicht als Lizenzinhaber erkannt.<br />
<br />
Bitte öffnen Sie den <a href="http://members.vbulletin-germany.com/membersupport_priority.php">Kundenbereich</a>, tragen Sie Ihre E-Mail-Adresse ein, mit der Sie sich hier registriert haben und aktivieren Sie die Lizenzüberprüfung für http://www.vbulletin-germany.org.
----------

In Templates dürfen niemals $_POST, $_GET, $_REQUEST, $_COOKIE-Variablen direkt verwendet werden.

Noch ein paar Worte zur Effizienz:
Vielfach sieht man Hacks die weit mehr Datenbankabfragen verwenden als nötig.
Meist stehen benötigte Informationen nämlich bereits zur Verfügung, ohne dass man diese neu abfragen müsste.
Für häufig verwendete Datenobjekte gibt es daher fetch_xxx-Funktioen.
Braucht man also z.B. die Info für ein Forum sollte fetch_foruminfo() verwendet werden, für Threads fetch_threadinfo() usw.

Diese Funktionen verwenden Caches, somit ist (im Regelfall) sichergestellt dass keine unnötigen Abfragen ausgeführt werden.

Auch sollte man es vermeiden Abfragen in Schleifen auszuführen.
Sollte es nötig erscheinen dies zu tun, so liegt mit 99,99% Wahrscheinlichkeit ein Fehler in der Codestruktur vor und man ist gut beraten einfach mal nachzufragen wie man das vielleicht besser machen könnte.

Ein Klassiker für Abfragen in Schleifen ist z.B. der Einstiegspunkt postbit_display_[start|complete].
Hier eingepflegte Plugins werden für jeden Beitrag 1x ausgeführt - ein ganz schlechter Platz für Abfragen also.

Andreas
01.09.2007, 09:35
Gemäß vBulletin Code Standards ist die von mir gepostete Variante vorzuziehen, wobei deine Variante auch ok ist.
Letztendlich wie gesagt Geschmackssache.

Andree
01.09.2007, 09:47
Zend (die php Company ;)) schlägt folgende Syntax vor:
PHP:
----------
Der Inhalt dieses Abschnitts ist nur für Lizenznehmer sichtbar, Sie werden derzeit jedoch nicht als Lizenzinhaber erkannt.<br />
<br />
Bitte öffnen Sie den <a href="http://members.vbulletin-germany.com/membersupport_priority.php">Kundenbereich</a>, tragen Sie Ihre E-Mail-Adresse ein, mit der Sie sich hier registriert haben und aktivieren Sie die Lizenzüberprüfung für http://www.vbulletin-germany.org.
----------Ich bevorzuge selbst aber auch diese Möglichkeit
PHP:
----------
Der Inhalt dieses Abschnitts ist nur für Lizenznehmer sichtbar, Sie werden derzeit jedoch nicht als Lizenzinhaber erkannt.<br />
<br />
Bitte öffnen Sie den <a href="http://members.vbulletin-germany.com/membersupport_priority.php">Kundenbereich</a>, tragen Sie Ihre E-Mail-Adresse ein, mit der Sie sich hier registriert haben und aktivieren Sie die Lizenzüberprüfung für http://www.vbulletin-germany.org.
----------Das ist übersichtlicher wg. den Klammern.

Andree

GameR
01.09.2007, 10:00
Ich muss dazu sagen, das der "codestandard" von Entwicklerteams fest gelegt wird.
So wollen, soweit ich weiß das pear netzwerk für AND lieber && sehen und für OR ||
Was festh steht, der Code sollte sauber sein.
Ordentlich eingerückt, und vorallem dann gleich bleibend.

Lack
26.01.2008, 16:36
Dieser erste Standard für if ist grausam zu lesen, zum Glück ist das nicht vB-Standard.

Keine Einrückungen zu machen ist natürlich das letzte, nur statt Leerzeichen (wie mans oft findet) solltens Tabs sein - aber sowas kann ein guter Editor wenigstens in einem Aufwaschen bereinigen.

SQL-Anweisungen groß schreiben gehört auch zum guten Stil.

Und statt $db->escape_string($var) würde manchmal auch $db->sql_prepare($var) passen - erledigt, unter der Voraussetzung dass es angebracht ist, ein paar zusätzliche Aufgaben,
Bool zu Integer (0 oder 1), numerischer String zu Zahl.

Mir persönlich ist &&, || zwar sympatischer, aber bitte, besser trotzdem sich an Standard des restlichen Codes zu halten.

Und bei den Templates ein bissl auf valides XHTML zu achten wäre auch nicht schlecht.
Nur als Beispiel, weils am häufigsten zu sehen ist,
Images:
Muss ein alt-Attribut enthalten und dann wird ja nicht mit einem Image-Tag geschlossen, daher am Ende immer ein Leerzeichen, gfolgt von Slash und erst dann spitze Klammer zumachen - gilt für alle Tags die kein Endtag haben.

Andreas
26.01.2008, 18:35
Mir persönlich ist &&, || zwar sympatischer, aber bitte, besser trotzdem sich an Standard des restlichen Codes zu halten.
Es gibt einen IMHO guten Grund der gegen &&, || spricht: Fehleranfälligkeit.

Wie schnell schreibt man statt && nur ein & - und schon hat man etwas vollkommen anderes.
Bei AND, OR kannn das nicht passieren.

Lack
26.01.2008, 20:05
Ok, du hast sicher eine Menge Erfahrung, und über sowas diskutieren macht eh kein Sinn. Aber wenns danach ginge müsste man sich für alles mögliche ein Wort einfallen lassen, zB == (wohl einer der häufigsten Anfängerfehler).

Und der Verdacht liegt nahe dass vB zumindest teilweise mit VS.php gemacht wurde - würde das php-Image im Editor erklären, das denke ich von dort übernommen wurde. Mit VS.php kann man diese logischen Verknüpfungen fast unmöglich übersehen (fett und rotbraun).

Aber egal...

Andreas
26.01.2008, 20:09
Ich wollte auch gar nicht diskutieren :)
Nur einen Grund nennen warum (möglicherweise) AND/OR anstatt &&/|| verwendet wird.


zB == (wohl einer der häufigsten Anfängerfehler).
Den Fehler machen vmtl. auch Profis nach Jahren noch hin und wieder.
(Die wissen entgegen dem Anfänger dann aber dass es ein Fehler ist ;))