Does this MVC code need refactoring or not?

Question

I am writing a CSV / Excel → MySQL import manager for an MVC application (Kohana / PHP).

I have a controller named "ImportManager" that has an action called "index" (the default) that displays in the grid all valid .csv and .xls files that are in a specific directory and ready for import. Then the user can select the files that he wants to import.

However, since the .csv files are imported into the one database table and the .xls files are imported into the multiple tables, I needed to handle this abstraction . Therefore, I created a helper class called SmartImportFile , to which I send each file: .csv or .xls , and then I get a request to this smart object to add sheets from this file ( be it one or more ) to my collection . Here is my method of action in PHP code:

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

The SmartImportFile class is as follows:

 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('Ymd 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; } } 

In the code review, I was told that it is better not to have a helper class like this, but to save the bulk of the code in the controller’s action method itself. The argument was that you should be able to look at the code in the action of the controller and see what it does, instead of calling external helper classes outside of it. I do not agree. My argument is this:

• you must create a helper class at any time when it makes the clearer code, since in this case it abstracts the fact that some files have one worksheet or many , and allows us to simplify the future extension if, say, we also want to import from sqlite files or even directories with files in them, this abstraction class will be able to handle this.
• moving the main part of the code from this auxiliary class back to the controller will force me to create internal variables in the controller that make sense for this particular action, but other methods of action in the controller may or may not make sense.
• If I programmed this in C # , I would have made this helper class a nested class , which would literally be an internal structure that is inside and accessible only to the controller class, but since PHP does not allow nested classes, I need to call the class "outside" controller to help manage this abstraction so that the code is clean and readable

Based on your programming experience in the MVC pattern, should the helper class be reinstalled back into the controller or not?

Answer 1

There are two approaches to controllers: make it thin or thick. When I started my adventure with MVC, I made a mistake in creating thick controllers - now I prefer to make it as thin as possible. Your decision is good, in my opinion.

Here's how I reviewed your code even more:

 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 } 

The idea is to have two classes responsible for handling xml and cvs inheriting from the base class. The main class uses a special method to determine how data should be processed ( strategy template ). The controller simply passed the list of files to an instance of the Backend_Application_SmartImport class and passes the result of the process method to the view.

The advantage of my solution is that the code is more decoupled, and you can easily and simply add new types of processing, such as xml, pdf, etc.

Answer 2

I agree with you, Edward.

Your ImportController does what the controller should do. It generates a list of files for viewing and user action, then it passes this list to a view to display it. I assume that you have a process action or the like that processes the request, when the user selects a file, this file is then passed to the appropriate helper.

Helper is a great example of abstraction and is fully justified in its use and existence. In any case, it is not connected to the controller and is not needed. The helper can be easily used in other scenarios where the Controller is missing, for example, the CRON task, an open API that users can call programmatically without your ImportController .

Your right to the ball with this. Attach it to em!