Этот код контроллера MVC должен быть реорганизован или нет?

Question

Я пишу менеджер импорта CSV/Excel->MySQL для приложения MVC (Kohana/PHP).

У меня есть контроллер с именем "ImportManager", который имеет действие с именем "индекс" (по умолчанию), который отображает в сетке все действительные .csv а также .xls файлы, которые находятся в определенном каталоге и готовы к импорту. Затем пользователь может выбрать файлы, которые он хочет импортировать.

Тем не менее, так как .csv импорт файлов в одну таблицу базы данных и .xls Импорт файлов в несколько таблиц базы данных, мне нужно было обработать эту абстракцию. Поэтому я создал вспомогательный класс с именем SmartImportFile на который я отправляю каждый файл, будь то .csv или же .xls и затем я получаю, затем спрашиваю этот "умный" объект, чтобы добавить листы из этого файла (будь то один или много) в мою коллекцию. Вот мой метод действия в коде PHP:

public function action_index()
{
    $view = new View('backend/application/importmanager');

    $smart_worksheets = array();
    $raw_files = glob('/data/import/*.*');
    if (count($raw_files) > 0)
    {
        foreach ($raw_files as $raw_file)
        {
            $smart_import_file = new Backend_Application_Smartimportfile($raw_file);
            $smart_worksheets = $smart_import_file->add_smart_worksheets_to($smart_worksheets); 
        }
    }
    $view->set('smart_worksheets', $smart_worksheets);

    $this->request->response = $view;
}

Класс SmartImportFile выглядит следующим образом:

class Backend_Application_Smartimportfile
{
    protected $file_name;
    protected $file_extension;
    protected $file_size;
    protected $when_file_copied;
    protected $file_name_without_extension;
    protected $path_info;
    protected $current_smart_worksheet = array();

    protected $smart_worksheets = array();

    public function __construct($file_name)
    {
        $this->file_name = $file_name;
        $this->file_name_without_extension = current(explode('.', basename($this->file_name)));

        $this->path_info = pathinfo($this->file_name);
        $this->when_file_copied = date('Y-m-d H:i:s', filectime($this->file_name));
        $this->file_extension = strtolower($this->path_info['extension']);
        $this->file_extension = strtolower(pathinfo($this->file_name, PATHINFO_EXTENSION));
        if(in_array($this->file_extension, array('csv','xls','xlsx')))
        {
            $this->current_smart_worksheet = array();
            $this->process_file();
        }
    }

    private function process_file()
    {
        $this->file_size = filesize($this->file_name);
        if(in_array($this->file_extension, array('xls','xlsx')))
        {
            if($this->file_size < 4000000)
            {
                $this->process_all_worksheets_of_excel_file();
            }
        }
        else if($this->file_extension == 'csv')
        {
            $this->process_csv_file();
        }

    }

    private function process_all_worksheets_of_excel_file()
    {
        $worksheet_names = Import_Driver_Excel::get_worksheet_names_as_array($this->file_name);
        if (count($worksheet_names) > 0)
        {
            foreach ($worksheet_names as $worksheet_name)
            {
                $this->current_smart_worksheet['name'] = basename($this->file_name).' ('.$worksheet_name.')';
                $this->current_smart_worksheet['kind'] = strtoupper($this->file_extension);
                $this->current_smart_worksheet['file_size'] = $this->file_size;
                $this->current_smart_worksheet['when_file_copied'] = $this->when_file_copied;
                $this->current_smart_worksheet['table_name'] = $this->file_name_without_extension.'__'.$worksheet_name;
                $this->assign_database_table_fields();
                $this->smart_worksheets[] = $this->current_smart_worksheet;
            }
        }
    }

    private function process_csv_file()
    {
        $this->current_smart_worksheet['name'] = basename($this->file_name);
        $this->current_smart_worksheet['kind'] = strtoupper($this->file_extension);
        $this->current_smart_worksheet['file_size'] = $this->file_size;
        $this->current_smart_worksheet['when_file_copied'] = $this->when_file_copied;

        $this->current_smart_worksheet['table_name'] = $this->file_name_without_extension;
        $this->assign_database_table_fields();


        $this->smart_worksheets[] = $this->current_smart_worksheet;
    }

