Was mach ich falsch? Soviel Text in der Logik.:)

[omaliesschen]

Hi,

ich seh immer wieder die sauber strukturierten Projekte und bekomme regelmäßig einen Minderwertigkeitskomplex. Alle guten Projekte sind weitesgehend frei von "text".

Was mach ich falsch? Gibts einen Leitfaden oder sonstige Literatur zu dem Thema?

Beispiel:

Mein Paginator durchzogen von "grünen Textadern":

class paginator{


    private $url_parameter;
    private $total_num_sites;
    private $identifier;
    private $max_numbers;
    private $results_per_page;
    private $center_pagelist;

    public  $current_site;
    public  $total_pages;
    public  $paginator;

    private $disabled_button = 'class="disabled"';


    function create($g,$s,$t,$i,$n,$x){

    	$this->url_parameter    = $g;
    	$this->current_site     = $s;
    	$this->total_num_sites  = $t;
    	$this->identifier       = urlencode($i);
    	$this->max_numbers      = $n;
    	$this->results_per_page = $x;

        // number of pages
    	$this->total_pages = ceil($this->total_num_sites / $this->results_per_page);

        // handle invalid values
    	if( $this->total_pages <= 0 ){ 
    		$this->total_pages = 1; 
    	}
    	elseif ($this->current_site > $this->total_pages) { 
    		$this->current_site = $this->total_pages; 
    	}
    	elseif ($this->current_site <= 0) { 
    		$this->current_site = 1; 
    	}

        // first and last page buttons
    	$prc_f   = $this->current_site > 1 ? 
    	'href="index.php?'.$this->url_parameter.'='.$this->identifier.'.1"' : 
    	$this->disabled_button;

    	$prc_l   = $this->current_site < $this->total_pages ? 
    	'href="index.php?'.$this->url_parameter.'='.$this->identifier.'.'.$this->total_pages.'"' : 
    	$this->disabled_button;

    	$jump_first = '<a '.$prc_f.' ><< First</a>';
    	$jump_last  = '<a '.$prc_l.' >Last >></a>';

        // start value offset
    	$start = ( $this->current_site - $this->max_numbers ) < 0 ? 
    	1 : $this->current_site-$this->max_numbers;

    	$stop  = false;

        // create center page numeration
    	while(!$stop){

    		$urlB = '?'.$this->url_parameter.'='.$this->identifier.'.'.$start;

    		// highlight selected page */
    		if( $start == $this->current_site ){ 
    			$this->center_pagelist[] = "<a class='sel'>".$start."</a>"; 
    		}

    		// else normal font */
    		else{ $this->center_pagelist[] = "<a href='index.php".$urlB."'>".$start."</a>"; }

    		++$start;

    		if ( $start > $this->total_pages || $start == ( $this->current_site+$this->max_numbers ) ) { 
    			$stop = true; 
    		}

    	}

    	// previous and next page button
    	$prc_p = $this->current_site == 1 ? 
    	$this->disabled_button : 
    	'href="index.php?'.$this->url_parameter.'='.$this->identifier.'.'.($this->current_site - 1).'" ';

    	$prc_n = $this->current_site == $this->total_pages ? 
    	$this->disabled_button : 
    	'href="index.php?'.$this->url_parameter.'='.$this->identifier.'.'.($this->current_site + 1).'" ';

    	$bp = '<a '.$prc_p.' ><i class="icon-chevron-sign-left"></i></a>';
    	$bn = '<a '.$prc_n.' ><i class="icon-chevron-sign-right"></i></a>';

        // create pagination panel
    	$this->paginator = '<ul class="pageswitcher"><li class="s_page"><form action="index.php" method="get"><label class="page_switch">Jump to page:  <input type="hidden" name="'.$this->url_parameter.'" value="'.$this->identifier.'.1"><input type="text" size="4" name="s"></label><input class="sw" type="submit" value="Go" ></form></li><li>'.$jump_first.'</li><li>'.$bp.'</li><li>'.implode($this->center_pagelist).'</li><li>'.$bn.'</li><li>'.$jump_last.'</li></ul>';

    }


}

Und hier ein paar sauber geschriebene:
http://www.php.de/scriptboerse/74048-paginator-algorithmus.html


