[Jann Hendrik]

Basierend auf dem Tutorial [PHP] thumbnails erstellen - kleine Funktion habe ich angefangen eine kleine Klasse dafür zu schreiben.

Da ich noch nicht richtig fit in OOP bin gibt es sicherlich einige Punkte, die man verbessern kann und andere, die man verbessern muss!

Mich interessiert daher eure Meinung dazu, wo die Fallstricke in der Klasse sind, was ist daran brauchbar, was nicht....

Genutzt wird das dann so:

PHP-Code:

      <?php
        // binde Klassendatei ein
        require_once('thumbnail.class.php');

        // initialisiere die Klasse
        $thumbnail = new thumbnail();

        // definiere das Verzeichnis der Originalbilder
        $thumbnail->setFolderOfPicturesOriginalSize('pictures');

        // definiere das Verzeichnis der Vorschaubilder
        $thumbnail->setFolderOfPicturesThumbnailSize('thumbs');

        // definiere die max. horizontale Ausdehnung der Vorschaubilder
        $thumbnail->setThumbnailMaxX(300);

        // definiere die max. vertikale Ausdehnung der Vorschaubilder
        $thumbnail->setThumbnailMaxY(300);

        // gebe den von der Klasse generierten Code aus
        echo $thumbnail->makeThumbnails();
      ?>

Die Klasse selbst:

PHP-Code:

