Strarting with OOP [PHP]

[Cruorzy]

So im starting with OOP in my first language (PHP)

 

And im trying to figure out the best practice, understand im not that advanced to if you would keep the answers simple and nice

 

So im using the __construct to setting the data to variables but im not sure where to put my passwordLength check and nameLength check.

And once they are TRUE it will send a query to the DB to create the user.

 

Im assuming this would be done in __construct or should i make a new function for it?

So i can use the User class also to retrieve database data use it for a Login function?

 

class User{
  private  $id;
  private  $name;
  private  $email;
  private  $password;

  //Setting minimal and maximal values of name and password
  protected static $minPasswordLength = 5;
  protected static $maxPasswordLength = 15;
  protected static $minNameLength = 4;
  protected static $maxNameLength = 10;

  public function __construct($id, $name, $email, $password){
    $this->id         = $id;
    $this->name       = $name;
    $this->email      = $email;
    $this->password   = $password;


    //Have my doubts if this is a good practice.
    if($this->checkPasswordLength() === true){
      if($this->checkNameLength() === true){
        //If the 2 checks are True then create user
        echo 'Creating user';
        //database stuff etc..
      }
      else {
        echo $this->checkNameLength();
      }
    }
    else {
      echo $this->checkPasswordLength();
    }

  }

  public function checkPasswordLength()
  {

      if (strlen($this->password) > User::$maxPasswordLength) {
        return "Password is too long";
      }
      elseif (strlen($this->password) < User::$minPasswordLength) {
        return "Password is too short";
      }
      else{
        return true;
      }

  }

  public function checkNameLength()
  {

      if (strlen($this->name) > User::$maxNameLength) {
        return "Name is too long";
      }
      elseif (strlen($this->name) < User::$minNameLength) {
        return "Name is too short";
      }
      else{
        return true;
      }

  }

}

//Creates a new user with the following arguments
//UserID    = 1
//Usermame  = Cruorzy
//Email     = cruorzy@example.com
//Password  = Cruorzy123
$user = new User(1, 'Cruorzy', 'cruorzy@example.com', 'Cruorzy123');

?>

 

[colonel_mortis]

Generally, you would structure it slightly differently, so that you can reuse the same class when you are creating a new member as well as when loading an existing member from the database.

To do that, you don't insert into the database in __construct, and nor should you check the password there. Instead, construct should mostly just set the values that you need in the class (such as setting $this->name, $this->email, $this->password), then you have another (public) method such as isValid that will check the username is valid and not already used, make sure the password meets the requirements, etc, and another method that saves the member into the database. That means that when you load an existing user from the database, the checks won't be run on their data, so if you change the requirements later on, existing users will continue to work, even if they don't meet the new requirements.

 

As an aside, from that code it looks like you might store the plaintext (or conventionally encrypted) password in the database. DO NOT do this. Passwords need to be hashed using a one-way hash function so that there is no way for an attacker to access the plaintext passwords even if they access your database and steal any encryption keys. To do this, you want to use the PHP password_hash and password_verify functions.

[SSL]

• The constructor should do little if any work besides setting member fields. Move your business logic into separate methods that will be called when needed.
  • This makes the instantiation of your class more declarative and less procedural, which is the whole point of OO in the first place.
• Use exceptions to propagate errors to the part of the application that is responsible for handling them.
  • This will maintain the single responsibility principle. The User class should not be responsible for handling errors. The User class should not be responsible for displaying output to the end user.
  • This will ensure a standard interface for errors and preserve encapsulation. Mixed return types are one of the evils of PHP because you have to perform a strictly equals check on the result of "checkPasswordLength" which adds boilerplate and increases the opportunity for errors in your code.
• Continuing with the above point, I would argue that the checks for name and password length do not belong in the User class at all. These constraints are really needed to conform to the database schema, at least when it comes to password length and name max length. See my suggestion below for where to move these validations.
• The constants for name and password lengths probably don't belong in the User class, either. Again, this is probably more related to the Database schema or hash function used for passwords. What if these values need to be changed at runtime?

As far as organization goes, I would write a UserCollection class for reading one or more users from the database.

