MVC, как лучше избежать дублирование кода?

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

Пример

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

Если конкретизировать задачу, то получим:

  1. Пользователь заходит на главную страницу каталога и видит список стран. Он может просмотреть инф-ю/статистику по стране или перейти на страницу выбора области для выбранной страны (CountryController, Country).


  2. Страница выбора области работает по похожему принципу, только с проверкой на существование выбраной статьи (RegionController, Region).


  3. Далее по тому же принципу что описан выше можно попасть на страницу выбора города, с учетом проверки на существование обрасти (CityController, City).


  4. Аналогично попадаем на страницу товара, принцип тот же что и для города (ProductController,Product).


  5. У товара мы можем посмотреть рецензии, принцип все тот же (ReviewController, Review).






То есть очевидно что все 5 контроллеров типа CRUD:

<font color="black"><font color="#0000ff">class</font> CRUDController {<br/>
  <font color="#008000">// тут получаем список всех моделей и передаем вьюхе</font><br/>
  <font color="#0000ff">public</font> function actionIndex(){}<br/>
  <font color="#008000">// страница просмотра инф-ии о модели</font><br/>
  <font color="#0000ff">public</font> function actionView(){}<br/>
  <font color="#008000">// тут обрабатываем создание модели</font><br/>
  <font color="#0000ff">public</font> function actionCreate(){}<br/>
  <font color="#008000">// тут редактирование</font><br/>
  <font color="#0000ff">public</font> function actionUpdate(){}<br/>
  <font color="#008000">// и удаление</font><br/>
  <font color="#0000ff">public</font> function actionDelete(){}<br/>
  <font color="#008000">// к этому методу обращаются view, update и delete</font><br/>
  <font color="#0000ff">protected</font> function loadModel($id){<br/>
     $model=CActiveRecord($<font color="#0000ff">this</font>-&gt;modelClass)-&gt;findByPk($id);<br/>
     <font color="#0000ff">if</font>(!isset($model))<br/>
         <font color="#0000ff">throw</font> <font color="#0000ff">new</font> CHttpException(404,<font color="#A31515">'страница не найдена'</font>);<br/>
  }<br/>
}<br/>
</font><br/>
<font color="gray">* This source code was highlighted with <a href="http://virtser.net/blog/post/source-code-highlighter.aspx"><font color="gray">Source Code Highlighter</font></a>.</font>


Однако контроллерам областей, городов, товаров и рецензий еще нужно проверять верхний уровень, с которого посетитель (или злоумышленник) попал на страницу, например, области могут быть только у существующей страны(то есть RegionCountroller у через $_GET передается idCountry) и т.д.

Очевидно что для всех 4-х контроллеров еще нужны доп. методы для проверки правильности верхнего уровня:

  1. RegionController — loadCountry
  2. CityController — loadRegion
  3. ProductController — loadCity
  4. ReviewController — loadReview




Итого получается 4 дублированных метода. Как избежать такого дублирования? Ниже варианты которые решения которые я вижу. Может посоветуете способ лучше или какой правильней?

  1. Не проверять модель верхнего уровня на существование — по моему, это признак непрофессионализма


  2. Вынести все 5 методов типа loadModel в базовый контроллер: CountryRegionCityProductReviewBaseController — все хорошо, но, логически метод loadCountry больше относится к CountryController у, и не вижу смысла тащить его для CityController и прочих (можно использовать и механизм примесей или поведения, как это называют в yii).


  3. Вынести все 5 методов в статический класс-хелпер.
  4. Сделать loadCountry статическим у CountryController, но, пару людей уже подсказали мне что это плохо, почему — я так и не понял.


P.S.: Код написан под yii, но должен быть понятен и тем кто использует другие фреймворки.

P.S.2: Это моя первая запись на хабре, попробовал учесть все правила написания статей, если что не так — буду рад выслушать.

P.S.3: Хабр срезал вступление, переписал его.
  • Вопрос задан
  • 3672 просмотра
Решения вопроса 1
@bioroot
Первым делом определитесь чего вы хотите на самом деле. Не понятно почему отсутствие проверки родителя — непрофессиональность. Если она не нужна, то непрофессиональность как раз её написание ради какой-то абстрактной идеи. Например, для региона может быть нужно сделать отдельную страницу при наличии страны и отсутствии его самого («для указанной страны не существует такого региона»). Если все ошибки ведут на одинаковую надпись «страница не найдена», то дополнительные проверки ни к чему.

