这个 MVC 控制器代码是否需要重构？答案

【问题标题】：Does this MVC controller code need to be refactored or not?这个 MVC 控制器代码是否需要重构？
【发布时间】：2011-06-11 14:34:57
【问题描述】：

我正在为 MVC 应用程序 (Kohana/PHP) 编写 CSV/Excel-->MySQL 导入管理器。

我有一个名为“ImportManager”的控制器，它有一个名为“index”（默认）的操作，它在网格中显示特定的所有有效.csv 和.xls 文件目录并准备好导入。然后用户可以选择他想要导入的文件。

但是，由于.csv 文件导入一个数据库表，.xls 文件导入多个数据库表，我需要处理这个抽象。因此我创建了一个名为 SmartImportFile 的 helper 类 （是一个或多个）到我的收藏。这是我在 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 文件甚至 directories 中导入文件，这类抽象能够很好地处理这个问题。
将大部分代码从这个助手类移回控制器会迫使我在控制器中创建内部变量，这对这个特定的动作有意义，但可能有意义也可能没有意义控制器中的其他操作方法。
如果我在 C# 中对此进行编程，我会将这个帮助类设为 嵌套类，它实际上是一个 internal 数据结构，即在控制器类内部且仅可用于控制器类，但由于 PHP 不允许嵌套类，因此我需要在控制器“外部”调用一个类来帮助管理此抽象，以使代码清晰可读性强

根据您在 MVC 模式中的编程经验，是否应该将上述帮助程序类重构回控制器？

【问题讨论】：

控制器可以直接对视图进行操作，但不能渲染自身（这是视图的责任）。 “ImportMnager”不是控制器，而是视图。只需查看您的代码即可。
ImportManager 不会呈现自己，而只是准备数据集合（要导入的工作表），然后视图会根据需要显示这些数据集合。所以ImportManager 确实是一个控制器，它为它准备数据并决定哪个视图将显示该数据。

标签： php model-view-controller controller helpers

【解决方案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。主类使用一种特殊的方法来检测应该如何处理数据（Strategy Pattern）。控制器只是将文件列表传递给 Backend_Application_SmartImport 类的实例，并将处理方法的结果传递给视图。

我的解决方案的优点是代码更加解耦，您可以轻松、干净地添加新的处理类型，如 xml、pdf 等。

【讨论】：

是的，当然，如果我想改进它，我会这样重构它，也就是说，通过进一步分解它，而不是只有一个类处理多种类型的文件，而是有一个抽象类和将要导入的每种文件类型的类

【解决方案2】：

我同意你的观点，爱德华。

您的ImportController 完成了控制器应做的事情。它生成文件列表供用户查看和操作，然后将该列表传递给视图以供其显示。我假设您有一个 process 操作或类似的操作，当用户选择一个文件时处理请求，然后将该文件传递给相关的 Helper。

Helper 是抽象的完美例子，它的使用和存在是完全合理的。无论如何，它都没有与控制器耦合，也不需要。 Helper 可以很容易地用于其他不存在 Controller 的场景，例如 CRON 任务，一个公共 API，用户可以在没有您的 ImportController 的情况下以编程方式调用它。

你在这个球上的权利。坚持下去！

【讨论】：