<?php
  class thumbnail
  {
    #  Name des Ordners, in dem die Originale sind:
    private $FolderOfPicturesOriginalSize;

    #  Name des Ordners, in den die Thumbs sollen:
    private $FolderOfPicturesThumbnailSize;

    #  Max. Ausdehnung der thumb-Datei in x-Richtung
    private $ThumbnailMaxX;

    #  Max. Ausdehnung der thumb-Datei in y-Richtung
    private $ThumbnailMaxY;

    # Datei von welcher ein thumbnail generiert werden soll
    private $ImageSource;

    private function setString($var, $value = '')
    {
      if (strlen(trim($value)) > 0)
      {
        $this->$var = (string) $value;
      }
    }

    private function setInt($var, $value = '')
    {
      if (strlen(trim($value)) > 0)
      {
        $this->$var = (int) $value;
      }
    }

    public function setFolderOfPicturesOriginalSize($string = '')
    {
      $this->setString('FolderOfPicturesOriginalSize', $string);
    }

    public function setFolderOfPicturesThumbnailSize($string = '')
    {
      $this->setString('FolderOfPicturesThumbnailSize', $string);
    }

    public function setThumbnailMaxX($int = 300)
    {
      $this->setInt('ThumbnailMaxX', $int);
    }

    public function setThumbnailMaxY($int = 300)
    {
      $this->setInt('ThumbnailMaxY', $int);
    }


    // Diese Funktion gibt es im Original unter [url]www.codeschnipsel.net[/url]
    // Ich habe sie ein wenig modifiziert
    private function makeOneThumbnail()
    {
      // Größe und Typ ermitteln
      list($src_width, $src_height, $src_typ) = getimagesize($this->FolderOfPicturesOriginalSize."/".$this->ImageSource);

      // neue Größe bestimmen
      if($src_width >= $src_height)
      {
        $new_image_width  = $this->ThumbnailMaxX;
        $new_image_height = $src_height * $this->ThumbnailMaxX / $src_width;
      }
      if($src_width < $src_height)
      {
        $new_image_height = $this->ThumbnailMaxY;
        $new_image_width  = $src_width * $this->ThumbnailMaxX / $src_height;
      }

      if($src_typ == 1)     // GIF
      {
        $image = imagecreatefromgif($this->FolderOfPicturesOriginalSize."/".$this->ImageSource);
        $new_image = imagecreate($new_image_width, $new_image_height);
        imagecopyresampled($new_image, $image, 0, 0, 0, 0, $new_image_width,$new_image_height, $src_width, $src_height);
        imagegif($new_image, $this->FolderOfPicturesThumbnailSize."/".$this->ImageSource, 100);
      }
      elseif($src_typ == 2) // JPG
      {
        $image = imagecreatefromjpeg($this->FolderOfPicturesOriginalSize."/".$this->ImageSource);
        $new_image = imagecreatetruecolor($new_image_width, $new_image_height);
        imagecopyresampled($new_image, $image, 0, 0, 0, 0, $new_image_width,$new_image_height, $src_width, $src_height);
        imagejpeg($new_image, $this->FolderOfPicturesThumbnailSize."/".$this->ImageSource, 100);
      }
      elseif($src_typ == 3) // PNG
      {
        $image = imagecreatefrompng($this->FolderOfPicturesOriginalSize."/".$this->ImageSource);
        $new_image = imagecreatetruecolor($new_image_width, $new_image_height);
        imagecopyresampled($new_image, $image, 0, 0, 0, 0, $new_image_width,$new_image_height, $src_width, $src_height);
        imagepng($new_image, $this->FolderOfPicturesThumbnailSize."/".$this->ImageSource, 100);
      }
      imagedestroy($image);
      imagedestroy($new_image);
      return true;
    }

    public function makeThumbnails()
    {
      $return = false;

      // Überprüfung ob Variablen richtig gesetzt und ob Verzeichnisse existieren
      if($this->FolderOfPicturesOriginalSize == '')
        return '<h3 style="color:red;">error</h3><span style="color:red;">Pfad für die Originalbilder nicht gesetzt!</span>';

      if($this->FolderOfPicturesThumbnailSize == '')
        return '<h3 style="color:red;">error</h3><span style="color:red;">Pfad für die Vorschaubilder nicht gesetzt!</span>';

      if($this->ThumbnailMaxX == '')
        return '<h3 style="color:red;">error</h3><span style="color:red;">Angabe für maximale Größe der Vorschaubilder in horizontaler Richtung nicht definiert!</span>';

      if($this->ThumbnailMaxY == '')
        return '<h3 style="color:red;">error</h3><span style="color:red;">Angabe für maximale Größe der Vorschaubilder in vertikaler Richtung nicht definiert!</span>';

      if(!file_exists($this->FolderOfPicturesOriginalSize))
        return '<h3 style="color:red;">error</h3><span style="color:red;">Das Verzeichnis der Originalbilder ('.$this->FolderOfPicturesOriginalSize.') existiert nicht</span>';

      if(!file_exists($this->FolderOfPicturesThumbnailSize))
        return '<h3 style="color:red;">error</h3><span style="color:red;">Das Verzeichnis der Vorschaubilder existiert nicht</span>';


      // Verzeichnis wird geöffnet
      $verz=opendir($this->FolderOfPicturesOriginalSize);

      // Ein Array wird erstellt in dem nachher die Dateien gespeichert werden
      $linkl = array ();

      while ($file = readdir ($verz))
      {
        clearstatcache ();
        // Mit jeder Datei wird etwas getan. Damit "." und ".." nicht ins Array geschrieben werden, werden sie ausgenommen.
        if(substr($file, 0, 1) != '.' && $file != 'Thumbs.db' && !is_dir($file))
        {
          array_push ($linkl, $file); // Der Dateiname wird in den Array $linkl geschrieben
        }
      }

      // Anzahl der Dateien im Ordner == Anzahl der Einträge im Array
      $anzahl = count($linkl);

      // Array wird nach Alphabet sortiert
      sort ($linkl);

      foreach($linkl as $key => $value)
      {
        // Nun noch die Überprüfung, ob das thumbnail schon existiert,
        // wenn ja, dann erstellt er kein neues, was den Seitenaufbau beschleunigt.
        if(!file_exists($this->FolderOfPicturesThumbnailSize."/".$value))
        {
          // erstelle ein thumb
          $this->ImageSource = $value;
          $this->makeOneThumbnail();
        }

        // gib es aus:
        $return .= '<a href="'.$this->FolderOfPicturesOriginalSize.'/'.$value.'"><img src="'.$this->FolderOfPicturesThumbnailSize.'/'.$value.'" alt="" /></a>'."\n";
      }

      // schließe das Verzeichnis:
      closedir($verz); // Verzeichnis wird geschlossen

      return $return;
    }
  } // Ende der Klasse
?>

// edit: ein paar Kleinigkeiten geändert, die Ben angesprochen hat

[Ben]

Einige Sachen, die nicht wirklich was mit der Klasse als Solches zu tun haben.

