Правильно ли я понимаю MVC?

Question

[Эдуард]

Знакомый попросил написать небольшую систему для управления лицензиями(сам он пишет на си). Я решил писать с использованием MVC. Вопрос такой - правильно ли я понял этот паттерн, вот пример авторизации на сайте:
Controller_Login.php

<?php

class Controller_Login extends Controller
{
	function __construct()
	{
		Session::init();
		$logged = Session::get('login');
		if($logged == false) {	
		$this->model = new Model_Login();
		$this->view = new View();
		}
		else header('Location: ../main');			
	}
	
	function action_index()
	{
		if(!empty($_POST['login'])&& !empty($_POST['password'])){
				$data = $this->model->login($_POST['login'],$_POST['password']);	
		}		
		$this->view->generate('login.php',array('data' => $data,'title'=> "Авторизация"));
	}
	
	function action_logout()
	{			
		$this->model->logout();
	}
	
}

Model_Login.php

<?php

class Model_Login extends Model
{
	public function login($login, $password)
    {
		$link = self::db();	
		$login    = $link->real_escape_string(htmlspecialchars($login));
		$password = $link->real_escape_string(htmlspecialchars(md5(md5($password))));
		$query    = "SELECT user_id, email, user_group,name FROM dle_users WHERE name= '$login' AND password = '$password' LIMIT 1";
		$sql = $link->query($query);
		if (mysqli_num_rows($sql) == 1) {
			$row = $sql->fetch_assoc();
			if ($row['user_group'] == 7 || $row['user_group'] == 1) {						
				Session::set('id',$row['user_id']);  
				Session::set('email',$row['name']);  
				Session::set('group',$row['user_group']);  
				Session::set('login',$row['email']);  	
				header("Location: ../main");
			} else{
				$data['msg']= "Ваша группа не соответствует требованиям!";							
			}
		} 
		else {			
			$data['msg']= "Неверное имя пользователя или пароль. Проверьте правильность введенных данных!";					
		}
		$link->close();
		return $data;
	}	
}

Надо ли было создавать глобальные переменные в модели, и делать сеттеры и геттеры?

Answer 1

[Сергей Протько]

нет, вы не правильно поняли MVC.

Попробуйте подойти к проблеме чуть с другой стороны. Реализуйте ваше приложение так, что бы оно ничегошеньки не знало о HTTP, внутри него небыло ни единого echo, оно ничего не знало о сессиях и вообще ничего не знал о таких вещах. Проще всего этого добиться - ваше приложение можно запускать через CLI. Грубо говоря как-то так:

<?php
// run.php - просто скрипт для разового теста

require __DIR__ . '/vendor/autoload.php'; // вы же уже используете composer?

$app = new App();
$app->getService('login_handler')->login('[e-mail]', 'password');

Что-то типа такого. То есть на каждый "юз кейс", то есть то что приложение должно делать, у вас должен быть метод. Типа "сделай то-то" и "сделай еще что-то".

Вот это - наше приложение, наша "модель". Не один какой-то класс... это все приложение. MVC же ставит перед собой задачу снижения связанности между UI и приложением. Цель при этом приследуется простая - UI обычно меняется намного чаще чем логика приложения, а стало быть лучше их друг от дружки отделить целиком и полностью.

Делается это за счет того что между UI и приложением вводят дополнительный промежуточный слой адаптеров - контроллеры (это опять же не обязательно один объект, это может быть целая цепочка адаптеров, каждый из которых делает что-то конкретное, в плодь до последнего адаптера который уже конвертирует http запрос в нужный вызов нужного метода).

То есть что бы сказать "я сделал MVC" у вас приложение не должно зависить от UI. Если вы хоть где-то в приложении используете суперглобальные массивы, и т.д. вы проиграли. Ну либо просто не называйте это MVC, скажите что вы просто шаблоны отдельно ложите ну и роутер еще есть. Но это не MVC, это smartui, то есть наше приложение вкурсе что у него есть UI и они сильно связаны.

MVC нужно далеко не всем, и smartui сойдет для простых проектов. Но вы должны понимать разницу, и знать когда стоит загоняться а когда можно логику в контроллеры выносить.

Надо ли было создавать глобальные переменные в модели

Это вы еще не уяснили значит что такое ООП, почему глобальное состояние плохо и что такое побочные эффекты (погуглите в контексте состояния).

делать сеттеры и геттеры?

А этим мы нарушаем инкапсуляцию. Внешний мир должен знать только что можно делать с моделью, но никак не ее структуру. То есть вместо setSomething у вас должно быть осмысленное название, типа updateSomething, changeSomething и т.д. Типа "user should be able to change password" и у вас появляется метод "changePassword". Или "User should be able to update profile details" и у вас появляется один единственный метод "updateProfileDetails()". А что оно как состояние меняет - это консерн исключительно объекта. Ему рашеть менять чего или нет. Мы таким образом изолируем побочные эффекты и уменьшаем вероятность багов. Ну и нам не нужно валидировать при таком раскладе ничего так как нет промежуточного невалидного состояния.

Comments

[Эдуард]

Большое спасибо за ответ, сейчас перевариваю прочитанное. Но куда тогда перенести http и сессии, и вообще стоит ли все так усложнять, какая польза будет от этих изменений?

[Сергей Протько]

Эдуард: я жирным основную мысль пометил.

