Правильно ли реализован класc?

Question

[Вадим Сибыч]

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

<?php

if ( !defined( 'FILE_COMPILE' ) )
  exit;

//	Developed by Vadim Kondakov,
//	Copyright 2016, SibWeb Group,
//	License GPL v 2.

if ( !class_exists ( 'Controller' ) ) {

  class Controller extends UDAPI {

    public $data = array ( );

    protected $uri;
    protected $pattern = array();
    protected $params = array();
    protected $request = array();

	public $controller;
	public $action;

    public $default = array ( 
      'controller' => 'home',
      'action' => 'index'
    );

	public function __construct ( ) {
      // Получаем и сразу фильтрируем данные URI
	  $this->uri = filter_input ( INPUT_SERVER, 'REQUEST_URI', FILTER_SANITIZE_URL );
	  $this->uri = trim ( $this->uri, '/\\' );
	  $this->uri = urldecode ( $this->uri );
	  $this->uri = parse_url ( $this->uri, PHP_URL_PATH );
	  if (!$this->uri) { return; }
	  // Разбиваем в массив...
	  $this->pattern = array_filter ( explode ( '/', $this->uri ), 'mb_strlen' );

      if ( $this->uri == '' )
        $this->redirect ( $this->config->root_uri . $this->default[ 'controller' ] . '/' . $this->default[ 'action' ], true );
	}

    public function ready (  ) {
	  if ( !is_bool ( $this->uri ) ) {
        try {
          if ( sizeof ( $this->pattern ) % 2 )
            throw new Exception ( );

          $this->setController ( array_shift ( $this->pattern ) );
          $this->setAction ( array_shift ( $this->pattern ) );

          if ( is_array ( $this->pattern ) ) {
            for ( $i = 0; $i < sizeof ( $this->pattern ); $i++ )
              $this->params[ $this->pattern[ $i ] ] = $this->pattern[ ++$i ]; 
          }
		  
          // Запускаем...
          $this->getModel ( $this->controller );
          $controller = $this->getController ( ); 
	  
          if ( $this->getAction ( $controller ) ) {
            if ( is_array ( $this->params ) ) {
              foreach ( $this->params as $key => $value )
                $this->request[ $key ] = $value;
            }
            call_user_func_array ( array ( $controller, $this->action ), $this->request );
          } else {
            echo '404 error';
          }
		  
		} catch ( Exception $e ) {
		  echo 404;
		}
	  }

	}

    /**
     * Проверка существование контроллера
     * @param string $controller
     * @return boolean
     */
	public function isController ( $controller ) {
	  if ( is_dir ( SOURCE_DIR . '/controllers/' . $controller . '/' ) ) {
		$query = $this->model->db->prepare ('SELECT * FROM `controllers` WHERE `name` = :name');
		$query->execute ( array ( 'name' => $controller ) );
		// получаем и сохраняем данные о контроллере...
		$this->data = $query->fetch ( PDO::FETCH_ASSOC );
        // включен ли контроллер в базе?
		if ( $this->data[ 'is_enable' ] == 1 )
		  return true;
	  }
	  return false;
	}

    /**
     * Устанавливает текущий контроллер
     * @param string $controller
     * @return string
     */
    public function setController ( $controller ) {
      if ( trim ( $controller ) )
        $this->controller = $controller;
      else
        $this->controller = $this->default[ 'controller' ];
    }

    /**
     * Подключает текущий контроллер
     * @return object
     */
    public function getController (  ) {
	  if ( $this->isController ( $this->controller ) ) {
        require_once ( SOURCE_DIR . '/controllers/' . $this->controller . '/controller.php' );
		$class = $this->controller . 'Controller';
        if ( class_exists ( $class ) )
		  return new $class( );
	  }
	  return false;
	}

    /**
     * Проверка существование модели
     * @param string $model
     * @return boolean
     */
    public function isModel ( $model ) {
      return is_file ( SOURCE_DIR . '/controllers/' . $model . '/model.php' );
	}

    /**
     * Подключение существующей модели
     * @param string $controller
     * @return object
     */
    public function getModel ( $controller ) {
      if ( $this->isModel ( $controller ) ) {
        require_once ( SOURCE_DIR . '/controllers/' . $controller . '/model.php' );
        $model = $controller . 'Model';
        if ( class_exists ( $model ) )
          return new $model( );
      }
	  return false;
	}

    /**
     * Проверка существование экшена
     * @param string $controller
     * @param string $action
     * @return boolean
     */
    public function isAction ( $controller, $action ) {
      if ( method_exists ( $controller, $action ) )
        return true;

      return false;
    }

    /**
     * Устанавливает текущий экшен
     * @param string $action
     * @return string
     */
    public function setAction ( $action ) {
      if ( !is_null ( $action ) )
        $this->action = $action;
      else
        $this->action = '';
    }

    /**
     * Загружает текущий экшен
     */
    public function getAction ( $controller ) {
      // проверяем чтобы контроллер был включен...
      if ( $this->isAction ( $controller, $this->action ) )
        return true;

      return false;
    }

    public function redirect ( $url = '/', $moved301 = false ) {
      if ( $moved301 !== false ) {
        header ( "HTTP/1.1 301 Moved Permanently" );
        header ( "Location: {$url}" );
      } else {
        header ( "Location: {$url}" );
      }
    }

  }

}

