• Jetzt anmelden. Es dauert nur 2 Minuten und ist kostenlos!

Feedback gewünscht Bitte um Feedback!

J

Juare

Neues Mitglied
Servus,

der funktioniert! Nach vielen Hürden endlich zum laufen bekommen nun eine andere Frage:

Wie findet ihr den Code?
Was kann ich verbessern?

PHP: 
<?php

class registrierung
{
    public $name;
    public $email;
    public $password;

    public function setzenName($pruef_name) {
        $this->name = $pruef_name;
    }

    public function setzenEmail($pruef_email) {
        $this->email = $pruef_email;
    }

    public function setzenPasswort($pruef_password) {
        $this->password = $pruef_password;
    }

    public function pruefung_registrierungsvorgang_korrektheit() //
    {
         if(empty($this->name) OR empty($this->email) OR empty($this->password))
         {
             print_r ("Ups... Da ist wohl irgendwas schief gelaufen :( \n");
            
            if(empty($this->name))
            {
                print_r ("Bitte gebe einen Namen ein! \n");
            }

            if(empty($this->email))
            {
                print_r ("Bitte gebe eine Email ein! \n");
            }

            if(empty($this->password))
            {
                print_r ("Bitte gebe einen Passwort ein! \n");
            }
         }
         else
         {
            print_r ("Yuhuu! Dein Start ins neue Abenteuer kann beginnen deine Registrierung war Erfolgreich!</br> \n");
            print_r ("Wir haben dir außerdem eine Email zukommen lassen in dem nochmal all deine Daten stehen.</br> \n");
            print_r ("Viel Spaß beim spielen :) \n");
         }
        
    }
        

}

$registrierung_pruefung = new registrierung(); //Bauplan aufrufen

$registrierung_pruefung-> setzenName("1");
$registrierung_pruefung-> setzenEmail("1");
$registrierung_pruefung-> setzenPasswort("1");
$registrierung_pruefung-> pruefung_registrierungsvorgang_korrektheit(); //Ueber Bauplan "Parameter" übergeben

print_r ("</br>");
print_r ("</br>");
print_r ("Dein Name: $registrierung_pruefung->name");
print_r ("</br>");
print_r ("Deine Email: $registrierung_pruefung->email");
print_r ("</br>");
print_r ("Dein Passwort: $registrierung_pruefung->password");

error_reporting(-1);
?>

Danke im Vorraus
 
Werbung:
Juare schrieb:
Was kann ich verbessern?
Zum Vergrößern anklicken....

1. Klassen werden groß geschrieben.
2. Immer englisch statt deutsch.
3. Bitte gib...
4. Die Email solltest du mit filter_var($email, FILTER_VALIDATE_EMAIL) prüfen
5. Warum print_r? Wenn dann echo. Abgesehen davon haben Ausgaben innerhalb der Klasse sowieso nichts zu suchen.
6. Außerhalb genauso wenig. Zumindest nicht in Form von echo/print. Besser wäre es HTML Templates zu programmieren und diese mit PHP Ausgaben zu bestücken. Noch "schöner" wäre es eine Template Engine wie bspw. Twig zu verwenden.
7. Setter Methoden sollten immer mit "return $this;" enden, um Method chaining zu ermöglichen ("$obj->setName('name')->setEmail('[email protected]')");
8. Wo es Setter gibt, werden auch Getter erwartet. Die Properties kannst du dann auf protected statt public stellen.
9. Die validate-Methode sollte entweder true/false zurückgeben, ein Array aus Fehlern, oder wenn man meint sogar eine Exception werfen. Aber keine Ausgaben mit print/echo.
10. error_reporting in der Entwicklung abzudrehen ist schon mal ganz schlecht. Das sollte man in der Production Umgebung machen, aber auch dann nicht hardcoded sondern über entsprechend Einstellungen in der php.ini.
11. Ist es konzeptuell der falsche Ansatz eine Klasseninstanz für die Registrierung zu erstellen. Richtig wäre eine Instanz für den neuen Nutzer zu erstellen welche das verschlüsselte Passwort aus einem PasswordEncoder erhält, anschließend durch einen Validator validiert wird und dann, sofern alles passt, in der Datenbank gespeichert wird.