    private function assign_database_table_fields()
    {
        $db = Database::instance('import_excel');
        $sql = "SHOW TABLE STATUS WHERE name = '".$this->current_smart_worksheet['table_name']."'";
        $result = $db->query(Database::SELECT, $sql, FALSE)->as_array();
        if(count($result))
        {
            $when_table_created = $result[0]['Create_time'];
            $when_file_copied_as_date = strtotime($this->when_file_copied);
            $when_table_created_as_date = strtotime($when_table_created);
            if($when_file_copied_as_date > $when_table_created_as_date)
            {
                $this->current_smart_worksheet['status'] = 'backend.application.import.status.needtoreimport';
            }
            else
            {
                $this->current_smart_worksheet['status'] = 'backend.application.import.status.isuptodate';
            }
            $this->current_smart_worksheet['when_table_created'] = $when_table_created;
        }
        else
        {
            $this->current_smart_worksheet['when_table_created'] = 'backend.application.import.status.tabledoesnotexist';
            $this->current_smart_worksheet['status'] = 'backend.application.import.status.needtoimport';
        }
    }

    public function add_smart_worksheets_to(Array $smart_worksheets = array())
    {
        return array_merge($smart_worksheets, $this->get_smart_worksheets());
    }

    public function get_smart_worksheets()
    {
        if ( ! is_array($this->smart_worksheets))
        {
            return array();
        }

        return $this->smart_worksheets;
    }

}

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

• Вы должны создавать вспомогательный класс в любое время, когда он делает код более понятным, так как в этом случае он абстрагируется от того факта, что некоторые файлы содержат одну рабочую таблицу или несколько рабочих листов, и позволяет легко расширять ее в будущем, если, скажем, мы хотим также импортировать из файлов sqlite или даже каталогов с файлами в них, эта абстракция класса сможет справиться с этим красиво.
• перемещение основной части кода из этого вспомогательного класса обратно в контроллер заставило бы меня создать в контроллере внутренние переменные, которые имеют смысл для этого конкретного действия, но могут иметь или не иметь смысла для других методов действия в контроллере.
• если бы я программировал это на C#, я бы сделал этот вспомогательный класс вложенным классом, который был бы буквально внутренней структурой данных, которая находится внутри и доступна только для класса контроллера, но поскольку PHP не допускает вложенные классы, мне нужно вызвать класс "за пределами" контроллера, чтобы помочь управлять этой абстракцией таким образом, чтобы сделать код понятным и читаемым

Основываясь на вашем опыте программирования в шаблоне MVC, должен ли вышеупомянутый вспомогательный класс быть реорганизован обратно в контроллер или нет?

Answer 1

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

Вот как я бы переработал ваш код еще дальше:

class Backend_Application_SmartImport {

    public function __construct( $raw_files ) {
    }

    public function process() {     
        foreach ($raw_files as $raw_file) {
            // (...)
            $oSmartImportFileInstance = $this->getSmartImportFileInstance( $smart_import_file_extension );
        }
    }   

    protected function getSmartImportFileInstance( $smart_import_file_extension ) {
        switch ( $smart_import_file_extension ) {
            case 'xml':
                return new Backend_Application_SmartImportFileXml();
            // (...)
        }
    }
}

abstract class Backend_Application_SmartImportFile {
    // common methods for importing from xml or cvs
    abstract function process();
}

class Backend_Application_SmartImportFileCVS extends Backend_Application_SmartImportFile {
    // methods specified for cvs importing
}

class Backend_Application_SmartImportFileXls extends Backend_Application_SmartImportFile {
    // methods specified for xls importing
}

Идея состоит в том, чтобы два класса отвечали за обработку xml и cvs, наследуемых от базового класса. Основной класс использует специальный метод для определения способа обработки данных ( шаблон стратегии). Контроллер только что передал список файлов в экземпляр класса Backend_Application_SmartImport и передает результат метода процесса в представление.

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

Answer 2

Я согласен с тобой, Эдвард.

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

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

Ваше право на мяч с этим. Приклей их!