php - software - ¿Este código de controlador MVC necesita ser refactorizado o no?

refactory

Question

Estoy escribiendo un administrador de importación CSV / Excel -> MySQL para una aplicación MVC (Kohana / PHP).

Tengo un controlador llamado "ImportManager" que tiene una acción llamada "index" (predeterminado) que muestra en una grilla todos los archivos .csv y .xls válidos que están en un directorio específico y listos para la importación. El usuario puede elegir los archivos que quiere importar.

Sin embargo, como los archivos .csv importan en una tabla de base de datos y los archivos .xls importan en varias tablas de bases de datos, necesitaba manejar esta abstracción . Por lo tanto, creé una clase de ayuda llamada SmartImportFile a la que envío cada archivo, ya sea .csv o .xls y luego solicito a este objeto "inteligente" que agregue las hojas de trabajo de ese archivo ( ya sean uno o varios ) a mi colección. Aquí está mi método de acción en código 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;
}

La clase SmartImportFile se ve así:

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

En una revisión del código, me dijeron que sería mejor no tener una clase de ayuda como esta, sino guardar la mayor parte del código en el método de acción del controlador. La argumentación era que debería ser capaz de ver el código en una acción de controlador y ver qué hace en lugar de hacer que llame a clases de ayuda externas fuera de sí mismo. Estoy en desacuerdo. Mi argumentación es:

• debe crear una clase auxiliar cada vez que haga que el código sea más claro , ya que en este caso, abstrae el hecho de que algunos archivos tienen una hoja de trabajo o varias hojas de trabajo y permite una futura extensión fácil, por ejemplo, si queremos importar también desde archivos sqlite o incluso directorios con archivos en ellos, esta abstracción de clase sería capaz de manejar esto muy bien.
• mover la mayor parte del código de esta clase auxiliar al controlador me obligaría a crear variables internas en el controlador que tengan sentido para esta acción en particular, pero pueden o no tener sentido para otros métodos de acción dentro del controlador.
• si estuviera programando esto en C # , haría de esta clase auxiliar una clase anidada que literalmente sería una estructura interna de datos que está dentro y solo está disponible para la clase de controlador, pero como PHP no permite clases anidadas, necesito llamar a un Clasifique "fuera" del controlador para ayudar a gestionar esta abstracción de forma que el código sea claro y legible

En función de su experiencia de programación en el patrón MVC, ¿la clase de ayudante anterior se debe refactorizar de nuevo al controlador o no?

Answer 1

Estoy de acuerdo contigo, Edward.

Su ImportController hace lo que un Controlador debe hacer. Genera la lista de archivos para que el usuario los vea y actúe, luego pasa esa lista a la Vista para que se muestre. Supongo que tiene una acción de process o similar que maneja la solicitud cuando un usuario ha seleccionado un archivo, este archivo luego se pasa al Ayudante en cuestión.

El Ayudante es un ejemplo perfecto de abstracción y está completamente justificado en su uso y existencia. No está acoplado con el controlador de todos modos y no necesita ser. El Helper podría ser utilizado fácilmente en otros escenarios donde el Controlador no está presente, por ejemplo, una tarea CRON, una API pública que los usuarios pueden llamar mediante programación sin su ImportController .

Tu derecho sobre el balón con este. ¡Adhiérelo a ellos!

Answer 2

Hay dos enfoques para los controladores: hacerlo delgado o grueso. Cuando comencé mi aventura con MVC cometí un error al crear controladores de gran tamaño; ahora prefiero hacerlo lo más delgado posible. Tu solución es buena en mi opinión.

Así es como rediseñé su código aún más:

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
}

La idea es tener dos clases responsables del procesamiento de xml y cvs que heredan de una clase base. La clase principal usa un método especial para detectar cómo deben procesarse los datos ( Patrón de estrategia ). El controlador acaba de pasar una lista de archivos a la instancia de la clase Backend_Application_SmartImport y pasa el resultado del método de proceso a la vista.

La ventaja de mi solución es que el código está más desacoplado y puede agregar fácilmente nuevos tipos de procesamiento como xml, pdf, etc.