Ich könnte jetzt zwar noch ewig weitermachen, aber ich denke das reicht für den Anfang.
 
Ok danke!
Tolles Feedback habe mir jetzt alles durchgelesen und werde die einzelnen Punkte abarbeiten.
Sollte erst einmal eine Übung werden zwecks OOP aber ohne Feedback hätte ich so weiter gemacht o_O.

Wenn ich fertig bin poste ich das Script nochmals.

Schönen Abend noch!
Juare
 
Werbung:
Werbung:
*Schäm*
Mein Programm gibt mir innerhalb der Funktionen und Klassen nichts mehr aus genauso wenig wie Fehler was habe ich falsch gemacht würde mich über einen Tipp freuen :/

PHP: 
<?php
error_reporting( "E_ALL" );
ini_set('display_errors', 1);


$name = "Eldd33r"; // Später Übergabe mittels POST/FORMULAR
$password = "pdddd33sd";
$email = "[email protected]";
$error = false;

class INPUT_DATA_DATABASE{

    private $name;
    private $email;
    private $password;

    public function setName($bt_name) {
        $this->name = $bt_name;
        return $this->name;
     
    }

    public function setEmail($bt_email) {
        $this->email = $bt_email;
        return $this->email;
    }

    public function setPassword($bt_password) {
        $this->password = $bt_password;
        return $this->password;
    }
}

function check_register_data($email, $password, $name, $error)
{

  if (!filter_var($email, FILTER_VALIDATE_EMAIL) === false)
  {
     echo("$email is a valid email address");
  } else
  {
     echo("$email is not a valid email address");
     $error = true;
  }
  //////////////////////////////////////////////////
  if(strlen($password) === 0 OR strlen($password) < "6")
  {
    echo "Bitte Passwort eingeben!";
    $error = true;
  }else
  {
    echo "Passt";
  }
  //////////////////////////////////////////////////
  if(strlen($name) === 0 OR strlen($name) < "5")
  {
    echo "Bitte Passwort eingeben!";
    $error = true;
  }else
  {
    echo "Passt";
  }
  //////////////////////////////////////////////////
  if(!$error)
  {
      echo "Erfolgreich";

      $input_data_db = new INPUT_DATA_DATABASE();
      $input_data_db-> setName($name);
    $input_data_db-> setPassword($password);
    $input_data_db-> setEmail($email);

    }else
  {
    return "Fehler";
  }

  ////// ÜBERPRÜFUNG OB DATEN IN DER DATENBANK SCHON VORHANDEN: SYSTEM = "PDO ODER MYSQL_QUERY"?
}



?>
 
Juare schrieb:
*Schäm*
Mein Programm gibt mir innerhalb der Funktionen und Klassen nichts mehr aus genauso wenig wie Fehler was habe ich falsch gemacht würde mich über einen Tipp freuen :/

PHP: 
<?php
error_reporting( "E_ALL" );
ini_set('display_errors', 1);


$name = "Eldd33r"; // Später Übergabe mittels POST/FORMULAR
$password = "pdddd33sd";
$email = "[email protected]";
$error = false;

class INPUT_DATA_DATABASE{

    private $name;
    private $email;
    private $password;

    public function setName($bt_name) {
        $this->name = $bt_name;
        return $this->name;
   
    }

    public function setEmail($bt_email) {
        $this->email = $bt_email;
        return $this->email;
    }

    public function setPassword($bt_password) {
        $this->password = $bt_password;
        return $this->password;
    }
}