Работа с HTTP - слой адаптеров, контроллеры и т.д. Задача контроллера - конвертировать асинхронное действие пользователя (будь то клик по мышке, http запрос от браузера и т.д.) в синхронный вызов метода модели. Причем по хорошему у нас должен быть ровно один вызов метода модели в контроллере. Тут стоит опять же уточнить - имеется в виду для изменения состояния. То есть если мы вызвали метод что бы поменять состояние и потом хотим вызвать метод что бы забрать какое-то состояние, дабы ответ сформировать - это более чем ок.

Вообще хорошая идея разделять методы на те, что только пишут и те что только читают. Это сильно снижает сложность кода, тестировать такой код одно удовольствие, масштабируется система лучше, а багов становится меньше.

[Сергей Протько]

Эдуард: еще рекомендую глянуть концепцию middlewares в laravel например. Это те самые адаптеры о которых я говорю. Скажем маршрутизация - так же просто еще один адаптер на пути к вашему контроллеру, который является... по сути последним адаптером в цепочке.

[Сергей Протько]

vlad tu:

Ну вот у вас типичный случай - вам надо что-то создать и потом это что-то отдать обратно пользователю. В этом случае выгоднее использовать отдельно метод, который создает, и отдельно метод, который забирает.

Тут соль в чем, вопервых код сильно упрощается, во вторых завтра вам понадобится не только созданный объект отдать, а еще какую-то доп информацию, и если нет разделения - придется править существующий код, а если бы разделение было - вы бы просто написали новый метод который забирает какие-то данные, а метод который добавляет что-то оставался бы без изменений. Таким образом мы защищаем себя от правок кода. А чем меньше правок - тем меньше шансов сломать код. Ну и тестирование это дико сильно упрощает.

Есть конечно ситуации, когда разделить не выйдет. Например работа со стэком (array_pop) который должен атомарно поменять массив и вернуть удаленное значение. Но вы просто должны делать так, что бы таких штук было по возможности меньше. Принцип "работает не трогай" если хотите, вместо того что бы менять всегда лучше добавить.

[Сергей Протько]

vlad tu: вот только в вашем примере это не один вызов. Должно быть тогда уже что-то типа:

$article = Article::create($title, $content);

Тут как бы все довольно просто. А теперь представьте себе что действие что-то делает. Например метод, который публикует статью. Или вносит обновление. И вот вам надо сделать механизм ревизий, и будет уже намного проще, если операция обновление и операция "взять конкретную ревизию" - это будет две отдельные операции.

А вообще - это уже чуть другой уровень судя по вашему коду. Вам же пока рекомендую почитать чего на тему состояния, побочных эффектов, подумать откуда берутся баги и почитать про инкапсуляцию. А ну и еще - почитайте про row data gateway - вы по сути реализовали этот паттерн (а не active record).

[Сергей Протько]

vlad tu: у вас логика сущностей по ревизиям вытекла в нейкий RevisionManager. Вот о чем я говорю. То есть поведение, управляющее состоянием объекта Article теперь находится у RevisionManager. Полностью нарушена инкапсуляция. По сути это процедурщина, а не ООП. Но да, так большинство и делают.

[Сергей Протько]

vlad tu: помимо принципа единой ответственности есть еще такая замечательная штука как "высокое зацепление". А у вас зацепление выходит низким.

Поймите, важно не то где поведение находится, а кто знает о внутреннем состоянии объектов. Внешний мир понятия не должен иметь о том, как статья хранит свои ревизии. Их интересуют только конечное состояние. И весь фокус в том, что бы спрятать как можно больше, что бы в случае если мы вместо хранения полноценных копий будем хранить diff-ы текста, нам не пришлось много менять.

[Сергей Протько]

vlad tu: почитайте википедию, есть несколько видов зацепления. Вам нужно высокое зацепление по данным.

короч просто подумайте, нужно ли выносить логику из сущности куда-то наверх, если вся логика покрывает только работу с одной единственной сущностью.

[Сергей Протько]

vlad tu: нет, ревизия - это деталь реализации статьи по сути. Хотя случаи могут быть разными конечно.

Самый распространенный пример - оформление заказов в интернет магазине, когда у вас есть объект Order, который хранит OrderItem, который хранит ссылку на продукт и количество. А может быть не на продукт а на его копию для архива. А может еще чего. Словом это уже детали реализации. Мы же в основном работаем с Order-ом.

Answer 2

[jacksparrow]

Кратко: если проект пишите для реальной задачи - возьмите фреймворк

Comments

[Эдуард]

Много раз пытался понять и научится использовать фреймворки, но ни в какую не получается...

[jacksparrow]

Эдуард: я даже затрудняюсь вам ответить , в вебе без фреймворков почти никуда

Answer 3

[trevoga_su]

htmlspecialchars

ответь сам себе на вопрос - каким боком функция замены символов, являющихся частью html, у тебя присутствует при работе с БД? Это бред.

Вообще, что бы написать в MVC стиле, нужно иметь базу ООП знаний и, в целом, надо иметь то ли фрейморк, то ли просто код твой должен быть разделен на слои... без этого писать на MVC по большей части затея бессмысленная.

Сейчас у тебя типичный код, обернутый в классы.

Ну и на www.phpinfo.su/articles/theory/model_view_controll...

Comments

[Эдуард]

"Сейчас у тебя типичный код, обернутый в классы." - аргументируйте ваше высказывание. Если вы так считаете то и статья на хабре которую я использовал при написании этого сайта тоже не подходит под MVC?