Answer 1

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

Пишу свою модель MVC

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

и мне кажется что я делаю что-то не так и хочется его добить

Давайте сначала по мелочам:

if ( !defined( 'FILE_COMPILE' ) )

убрать. Если вы собираетесь "конкатенировать PHP" - не делайте этого. Есть opcache.

if ( !class_exists ( 'Controller' ) ) {

composer и PSR-4 совместимая автозагрузка классов.

class Controller extends UDAPI {

Наследование классов (extends) - плохо (ну опять же в подавляющем большинстве случаев). Наследование типов (implements) - хорошо. Приучайте себя использовать наследование классов как крайнюю меру.

Ну и опять же, что это за UDAPI от которого вы наследуете контроллер? Не выгоднее ли передать его в конструктор контроллера как зависимость?

protected $uri;
    protected $params = array();
    protected $request = array();

сделайте отдельный класс Request и инкапсулируйте работу с ним там.

$this->uri = filter_input ( INPUT_SERVER, 'REQUEST_URI', FILTER_SANITIZE_URL );
    $this->uri = trim ( $this->uri, '/\\' );
    $this->uri = urldecode ( $this->uri );
    $this->uri = parse_url ( $this->uri, PHP_URL_PATH );

Опять же, это не задача контроллера. Контроллер пусть принимает уже готовый объект запроса.

if ( $this->uri == '' )
        $this->redirect ( $this->config->root_uri . $this->default[ 'controller' ] . '/' . $this->default[ 'action' ], true );

Почитайте про мидлвэры, про фронт-контроллеры, про Model-View-Adapter. Последняя схема куда лучше вписывается в модель HTTP сервера.

public function ready ( ) {

сделайте отдельный компонент - роутер. Кто-то (например фронт контроллер) должен просить роутер узнать какой контроллер дергать. И потом уже дергать этот контроллер. Не надо пихать все в одну хрень, тем самым вы нарушаете саму идею "разделения ответственности" пытаясь реализовать конкретную реализацию этого принципа.

$this->setController ( array_shift ( $this->pattern ) );
$this->setAction ( array_shift ( $this->pattern ) );

Пара слов о состояние. Состояние это сложность. То есть посмотрев на переменную `$this->pattern` мы понятия не имеем что там хранится поскольку с течением времени его значение меняют. Причем зачем меняют - непонятно.

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

call_user_func_array ( array ( $controller, $this->action ), $this->request );

На дворе PHP7 между прочим.

} else {
            echo '404 error';
          }

раз уж начали - делайте всю обработку ошибок через исключения. Тут вам пригодится как раз фронт контроллеры и прочее.

public function isController ( $controller ) {
    if ( is_dir ( SOURCE_DIR . '/controllers/' . $controller . '/' ) ) {

... PSR-4, классы... автозагрузка...

public function getModel ( $controller ) {

Для 2006-ого года в целом нормально, но это ж 10 лет назад.

Comments

[Вадим Сибыч]

Благодарю вас за развернутое замечания, начну юзать литературу и буду совершенствовать свой скилл, еще раз спасибо вам!

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

Вадим Кондаков: могу сказать что вот этот момент со стэйтом и наследованием - крайне важен. Я не могу гарантировать вам этого, но если вы попробуете вникнуть глубже в следующие темы, возможно процесс обучения у вас пойдет эффективнее:

- состояние. Почему глобальные переменные это плохо. Что такое побочные эффекты. Почему код завязанный на состоянии сложнее кода, в котором состояние отсутствует.
- типы данных. Классы как способ создавать новые типы данных. Интерфейсы как способ задавать абстрактные типы. implements как способ композиции типов. Как следствие, почему implements лучше extends.

Answer 2

[dev400]

прочитайте это www.phptherightway.com (русская версия getjump.me/ru-php-the-right-way/)

Comments

[Вадим Сибыч]

Спасибо, почитаю

Answer 3

[T_y_l_e_r]

mvcphp.ru

Comments

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

что это за ужас?