function check_register_data($email, $password, $name, $error)
{

  if (!filter_var($email, FILTER_VALIDATE_EMAIL) === false)
  {
     echo("$email is a valid email address");
  } else
  {
     echo("$email is not a valid email address");
     $error = true;
  }
  //////////////////////////////////////////////////
  if(strlen($password) === 0 OR strlen($password) < "6")
  {
    echo "Bitte Passwort eingeben!";
    $error = true;
  }else
  {
    echo "Passt";
  }
  //////////////////////////////////////////////////
  if(strlen($name) === 0 OR strlen($name) < "5")
  {
    echo "Bitte Passwort eingeben!";
    $error = true;
  }else
  {
    echo "Passt";
  }
  //////////////////////////////////////////////////
  if(!$error)
  {
      echo "Erfolgreich";

      $input_data_db = new INPUT_DATA_DATABASE();
      $input_data_db-> setName($name);
    $input_data_db-> setPassword($password);
    $input_data_db-> setEmail($email);

    }else
  {
    return "Fehler";
  }

  ////// ÜBERPRÜFUNG OB DATEN IN DER DATENBANK SCHON VORHANDEN: SYSTEM = "PDO ODER MYSQL_QUERY"?
}



?>
Zum Vergrößern anklicken....

Naja, das ist schon wieder nicht sauber...
Hier ein besserer Vorschlag.

Klassendefinition:
PHP: 
class User {

    private $id;
    private $name = '';
    private $email = '';
    private $password = '';
   
    public function setName ($name) {
        $this->name = $name;
       
        return $this;
    }
   
    public function getName ($name) {
        return $this->name;
    }
   
    ...
   
    public function validate () {
        $errors = [];
       
        if(!$this->email || !filter_var($this->email, FILTER_VALIDATE_EMAIL)) {
            $errors[] = 'Email ungültig';
        }
       
        if(!$this->name || strlen($this->name) < 5) {
            $errors[] = 'Kein Name bzw. zu kurz';
        }
       
        if(!$this->password) {
            $errors[] = 'Kein Passwort';
        }
       
        return $errors;
    }

}

Anwendung:
PHP: 
$user = new User;
$user
    ->setName($_POST['name'])
    ->setEmail($_POST['email'])
    ->setPassword(password_hash($_POST['password'], PASSWORD_DEFAULT));

if(count($errors = $user->validate()) > 0) {
    // fehler ausgeben   
} else {   
    // speichern   
}

Für eine wirklich saubere Lösung wäre aber ein Framework wie bspw. Symfony sinnvoll.
 
Zuletzt bearbeitet:
scbawik schrieb:
Naja, das ist schon wieder nicht sauber...
Hier ein besserer Vorschlag.

Klassendefinition:
PHP: 
class User {

    private $id;
    private $name = '';
    private $email = '';
    private $password = '';
 
    public function setName ($name) {
        $this->name = $name;
     
        return $this;
    }
 
    public function getName ($name) {
        return $this->name;
    }
 
    ...
 
    public function validate () {
        $errors = [];
     
        if(!$this->email || !filter_var($this->email, FILTER_VALIDATE_EMAIL)) {
            $errors[] = 'Email ungültig';
        }
     
        if(!$this->name || strlen($this->name) < 5) {
            $errors[] = 'Kein Name bzw. zu kurz';
        }
     
        if(!$this->password) {
            $errors[] = 'Kein Passwort';
        }
     
        return $errors;
    }

}

Anwendung:
PHP: 
$user = new User;
$user
    ->setName($_POST['name'])
    ->setEmail($_POST['email'])
    ->setPassword(password_hash($_POST['password'], PASSWORD_DEFAULT));

if(count($errors = $user->validate()) > 0) {
    // fehler ausgeben 
} else { 
    // speichern 
}

Für eine wirklich saubere Lösung wäre aber ein Framework wie bspw. Symfony sinnvoll.
Zum Vergrößern anklicken....


Jetzt verstehe ich auch was du gemeint hast x) mit get und set.
Versuche mich jetzt ein bisschen nach deinem Snippet zu richten vielleicht werde ich ja sauberer.
Und danke nochmals!
 
Werbung:
scbawik schrieb:
Am besten ein DBAL/ORM wie Doctrine.
Wie im Hintergrund mit der DB kommuniziert wird kann dir dann egal sein ;)

Achja, objektorientiert ist das ganze dann auch.
Zum Vergrößern anklicken....
Stimme ich dir voll und ganz zu aber meinst du nicht er sollte sich vorher ein bisschen mit PDO / MySQLi beschäftigen? Damit man wenigstens ein bisschen nachvollzieht, wie mit der DB kommuniziert wird.
 