Далее надо определиться с реализацией. Но тут уже вам должно быть виднее какая у вас архитектура. По сути всё предложенное выше сводится к классическому паттерну Strategy: весь общий функционал оставляем в родительском классе, а все частности (ключевой момент — их должно быть мало) переносим в наследников. В вашем случае предком будет CRUDController с реализацией метеодов завязанной на modelClass, а потомками классы определяющие этот modelClass и метод checkRequest. Дёрганье checkRequest прописывайте где вам нравится. Не знаю как устроен Yii, но если он позволяет создавать контроллеры произвольных классов не наследуясь ни от кого, то имеет смысл вообще объявить CRUDController абстрактным и в нём же прописать абстрактный метод checkRequest. Тогда это будет вообще Strategy из книжки, а вы получите возможность использовать checkRequest в других методах CRUDController не опасаясь того что в наследниках он не определён. И ещё обычно в современных фреймворках есть метод, который дёргается перед любым экшеном контроллера. Возможно, там вашему checkRequest самое место.

Продолжая фантазировать на тему, можно сделать тип модели modelClass передаваемым в параметре. Или получать его исходя из того что вам передали. Только надо сделать карту допустимых значений. Что-нибудь типа
array(
	...
	'region' => array( 'model' => 'regionModel', 'parent' => 'countryModel' ),
	'country' => array( 'model' => 'countryModel', 'parent' => null )
)

и из этой карты получать модель и родителя проходя по ключам массива и проверяя на существование параметра с таким ключом. Как только нашли — проверяем существование родителя и работаем по общим методам с уже определённым значением modelClass.

Почему все советуют делать Strategy. Потому что эта штука зарекомендовала себя в боях. Классический ООП подход часто приносит больше трудностей при разработке (надо быть аккуратнее и чаще всего писать больший объём кода), но даёт заметный выигрыш при необходимости изменить какую-то часть проекта. К примеру, «нафантазированный» способ реализуется быстрее (надо добавить пару методов и карту соответствия, против базового класса + 5 классов дочерних для сущностей). Но при этом если понадобиться добавить какой-то новый экшн (например, добавление комментария к ревью), то в случае со стратегией вы просто впишите его в нужный класс. А для более простого метода придётся ломать себе голову с изобретением исключения в логике. Пару раз так можно сделать, но… после 12 костылей код превратится в тыкву.

Но, в конечном итоге, решать всё-равно вам. Может быть и второй быстрый способ подойдёт, потому что «там стопудова ничего не будет меняться» (не верьте тем кто так говорит — обязательно будет). Может вы их скрестите и внесёте в базовый класс стандартную реализацию checkRequest, а в дочерних классах будете определять только modelClass и parentModelClass. Всё зависит от конкретных потребностей и архитектуры проекта.
Ответ написан
Пригласить эксперта
Ответы на вопрос 5
Fr3nzy
@Fr3nzy
В Yii есть хороший механизм расширений. Вплоть до переопределения базовых классов. Так что, это вполне можно было бы сделать и не на уровне контроллеров. Сделать какой-нибудь аналог функции метода checkAccess()
Ответ написан
asm0dey
@asm0dey
Не знаю, как с этим в пыхе, но в java я бы унаследовался от одного класса, в котором реализовал бы необходимые общие функции
Ответ написан
Necro
@Necro
Правила все ты не учёл — не в том разделе создал.: ) Перекинь в посты.
Ответ написан
Комментировать
@Ekstazi Автор вопроса
Ну это вопрос как бы, я сам не знаю как правильно. Есть ли смысл перекинуть? Так как вступление пропало, то и смысл не до конца ясен. Сейчас перепишу его.
Ответ написан
Комментировать
Kindman
@Kindman
Я бы сделал по-тупому, в лоб:
Написал бы отдельную функцию (не метод, а просто функцию), которая проверяет все четыре параметра, и возвращает 0 — если все ок, либо значение от 1 до 15, как флаги неверных параметров.
<?php
function check4($a, $b, $c, $d)
    {
    return check1($a)+check1($b)*2+check1($c)*4+check1($d)*8;
    }
Ответ написан
Ваш ответ на вопрос

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

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