@tryvols
Front-End разработчик

Насколько код ООП и что бы вы посоветовали по его улучшению?

Недавно начал разбираться в ООП и MVC, по этому очень хочу узнать на правильном ли я пути и как его еще больше выпрямить) Не буду многословен, вот моя первая модель:

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;
	}

}
  • Вопрос задан
  • 681 просмотр
Пригласить эксперта
Ответы на вопрос 6
index0h
@index0h
PHP, Golang. https://github.com/index0h
<?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;
    }
// Лишний перевод строки
}
Ответ написан
Fesor
@Fesor
Full-stack developer (Symfony, Angular)
забейте на время на MVC, почитайте про SOLID.

1) mysql_* функции депрекейтед, используйте pdo (mysqli слишком низкоуровневая штука)
2) делать 2 раза md5 не имеет никакого смысла. Сейчас можно генерить миллиарды хэшей в секунду на видеокартах так что подбор коллизии к хэшу займет не сильно много времени и большая часть паролей будет подобрана за первые пару часов. Используйте password_hash и password_verify. Для PHP < 5.5 есть флэбэк написанный на PHP.
3) вы нарушили принцип единой ответственности, это к вопросу о том насколько ваш код ООП. Ну и принцип инверсии зависимостей туда же....
Ответ написан
@dk-web
Привет, коллега! Сейчас тебе посыпется, надеюсь... Мой совет - не бойся и прислушайся... я сначала не воспринимал все эти новшества, понадобилось время.
Но на правах первого ответа... если уж ООП стал постигать - сразу внедряй PDO и подготовленные запросы...
- уходи с mysql_query,,,
mysql_query("SELECT 'code' FROM admin_authorization WHERE code='".$user_code."'");
вот за это точно по голове "получишь")...
Ответ написан
oOLokiOo
@oOLokiOo
PHP Developer
используйте PDO, да
ну и логику разносите
в моделе не должно быть разных "SELECT * FROM", раз вы уже про ООП заговорили
делайте отдельный класс DB и используйте его аля - $DB->getOne('TABLE', array(PARAMS))
ну и про singleton можете почитать за одно
Ответ написан
trevoga_su
@trevoga_su
почитай фаулера для начала
Ответ написан
Комментировать
Acuna
@Acuna
Заполнил свой профиль
Совсем по мелочам если - все переменные можешь ввести в одном типе через запятую:

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

Для работы с БД (любыми) нужно пользовать готовые библиотеки, свои или сторонние. Они регулярно развиваются и поддерживаются, не нужно городить свои велосипеды. Это же касается и работы с мылом, для него в PHP имеется мощнейшая библиотека phpmailer.
Остальное все коллеги более чем подробно расписали, нет смысла повторяться.
Ответ написан
Комментировать
Ваш ответ на вопрос

Войдите, чтобы написать ответ

Похожие вопросы