• It would have an iterate() method, with optional parameters for filtering the SQL query. This method would return some kind of collection of User objects.
• It would have an add() method for inserting a single new User into the database. I think that this would be a more logical place to put validation checks on name and password length.
• Avoid creating isValid type methods for usernames and passwords. It is unnecessary boilerplate and blurs the responsibility of the class. If you find the need to do this, I would actually create separate classes for handling usernames and passwords.
• And, as I suggested above, use exceptions to propagate errors.

The User class would then be responsible for holding data on a single user. It will be a much lighter class until you find that you need to add more behavior, which can be done with the Decorator pattern.

[Cruorzy]

10 minutes ago, colonel_mortis said:

Generally, you would structure it slightly differently, so that you can reuse the same class when you are creating a new member as well as when loading an existing member from the database.

To do that, you don't insert into the database in __construct, and nor should you check the password there. Instead, construct should mostly just set the values that you need in the class (such as setting $this->name, $this->email, $this->password), then you have another (public) method such as isValid that will check the username is valid and not already used, make sure the password meets the requirements, etc, and another method that saves the member into the database. That means that when you load an existing user from the database, the checks won't be run on their data, so if you change the requirements later on, existing users will continue to work, even if they don't meet the new requirements.

So i just create all of the checks in methods and then lets say in the registration.php ill do a few if statements and if they are all good then call the method to actually save it to the DB? Because i was thinking of that but not sure what the best practice would be in OOP.

 

12 minutes ago, colonel_mortis said:

As an aside, from that code it looks like you might store the plaintext (or conventionally encrypted) password in the database. DO NOT do this. Passwords need to be hashed using a one-way hash function so that there is no way for an attacker to access the plaintext passwords even if they access your database and steal any encryption keys. To do this, you want to use the PHP password_hash and password_verify functions.

Im aware of hasing/encrypting just wanted to keep it as simple as possible.

 

I might rewrite a bit and post here again just to see if someone has some more points to point out

[SSL]

4 minutes ago, Cruorzy said:

So i just create all of the checks in methods and then lets say in the registration.php ill do a few if statements and if they are all good then call the method to actually save it to the DB? Because i was thinking of that but not sure what the best practice would be in OOP.

 

Nope, too procedural. Checks should be done in the method called for saving to the database, which should throw an exception if the checks fail.

[Cruorzy]

23 minutes ago, SSL said:

• Snip

Understood roughly 70% at least i think, havent worked with error exceptions but this would be a good chance, when i think i actually achieved some of your points i might post some code again. Might be completely wrong but i'll do my best.

 

Thanks for the advice and the tons of work you gave me again (for me at least till i get familair with it)

 

Just a quick question after googling a bit, 

27 minutes ago, SSL said:

Use exceptions to propagate errors to the part of the application that is responsible for handling them.

• This will maintain the single responsibility principle. The User class should not be responsible for handling errors. The User class should not be responsible for displaying output to the end user.

 

When the check fails and there might be some error do i use 

throw new Exception('Some sort of error');

that will actually pass it onto a new method? and that method will finally display the error.

Kind of lost here.

[SSL]

1 minute ago, Cruorzy said:

Understood roughly 70% at least i think, havent worked with error exceptions but this would be a good chance, when i think i actually achieved some of your points i might post some code again. Might be completely wrong but i'll do my best.

 

Thanks for the advice and the tons of work you gave me again (for me at least till i get familair with it)

 

Just a quick question after googling a bit, 

When the check fails and there might be some error do i use 

throw new Exception('Some sort of error');

that will actually pass it onto a new method? and that method will finally display the error.

Kind of lost here.

 

The exception is propagated up to the calling method, where it can be caught and handled or thrown again. It's a little bit like returning from the function, but the control flow isn't the same. http://php.net/manual/en/language.exceptions.php

[Cruorzy]

8 minutes ago, SSL said:

The exception is propagated up to the calling method, where it can be caught and handled or thrown again. It's a little bit like returning from the function, but the control flow isn't the same. http://php.net/manual/en/language.exceptions.php

I might have a better understanding now, i dont know if this should be rather easy to understand but it seems kind of hard i guess, dont get me wrong i having fun doing this.

Anyways i'll try somethings out probably quote you when im a bit futher. It might be completely different from what you said but we shall see that later on i guess

 

Thanks for the great advice!