nookie schrieb:
Stimme ich dir voll und ganz zu aber meinst du nicht er sollte sich vorher ein bisschen mit PDO / MySQLi beschäftigen? Damit man wenigstens ein bisschen nachvollzieht, wie mit der DB kommuniziert wird.
Zum Vergrößern anklicken....

Ich muss gestehen dass ich bisher überhaupt keinen Kontakt mit PDO bzw. der MySQLi-Klasse hatte :p
Bin damals eigentlich direkt vom prozeduralen MySQLi zu Doctrine gewechselt.

Der große Vorteil den ich im Nachhinein darin sehe ist halt, dass man gleich mit hochwertigem Code in Berührung kommt und dadurch viel mehr mitnehmen kann als durch selbsterdachte Konzepte und daraus resultierende Fehlschläge. Als Anfänger fehlt einem einfach die Erfahrung um zu beurteilen was gut und was schlecht ist. Darum finde ich es besser direkt mit etwas konfrontiert zu werden, von dem man annehmen kann dass es gut ist und das wenig Spielraum für Fehlkonzepte lässt.

Aber ja, wenn man sich mal auskennt hat man natürlich leicht reden :D
 
scbawik schrieb:
Ich muss gestehen dass ich bisher überhaupt keinen Kontakt mit PDO bzw. der MySQLi-Klasse hatte :p
Bin damals eigentlich direkt vom prozeduralen MySQLi zu Doctrine gewechselt.

Der große Vorteil den ich im Nachhinein darin sehe ist halt, dass man gleich mit hochwertigem Code in Berührung kommt und dadurch viel mehr mitnehmen kann als durch selbsterdachte Konzepte und daraus resultierende Fehlschläge. Als Anfänger fehlt einem einfach die Erfahrung um zu beurteilen was gut und was schlecht ist. Darum finde ich es besser direkt mit etwas konfrontiert zu werden, von dem man annehmen kann dass es gut ist und das wenig Spielraum für Fehlkonzepte lässt.

Aber ja, wenn man sich mal auskennt hat man natürlich leicht reden :D
Zum Vergrößern anklicken....

Sehe ich genau so wie du @scbawik.

Ich werde jetzt aber erstmal PDO nutzen und wenn ich da mal bisschen mehr Ahnung habe Doctrine anschauen. Ich denke mit OOP bin ich noch ein bisschen beschäftigt aber danke nochmal für eure Tipps :)
 
Werbung:
scbawik schrieb:
Ich muss gestehen dass ich bisher überhaupt keinen Kontakt mit PDO bzw. der MySQLi-Klasse hatte :p
Bin damals eigentlich direkt vom prozeduralen MySQLi zu Doctrine gewechselt.

Der große Vorteil den ich im Nachhinein darin sehe ist halt, dass man gleich mit hochwertigem Code in Berührung kommt und dadurch viel mehr mitnehmen kann als durch selbsterdachte Konzepte und daraus resultierende Fehlschläge. Als Anfänger fehlt einem einfach die Erfahrung um zu beurteilen was gut und was schlecht ist. Darum finde ich es besser direkt mit etwas konfrontiert zu werden, von dem man annehmen kann dass es gut ist und das wenig Spielraum für Fehlkonzepte lässt.

Aber ja, wenn man sich mal auskennt hat man natürlich leicht reden :D
Zum Vergrößern anklicken....
Eben, wenn man sich leicht auskennt hat man leicht reden. :D Aber wenn wir schon vor Ihm von DBAL/ORM reden, können wir Ihm auch ein Framework wie Symfony oder Zend Framework nahe legen. Ich habe durch diese Frameworks mehr über OOP gelernt, als es mit eignen Klassen zu machen.

Achja @Juare schau dir doch mal diesen Link an, finde Ihn hilfreich: http://www.phptherightway.com/
 
Du musst dich einloggen oder registrieren, um hier zu antworten.

Neueste Beiträge

Zurück
Oben