Wenn du eine Ganzzahl verwendest .. warum machst du sie dann explizit zu einem String? Ich finde, dass man immer mit dem Typ arbeiten sollte, den man auch erwartet.

PHP-Code:

private $Thumb[b]n[/b]ailMaxY; 


Lang lebe Copy & Paste!

Was mich total stört ist der Aufruf:

PHP-Code:

$sudoku = new thumbnail(); 


Abgesehen davon, dass Klassen groß geschrieben werden sollten, passt das doch vorne und hinten nicht. Ich finde es verwirrend. Was hat denn ein Sudoku mit einem Thumbnail zu tun?
Man sollte versuchen Funktionalitäten so zu abstrahieren, dass man allein durch das Lesen des Aufrufs im Grunde versteht, was dort geschieht. Da ist das $sudoku meiner Ansicht nach total kontraproduktiv.

Ansonsten ist da nichts Besonderes zu entdecken, was man beurteilen könnte. Eine Klasse allein hat ja nichts mit objektorientierte Programmierung zu tun.

[Jann Hendrik]

Nun, auf beide deine Punkte (thumbail und sudoku) kann ich nur antworten:

Zitat von Ben

Lang lebe Copy & Paste!

*mist* - klar, wird korrigiert!

Zitat von Ben

Wenn du eine Ganzzahl verwendest .. warum machst du sie dann explizit zu einem String? Ich finde, dass man immer mit dem Typ arbeiten sollte, den man auch erwartet.

Ja, da hast du Recht - auch dieser Punkt ist somit überarbeitet.

Zitat von Ben

Ansonsten ist da nichts Besonderes zu entdecken, was man beurteilen könnte. Eine Klasse allein hat ja nichts mit objektorientierte Programmierung zu tun.

Das mag sein - aber eine Klasse allein kann schon schlimm genug werden, wenn sie unsauber ist.

[Ben]

Ich versteh nicht so ganz, wozu man eine Methode setString() und setInt() braucht, aber nunja. Scheinst du ja irgendwo aufgegabelt zu haben und dir dabei etwas denken.

[Jann Hendrik]

Ich dachte mir, dass das sinnvoll ist um ein korrektes type-casting machen zu können.

Aufgegabelt habe ich das in der Klasse, die ich hier ([PHP] vCard PHP Parser) beschrieben habe.

[Basti]

Wie wäre es mit folgender API?

PHP-Code:

<?php

$ThumbnailGenerator = new ThumbnailGenerator;

$ThumbnailGenerator->createThumbnails('images', 'thumbs', 300, 300);

Fragen an diese Methode wären dann noch, ob und wie die Dateien benannt werden sollen, ob nur spezielle Bild-Typen hergenommen werden sollen, ob das angegebene Verzeichnis rekursiv durchlaufen werden soll.

Zu deiner API stellt sich mir einfach die Frage, warum so viele Methodenaufrufe? Was willst du mit einer Konfigurierten "Thumbnail"-Klasse? Doch nur einmal Thumbnails in einem bestimmten Format erzeugen, ggf. auch ein zweites mal in einer anderen Größe, aber da wäre ja nur das Quell-Verzeichnis redundant.

Ich bin kein Freund von langen Parameterlisten, aber du hast hier doch nur eine Funktion, die 4 Parameter benötigt. Das "Ausklammern" dieser 4 Parameter in eigene Methodenaufrufe macht doch nur Sinn, wenn die Klasse mehrmals mit den selben Parametern benutzt wird. Anders betrachtet: Du speicherst einen Status deines Objektes. Warum muss ein Thumbnail-Generator die beiden Verzeichnisse speichern? Warum das zuletzt benutzte Bildgrößen-Format? Das macht nur Sinn, wenn der Generator mehrfach mit den gleichen Werten aufgerufen werden soll, macht ihn aber schwerfälliger und seine Bedienung fehleranfälliger.

Mit Ausklammern meine ich folgendes:

PHP-Code:

// aus

$aThumbnailClasses = array(
    'small' => array(100, 100),
    'large' => array(400, 400)
);

$TG = new ThumbnailGenerator;