Irgendwo muss man das html natürlich unterkriegen. Was wäre der fachlich korrekte Weg?

 

[1668mib]

- Templates...
- in andere Methoden auslagern... (gerade die dinge, die du kommentierst, sind prädestiniert, in eigene methoden ausgelagert zu werden...)

dass es am Ende so aussehen könnte (nur mit dem Auslagern, Templates sind davon unabhängig):

function create($g,$s,$t,$i,$n,$x){
  $this->url_parameter    = $g;
  $this->current_site     = $s;
  $this->total_num_sites  = $t;
  $this->identifier       = urlencode($i);
  $this->max_numbers      = $n;
  $this->results_per_page = $x;
 
  getNumberOfPages();
  handleInvalidValues();
  createFirstAndLastPageButtons();
  createCenterPageNumeration();
  createPreviousAndNextPageButtons();
  createPaginationPanel();
} 

// functions
...

Buch-Tipp allgemein für sowas: Clean Code

 

[CBCrosser]

Funktionen, Auslagerungen (für oft genutzte Anweisungen) und globale Variablen könnten helfen.

 

[ice-breaker]

wenn du die Klasse in einen "PaginatorRenderer" umbennenst, dann stimmt es schon. Denn du hast einen Renderer gebaut, denn er bekommt die fertigen Daten und macht daraus HTML. Ein normaler Paginator hätte nur etwas wie ein Suchquery und einen Offset bekommen und hätte daraus die Daten berechnet, die du deiner create()-Methode übergibst.

Dieser Renderer sollte aber wirklich nur vom View aufgerufen werden, schau dir mal das MVC-Modell mit der Trennung an.

Und an den Konventionen bzw. Stil musst du auch noch arbeiten: Klassennmane Camelcasing, Sichtbarkeit für Methoden, ansprechende Variablennamen (z.B. die Parameter der create-Methode). Und da kann man sich streiten, aber auch bei Attributen hat sich CamelCasing statt Unterstrichen durchgesetzt Und im PHP-Lager wird gerne immernoch der Name des Attributs bei private/protected mit einem Unterstrich begonnen.


@Gabbadome: globale Variablen? O RLY? Jetzt komm ihm nicht mit so einem Blödsinn, wenn er sich versucht sinnvolles OOP anzugewöhnen.

 

[Daaron]

Was ich ändern würde:
- Klassennamen groß schreiben und natürlich jede Klasse in einer eigenen Datei lagern. Dazu gehört noch ein hübscher Autoloader, das spart viele includes()
- function create(...) ersetzen durch einen richtigen Konstruktor. Dann kann der User einfach sagen: $pagi = new Pagination(...)
- dazu gehören sich evtl. noch magische Getter/Setter-Funktionen, falls mal jemand die Parameter nicht direkt zur Hand hat
- gib deinen Parametern anständige Namen. $g,$s,$t,$i,$n,$x versteht kein Mensch. Außerdem kann man bei PHP auch für Parameter einen Type Hint mit angeben. Angenommen du hast eine Klasse Database und ein Objekt der Klasse verwaltet deine aktuelle DB-Verbindung: du könntest jetzt eine Funktion, die etwas mit der Datenbank machen soll, in etwa so schreiben: function dummy(Database $dbo){}
- gewöhn dir an, Funktionen zu kommentieren. Gute IDEs wie Eclipse helfen dabei.
- Rückgabe der Pagination mit ner eigenen Funktion, nicht im Konstruktor, z.B. public function parse().
- Pack eine kleine Template-Engine in den Hintergrund. In der steht dein purer HTML-Code zusammen mit der Schleifenlogik und den einzutragenden Werten. Ich würd auch nicht mit jump_first/_last anfangen, sondern mir direkt in der Programmlogik ein Array aufbauen, das auch die First/Last - Felder enthält.

 

[1668mib]

Und um die Verwendung solcher Parameterlisten einfacher zu machen, evtl das Builder-Pattern verwenden ...

 

[ice-breaker]

Imo hätte das Builder Pattern hier aber gar keinen Vorteil: Weder werden verschiedene Paginatorimplementationen auf Basis der Builderdaten erzeugt, noch hat man eventuell einige optionale Attribute (wo ein Builder wirklich echt genial) ist.
Das Builder Pattern wäre einfach nur java mäßig absolut overengineered, vor allem hat der Fall hier iirc auch keinen der GOF Vorteile des Builder Patterns.

 

