How OOP is the code and what would you recommend to improve it?

Question

PHP

I recently started to understand OOP and MVC, so I really want to know if I'm on the right track and how to straighten it even more) I won't be verbose, here's my first model:

class Model_admin extends Model
{

  private $name;
  private $password;
  private $code;
  private $mail;

  public function get($login, $password) {

    $this->name = $login;
    $this->password = $password;

    $this->db_connect();

    $result = msql_query("SELECT 'login', 'password', 'mail' FROM 'admin_authorization'";
    $arr = mysql_fetch_array($result);

    do {

      if ($login === $arr['login']) {

        if (
            md5( md5( trim( $password ))) === $arr['password']
        ) {
          $this->mail = $arr['mail'];
          $this->random_code();
          return true;
        }

      }

    } while ($arr = mysql_fetch_array($res));

    return false;

  }

  private function random_code($length = 15) {

    $symbols = '0123456789abcdefghijklmnopqrstuvwxyz_-~!+*%$#&';

    for ($i = 0; $i < (int)$length; $i++) 
    {
      $num = rand (1, strlen ($symbols));
      $this->code .= substr ($symbols, $num, 1);  
    }

    $bool_update = mysql_query('UPDATE "admin_authorization" SET code="'.$this->code.'" WHERE login="'.$this->name.'"');

    if ($bool_update) $this->send_code();

  }

  private function send_code() {

    mail ($this->mail, "Admin code", $this->code);
  }

  public function check_code($user_code) {

    $this->code = mysql_query("SELECT 'code' FROM admin_authorization WHERE code='".$user_code."'");

    if ($this->code != '') return true;
  }

}

Answer 1

<?php
// PSR-1, PSR-2, PSR-4 Читаем и пользуем!
// namespace все дела... "Model_admin" - это прошлое.
// phpDocumentor - твой друг, прописывай всюду типы данных
class Model_admin extends Model
{
// Лишний перевод строки
    private $name;
    private $password;
    private $code;
    private $mail;
// Не информативное название. get model admin... что бы это значило...
    public function get($login, $password) {
// Где проверка аргумантов? Влететь может что угодно
        $this->name = $login;
        $this->password = $password;
// Модель НЕ должна управлять подключением к БД, это должно выполняться выше в коде
        $this->db_connect();
// Код вообще проверялся?)) у вас закрывающей строки нет.
        $result = msql_query("SELECT 'login', 'password', 'mail' FROM 'admin_authorization'";
// сие уже deprecated, забудьте про mysql_*** функции, используйте PDO
        $arr = mysql_fetch_array($result);
// Зачем нужен цикл, это дро*ба БД!!! Вытягиваете одну запись по логину и проверяете соответствует ли пароль
        do {
// Лишний перевод строки
            if ($login === $arr['login']) {
// Лишний перевод строки
                if (
                    md5( md5( trim( $password ))) === $arr['password']
                ) {
                    $this->mail = $arr['mail'];
                    $this->random_code();
// Перед return лучше делать перевод строки
                    return true;
                }
// Лишний перевод строки
            }
// сие уже deprecated, забудьте про mysql_*** функции, используйте PDO
// Присваивание в условиях управляющих кнострукций лучше не делать, это операции разного характера
        } while ($arr = mysql_fetch_array($res));

        return false;
// Лишний перевод строки
    }

    private function random_code($length = 15) {
// Где проверка аргумантов? Влететь может что угодно
        $symbols = '0123456789abcdefghijklmnopqrstuvwxyz_-~!+*%$#&';

        for ($i = 0; $i < (int)$length; $i++)
        {
            $num = rand (1, strlen ($symbols));
            $this->code .= substr ($symbols, $num, 1);
        }
// сие уже deprecated, забудьте про mysql_*** функции, используйте PDO
        $bool_update = mysql_query('UPDATE "admin_authorization" SET code="'.$this->code.'" WHERE login="'.$this->name.'"');
// Вот так писать плохо, всегда используйте фигурные скобки.
        if ($bool_update) $this->send_code();
// Лишний перевод строки
    }

    private function send_code() {
// Модель НЕ должна отправлять письма, под отправку обычно пишется отдельная подсистема/сервис
        mail ($this->mail, "Admin code", $this->code);
    }

    public function check_code($user_code) {
// Где проверка аргумантов? Влететь может что угодно
// SQL инъекция!!!!
// сие уже deprecated, забудьте про mysql_*** функции, используйте PDO
        $this->code = mysql_query("SELECT 'code' FROM admin_authorization WHERE code='".$user_code."'");
// Вот так писать плохо, всегда используйте фигурные скобки.
        if ($this->code != '') return true;
    }
// Лишний перевод строки
}

Answer 2

forget about MVC for a while, read about SOLID.
1) mysql_* functions are deprecated, use pdo (mysqli is too low-level stuff)
2) doing 2 times md5 doesn't make any sense. Now it is possible to generate billions of hashes per second on video cards, so it won't take much time to find a collision to a hash, and most of the passwords will be guessed in the first couple of hours. Use password_hash and password_verify. For PHP < 5.5 there is a flashback written in PHP.
3) you have violated the Single Responsibility Principle, this is about how OOP your code is. Well, the principle of dependency inversion is there ....

Answer 3

Hi colleague! Now it will start pouring down on you, I hope... My advice is don't be afraid and listen... At first I didn't perceive all these innovations, it took time.
But on the rights of the first answer ... if OOP has begun to comprehend - immediately implement PDO and prepared queries ...
- leave with mysql_query ,,,
mysql_query("SELECT 'code' FROM admin_authorization WHERE code='".$user_code."' ");
for this you will definitely "get" on the head) ...

Answer 4

use PDO, yes,
and spread the logic
in the model, there should not be different "SELECT * FROM", since you have already started talking about OOP,
make a separate DB class and use it ala - $DB->getOne('TABLE', array(PARAMS))
well, you can read about singleton for one thing

Answer 5

read fowler for starters

Answer 6

Quite on trifles if - you can enter all variables in one type separated by commas:
To work with a database (any), you need to use ready-made libraries, your own or third-party ones. They are regularly developed and maintained, no need to fence your bikes. The same applies to working with soap, for which PHP has the most powerful phpmailer library.
The rest of the colleagues have described in more than detail, there is no point in repeating.