foreach ($aThumbnailClasses as $sClass => $aDimensions)

    $TG->generate('images', 'thumbs/' . $sClass, $aDimensions[0], $aDimensions[1]);

// wird


$aThumbnailClasses = array(
    'small' => array(100, 100),
    'large' => array(400, 400)
);

$TG = new ThumbnailGenerator;

$TG->setSourceDir('images');

foreach ($aThumbnailClasses as $sClass => $aDimensions)

    $TG->generate('thumbs/' . $sClass, $aDimensions[0], $aDimensions[1]); 


Zitat von Jann Hendrik

Ich dachte mir, dass das sinnvoll ist um ein korrektes type-casting machen zu können.

Ich würde hier eher prüfen, ob die übergebenen Werte korrekt sind anstatt sie irgendwie umzuwandeln. Das ist zwar übliche PHP-Strategie, aber meiner Meinung eher kontraproduktiv. Wenn ein nicht eindeutiger Wert (was auch immer das jetzt sei) übergeben wird, möchte ich einen Fehler sehen, um den Wert eindeutig machen zu können (und prüfen zu kommen, ob ich nur ungenau programmiert hab oder ob die Mehrdeutigkeit auf einem konzeptionellen Fehler beruht), anstatt dass die Klasse den Wert für mich erstmal unsichtbar umwandelt oder gar wortlos verwirft nach Regeln, die ich im Kopf behalten muss, um nicht dauernd nachschlagen zu müssen.

assert() wäre ein Ansatz, Exceptions ein weiterer.

Und, natürlich lässt sich sowas schön in abstrakten Basis-Klassen kapseln, nur dann eben besser nach dem Motto: "Setze, falls gültig, andernfalls schmeiße einen Fehler" anstatt "Setze, falls gültig, sonst nicht" oder "Machs irgendwie gültig und setze".

Basti

[Jann Hendrik]

Zitat von Basti

Wie wäre es mit folgender API?

PHP-Code:

<?php

$ThumbnailGenerator = new ThumbnailGenerator;

$ThumbnailGenerator->createThumbnails('images', 'thumbs', 300, 300);

So hatte ich das bisher immer (in Form einer einfachen Funktion).
Ich dachte, dass es vorteilhaft sein könnte, wenn das ausgeklammert ist, kann das aber nicht begründen, das war mehr intuitiv...

Zitat von Basti

Fragen an diese Methode wären dann noch, ob und wie die Dateien benannt werden sollen, ob nur spezielle Bild-Typen hergenommen werden sollen, ob das angegebene Verzeichnis rekursiv durchlaufen werden soll.

Ok, da erscheint es mir durchaus sinnvoller das mittels Parameter zu machen, die einfach nen default-Wert mitbekommen, wenn man damit glücklich ist und für andere Fälle das zur Verfügung hat.
Gerade die Einschränkung auf bestimmte Bildtypen bzw. die Rekursion gefällt mir als Gedanke!
Was die Datei-Umbenennung angeht - da weiß ich nicht so recht. Ich finde das durchaus praktisch, wenn die thumbnails genau heißen, wie die Originale.
Was wäre hier sinnvoll zu beachten, wenn man abweichende Namen haben will?
Vorstellen könnte ich mir die x-y-Dimensionen anzugeben, weil man dann theoretisch mehrere unterschiedlich große Versionen haben kann...ok, aber... gibt es noch andere Punkte?

Zitat von Basti

Zu deiner API stellt sich mir einfach die Frage, warum so viele Methodenaufrufe? Was willst du mit einer Konfigurierten "Thumbnail"-Klasse? Doch nur einmal Thumbnails in einem bestimmten Format erzeugen, ggf. auch ein zweites mal in einer anderen Größe, aber da wäre ja nur das Quell-Verzeichnis redundant.

Was spricht denn dagegen das mit mehreren Methoden zu lösen, oder: worin liegt der Vorteil weniger zu nutzen?
Ist es lediglich der Punkt der Wartbarkeit und der Punkt der Fehleranfälligkeit?

Zitat von Basti

Ich würde hier eher prüfen, ob die übergebenen Werte korrekt sind anstatt sie irgendwie umzuwandeln.

Der Einwand ist absolut korrekt!
Werde ich noch ausbauen.

Zitat von Basti

