MVC, what is the best way to avoid code duplication?

Question

Yii

Hello everyone, I've been programming in yii for 2 years now, but I still don't know how to properly avoid code duplication in my controllers. All the time the same same type of question in all my projects. Maybe the habrasoobshchestvo can help me figure it out?
Example
Let's say we have a product catalog site where all products depend on the selected locality and may have reviews. An example from life is given in a very simplified form.
If we specify the task, we get:

1. The user enters the main page of the catalog and sees a list of countries. He can view info/statistics for the country or go to the area selection page for the selected country ( CountryController , Country ).
   
2. The region selection page works in a similar way, only with a check for the existence of the selected article ( RegionController , Region ).
   
3. Further, according to the same principle as described above, you can get to the city selection page, taking into account the check for the existence of an area ( CityController , City ).
   
4. Similarly, we get to the product page, the principle is the same as for the city ( ProductController , Product ).
   
5. We can see reviews of a product, the principle is the same ( ReviewController , Review ).
   

That is, it is obvious that all 5 controllers are of the CRUD type:

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

However, controllers of regions, cities, products, and reviews still need to check the top level from which the visitor (or an attacker) got to the page, for example, only an existing country can have regions (that is, idCountry is passed to RegionCountroller via $ _GET), etc. .
Obviously, for all 4 controllers, additional ones are still needed. methods for top level validation:

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

In total, 4 duplicate methods are obtained. How to avoid such duplication? Below are the solutions that I see. Can you suggest a better or better way?

1. Do not check the top-level model for existence - in my opinion, this is a sign of unprofessionalism
   
2. Move all 5 methods of the loadModel type to the base controller: CountryRegionCityProductReviewBaseController - everything is fine, but, logically, the loadCountry method is more related to CountryController, and I see no reason to drag it for CityController and others (you can also use the impurity or behavior mechanism, as it is called in yii ).
   
3. Move all 5 methods to a static helper class.
   
4. Make loadCountry static in CountryController, but a couple of people have already told me that this is bad, why - I still don’t understand.
   

PS: The code is written in yii, but it should be clear to those who use other frameworks.
PS2: This is my first entry on Habré, I tried to take into account all the rules for writing articles, if something goes wrong, I will be glad to listen.
PS3: Habr cut the introduction, rewrote it.

Answer 1

First of all, decide what you really want. It is not clear why the lack of verification of the parent is unprofessional. If it is not needed, then unprofessionalism is just writing it for the sake of some abstract idea. For example, for a region, it may be necessary to make a separate page if there is a country and it does not exist (“there is no such region for the specified country”). If all errors lead to the same inscription “page not found”, then additional checks are useless.
The next step is to decide on the implementation. But here you should already be more aware of what kind of architecture you have. In fact, everything proposed above comes down to the classic Strategy pattern: we leave all the general functionality in the parent class, and transfer all the particulars (the key point is that there should be few of them) to the heirs. In your case, the ancestor will be CRUDController with the implementation of methods tied to modelClass, and the descendants will be classes that define this modelClass and the checkRequest method. Jerking checkRequest prescribe where you like. I don’t know how Yii works, but if it allows you to create controllers of arbitrary classes without inheriting from anyone, then it makes sense to generally declare CRUDController abstract and write the checkRequest abstract method in it. Then it will be generally Strategy from the book, and you will get the opportunity to use checkRequest in other CRUDController methods without fear that it is not defined in the heirs. And usually in modern frameworks there is a method that twitches before any controller action. Perhaps there is the right place for your checkRequest.
Continuing to fantasize about the topic, you can make the type of the modelClass model passed in the parameter. Or receive it based on what you were given. You just need to make a map of valid values. Something like

array(
	...
	'region' => array( 'model' => 'regionModel', 'parent' => 'countryModel' ),
	'country' => array( 'model' => 'countryModel', 'parent' => null )
)

and from this map, get the model and parent by passing through the keys of the array and checking for the existence of a parameter with such a key. As soon as we find it, we check the existence of the parent and work on common methods with the already defined modelClass value.
Why does everyone advise doing Strategy. Because this thing has proven itself in battles. The classic OOP approach often brings more difficulties during development (you have to be more careful and often write more code), but it gives a noticeable gain if you need to change some part of the project. For example, the "fantastic" way is implemented faster (you need to add a couple of methods and a correspondence map, against the base class + 5 child classes for entities). But at the same time, if you need to add some new action (for example, adding a comment to a review), then in the case of a strategy, you simply enter it in the desired class. And for a simpler method, you will have to rack your brains with the invention of an exception in logic. You can do this a couple of times, but ... after 12 crutches, the code will turn into a pumpkin.
But in the end, it's up to you to decide. Maybe the second quick way is also suitable, because “nothing will change there for sure” (do not believe those who say so - it will definitely happen). Maybe you will cross them and introduce the standard implementation of checkRequest into the base class, and in the child classes you will define only modelClass and parentModelClass. It all depends on the specific needs and architecture of the project.

Answer 2

Yii has a nice extension mechanism. Up to redefining base classes. So, it could well be done not at the controller level. Make some analogue of the function of the checkAccess() method

Answer 3

I don’t know how with this in puff, but in java I would inherit from one class in which I would implement the necessary common functions

Answer 4

You did not take into account all the rules - you created it in the wrong section. :) Throw it into posts.

Answer 5

Well, this is a question, as it were, I myself do not know how to do it right. Does it make sense to transfer? Since the introduction is gone, the meaning is not entirely clear. I'll rewrite it now.

Answer 6

I would do it stupidly, in the forehead: I would write
a separate function (not a method, but just a function) that checks all four parameters and returns 0 - if everything is ok, or a value from 1 to 15, as flags for invalid parameters.

<?php
function check4($a, $b, $c, $d)
    {
    return check1($a)+check1($b)*2+check1($c)*4+check1($d)*8;
    }