[1668mib]

@ice-breaker: ich stimme dir zu, dass das Builder-Pattern vor allem für optionale Parameter von Vorteil ist, aber bei der Länge der Parameterliste hier, wäre es eine Überlegung wert... oder eine besseres Zusammenfassen von Eingangsparametern...

Man könnte das Builder-Pattern hier verwenden, um die Parameter zu "benennen"...

6 Paramter, welche Reihenfolge haben sie? Was heißt $x was heißt $g?

Aber klar, man kann die Parameterliste auch evtl anders kleiner und übersichtlicher bekommen.

@Daaron:

gewöhn dir an, Funktionen zu kommentieren.

Darüber kann man rieflich diskutieren...
Öffentliche APIs ja. Interne APis die eh noch ständig geändert werden... naja...
Kommentare sind dort nötig, wo der Code nicht ausdrucksstark genug ist. Aber bevor man kommentiert, sollte man versuchen, den Code besser zu machen. ;-)

 

[Daaron]

Die Parameter-Liste lässt sich doch eh kürzen. Die aktuelle Seite könnte man aus einer globalen Variablen auslesen oder von einem Eltern-Element übernehmen, das den Kram kennt. Dasselbe gilt für die URL-Paramter und den Identifier (klingt nach nem Reintext-Alias für die aktuelle Page-ID). Class Pagination extends Page...
Die Anzahl pro Seite und die maximale Anzahl könnte man z.B. auf Datenbank-Basis lösen und bei Bedarf laden. Da hat man eben irgendwo eine Tabelle, die alle Paginations vom Backend aus verwaltet.

 

[1668mib]

Auf Datenbank-Basis? Ich finde ja, dass diese Klasse nichts von irgendeiner Datenbank wissen muss ...
Und welches Eltern-Element?

Ich finde nicht, dass man in die Klasse jetzt unbedingt so viel mehr Abhängigkeiten rein machen muss...

 

[omaliesschen]

Merci. Das Beispiel liegt schon halb unter der Erde...

Das letzte hier: http://www.php.de/scriptboerse/74048-paginator-algorithmus.html scheint gelungen zu sein.

Werd mich daran orientieren. Erfüllt wohl auch die genannten Kriterien.

 

[Daaron]

Auf Datenbank-Basis? Ich finde ja, dass diese Klasse nichts von irgendeiner Datenbank wissen muss ...
Und welches Eltern-Element?

Class Pagination extends Page <- Sowas in der Art.
Mit Vererbung und Traits kann man da einiges zusammen basteln.

[/quote]Ich finde nicht, dass man in die Klasse jetzt unbedingt so viel mehr Abhängigkeiten rein machen muss...[/QUOTE]
Ne nachdem, wie komplex das Gesamtpaket ist. Wenn die Seite eh viele Klassen lädt, dann kann es diese auch für das einzelne Pagination-"Modul" mit verwenden. Dafür kann man dann auf eine einheitliche Notation zugreifen.

 

[omaliesschen]

Die Seite ist Rechenintensiv. Je flotter und kompakter das ganze umso besser.

 

[ice-breaker]

als ob das einen Unterschied machen würde Du bewegst dich da im Nanosekundenbereich, jede Anfrage an eine Datenbank ist da kostenintensiver....

 

[Daaron]

Und selbst wenn es mal nicht an der DB hängt (tut es fast immer), kann es immer noch am verwendeten Multi-Processing Module liegen bzw. an dessen Konfiguration. Auch die Art, wie der PHP-Code ausgeführt wird, spielt eine extrem große Rolle. Du kannst ein Jahr mit Optimierung deines Codes zubringen, wenn der Server auf suPHP läuft, dann wars das einfach....
Wenn dir dein PHP-Code am Ende immer noch zu langsam ist: Verwende die HipHop-Engine.

Da es aber trotzdem immer an der DB hängt: Optimiere dort lieber.

 

[omaliesschen]

HipHop wird auf jedenfall mal getestet wenn ich irgendwann mal fertig werden sollte.

Der php Teil ist an einigen Stellen noch stark verbesserungswürdig wobei ich alles in allem durchaus zufrieden bin dem phpbbkiller.