assert() wäre ein Ansatz, Exceptions ein weiterer.

Und, natürlich lässt sich sowas schön in abstrakten Basis-Klassen kapseln, nur [...]

ja, da wäre ich dann eingetaucht in den Bereich der Thematik von der ich nichts verstehe...


Bleibt also die Frage (wenn ich das so alles richtig verstanden habe), warum es vorteilhaft ist, auf die set...-Methoden zu verzichten und den Teil in die Haupt-Methode zu übernehmen....
Der geänderte Aufruf wäre damit dann ja quasi verbunden.

[Ben]

Die setter brauchst du eigentlich nur, wenn du nach einem "set" auch andere Aktionen durchführen kannst. In deinem Fall ist der Ablauf immer der Gleiche. Du setzt die Sachen explizit und rufst dann die "generate"-Methode auf, wobei diese darauf angewiesen ist, dass im Voraus die set-Methoden erfolgreich ausgeführt wurden.
Irgendwie also doppelt gemoppelt.

Setzt du die entsprechenden Daten einfach als Übergabeparameter der Methode, so hast du alles in einem Schritt parat.
Sinnvoll könnte es sein, wenn du z.B. den Dateinamen setzt und danach die Dimension, welche ich übrigens als Array übergeben würde. Der Name kann dann mehrfach verwendet werden, die Dimension kannst du über die setter ändern.

Kommt drauf an, was du eigentlich erreichen willst.

[Jann Hendrik]

okay, das ist nachvollziehbar.

Ich werde die noch sehr übersichtliche Klasse dann mal ein wenig umstricken.

Zitat von Ben

Kommt drauf an, was du eigentlich erreichen willst.

Gute Frage.
1) dass das funktioniert
2) dass ich den Unterschied zwischen funktioniert und funktioniert gut lerne

Vor allem aber der 2te Punkt, denn eine einfache Funktion, die die Arbeit getan hat war schon verfügbar.
Nur will ich das ganze in etwas größeres einbauen können, ohne jedesmal nen Hampelmann machen zu müssen.

[Basti]

Zitat von Jann Hendrik

Was spricht denn dagegen das mit mehreren Methoden zu lösen, oder: worin liegt der Vorteil weniger zu nutzen?
Ist es lediglich der Punkt der Wartbarkeit und der Punkt der Fehleranfälligkeit?

Für mich stellt sich eher die Frage nach dem Sinn. Du rufst ja auch nicht bei deiner Bank an, gibst deine Kontonummer durch und legst wieder auf. 2 Minuten später rufst du wieder an und gibst einen Betrag durch, beim nächsten Anruf ein Empfänger-Konto und beim vierten Anruf sagst du dann nur noch "überweisen!" in den Hörer.

Hier würde es vielmehr Sinn machen, wenn du einmal anrufst und deine eigene Kontonummer angibst. Bei jedem weiteren Anruf sagst du, was du mit diesem Konto machen möchtest: Einmal "Überweise EUR 10.000 an Kto. sowieso", dann "richte Unterkonto 008 ein", dann "Schicke mir neue TANs für 005" usw. Hier würdest du also deine Kontonummer einmalig angeben, aber den Rest der Angaben machst du je Transaktion/Auftrag erneut. (jaaa, ich weiß, das Beispiel ist ein wenig unsinnig)

Du definierst also ein Objekt, das sich ausschließlich auf dein Konto bezieht.

Anderes Beispiel: Du hast einen speziellen Service-Vertrag mit deiner Autowerkstatt. Dieser bezieht sich ausschließlich auf ein Auto und macht damit den Unterschied zu jedem anderen, beliebigen Besuch einer Autowerkstatt:

PHP-Code:

<?php

class MyGarage extends Garage
{
    private $MyCar;

    public function __construct(Car $MyCar)
    {
        $this->MyCar = $MyCar;
        parent::__constuct();
    }

    public function maintainBrakes()
    {
        parent::maintainBrakes($this->MyCar);
    }

    public function changeTires()
    {
        parent::changeTires($this->MyCar);
    }
}


class Garage
{ 
    public function __construct()
    {
    }

    public function maintainBrakes(Car $Car)
    {
        $Car->getBrakes->iterate()->setState('new');
    }
 
     public function changeTires