CODEWITHSUNDEEP
phpfunction
JorgeeFG
JorgeeFG

Reputation: 5961

Should a function be created only if I use it more than once?

Sometimes I have very large functions, that I feel are very difficult to "compress" or separate into smaller functions, because the functions wouldn't be used in any other part of the script.

So, I'd like some advice on it: should I create functions that wouldn't be used in other parts of the script, or should I create them only if they will be used more than once?

Thanks!

Summary:

Currently my function is:

protected function processScanned()
  {
    try
    {
      // EJECUTAR BASH DE NAHUEL
      //
      //
      $PdfCPList = $this->model->getDirFilenames( $this->model->dirCartasPorte, 'pdf' );
      $PdfTBList = $this->model->getDirFilenames( $this->model->dirTicketsBalanza, 'pdf' );
      $PdfCompList = $this->model->getDirFilenames( $this->model->dirCompletos, 'pdf' );
      $PdfUnreconList = $this->model->getDirFilenames( $this->model->dirSinReconocer,'pdf' );
      // Adjuntar Novedades
      $newsToProcess = $this->model->getDirFilenames( $this->model->dirNovedades, 'txt', true);
      $this->appendNewsFiles($newsToProcess);
      $report = array();
      $report['info'] = array(
        'Documentos procesados correctamente'=>0,
        'Fecha de última actualización de datos'=>date('d/m/Y H:i:s',(int)file_get_contents($this->model->uriTxtInfo)),
      );
      if($file = fopen( $this->model->uriTxtProcesar, 'r' ) )
      {
$i = 0;
        $errors_file = fopen($this->model->uriTxtErrores,'w');
        while( $line = fgets( $file ) )
        {
          if( ! preg_match( '/^\s/', $line ) )
            continue;

          $lineData = array(
            'id'=> substr($line,3,9),
            'prefix'=>'1234-' . $i,
            'suffix'=>'1234-' . $i,
            'partner'=>'FAZON TIMBUES OMHSA',
            'date'=>time() - 222,
          );
$i++;
          $keywordsToPublish = array(
            'Nº de Operacion'=>$lineData['id'],
            'Prefijo'=>$lineData['prefix'],
            'Sufijo'=>$lineData['suffix'],
            'Socio'=>$lineData['partner'],
            'Fecha'=>date('Y/d/m',$lineData['date']),
          );

          if( $this->model->findInDocusearch( $lineData['id'] ) )
          {
            continue;
          }

          if( array_key_exists( $lineData['id'], $PdfCompList ) )
          {
            $lineData['docName'] = 'Carta de Porte - Ticket de Balanza';
            $lineData['docId'] = 'CP-TB';
            $lineData['path'] = $this->model->dirCompletos . '/' . $lineData['id'] . '.pdf';
            if( $id = $this->model->publishInDocusearch( $lineData, $keywordsToPublish ) ) {
              $report['info']['Documentos procesados correctamente']++;
              link( $this->model->dirDocusearchRepo . '/' . $id . '.pdf', 
                $this->model->dirBackupCliente . '/' . $lineData['partner'] . '_' . date('Ymd',$lineData['date']) . '_' . $lineData['id'] . '.pdf'
              );
            }
            unset( $PdfCompList[ $lineData['id'] ] );
          }
          else
          {
            fwrite($errors_file, $line); // Guarda la fila leida en el archivo de errores.
            // Valores por defecto
            $report[ 'errors' ][ $lineData['id'] ]['date'] = $lineData['date'];
            $report[ 'errors' ][ $lineData['id'] ]['id'] = $lineData['id'];
            $report[ 'errors' ][ $lineData['id'] ]['type'] = 'nn';
            $report[ 'errors' ][ $lineData['id'] ]['actions'] = array();
            // Valores por defecto

            if( array_key_exists( $lineData['id'], $PdfCPList ) )
            {
              $report[ 'errors' ][ $lineData['id'] ]['reportMsg'] = 'Falta Ticket de Balanza.';
              $report[ 'errors' ][ $lineData['id'] ]['type'] = 'cp';
              unset( $PdfCPList[ $lineData['id'] ] );
            }
            elseif( array_key_exists( $lineData['id'], $PdfTBList ) )
            {
              $report[ 'errors' ][ $lineData['id'] ]['reportMsg'] = 'Falta Carta de Porte.';
              $report[ 'errors' ][ $lineData['id'] ]['type'] = 'tb';
              unset( $PdfTBList[ $lineData['id'] ] );
            }
            else
            {
              $report[ 'errors' ][ $lineData['id'] ]['reportMsg'] = 'Ningún archivo digitalizado.';
            }
          }
        }

        fclose( $file );
        fclose( $errors_file );
        $this->fileRename( $this->model->uriTxtErrores, $this->model->uriTxtProcesar);
        foreach( $PdfCompList as $key=>$value )
        {
          $report[ 'errors' ][ $key ] = array( 
            'reportMsg'=>'Falta en archivo de datos del sistema externo.',
            'date'=>$value['date'],
            'id'=>$key,
            'type'=>'co',
            'actions'=>array('get','rename','delete'),
          );
        }
        foreach( $PdfCPList as $key=>$value )
        {
          $report[ 'errors' ][ $key ] = array(
            'reportMsg'=>'Carta de Porte no utilizada.',
            'date'=>$value['date'],
            'id'=>$key,
            'type'=>'cp',
            'actions'=>array('get','rename','delete'),
          );
        }
        foreach( $PdfTBList as $key=>$value )
        {
          $report[ 'errors' ][ $key ] = array(
            'reportMsg'=>'Ticket de Balanza no utilizado.',
            'date'=>$value['date'],
            'id'=>$key,
            'type'=>'tb',
            'actions'=>array('get','rename','delete'),
          );
        }
        foreach( $PdfUnreconList as $key=>$value )
        {
          $report[ 'errors' ][ $key ] = array(
            'reportMsg'=>'Documento no reconocido.',
            'date'=>$value['date'],
            'id'=>$key,
            'type'=>'un',
            'actions'=>array('get','rename','delete'),
          );
        }
        return $report;
      }
      else
      {
        throw new Exception('No se pudo abrir el archivo TXT');
      }
    }
    catch( Exception $e )
    {
      $this->mensaje = $e->getMessage();
      header('HTTP/1.1 500 ' . $this->mensaje);
    }
 }

Upvotes: 10

Views: 4304

Answers (3)

Samuel Danielson
Samuel Danielson

Reputation: 5471

Naming functions is hard. So whenever you see a block of code with an obvious name then you should give it that name. Separating concerns is hard. So whenever you see two obviously separate concerns you should keep them clearly separate. Reordering operations is hard. So when you see two operations that can obviously be reordered then you should make them easy to reorder. Of course it makes sense to write functions that may only be used once.

Upvotes: 1

Yotam Omer
Yotam Omer

Reputation: 15366

This is totally up to you.

However,
Separating code blocks into different functions can make the code more readable (when it's not done too excessively). Functions are not only meant for repeated use of code, they're also intended to make the code more orginized and easier to understand. You might get lost if you try to read through a long function that does a lot of tasks in parallel however if you take this function and break some parts of it into smaller functions with proper naming the function will be much shorter and clearer for you to maintain in the future or for the next programmer working on your project to understand what you've done.

Also, a good practice will be to create objects that will deal with certain more-specific tasks. This will allow (among many other benefits) to update the code by extending the classes without having to harm the original functionality.

As per your edit, a good way to determine whether or not you should split you function to pieces is found in the "function summary" you've written. When you have more than 1-2 tasks it will be a good idea to break into separate functions. I recommend writing a function for each of the following:

  • Fill arrays with info of files in directories
  • Processes TXT line by line, looks if the ID in TXT matches "Completed" files array
  • Publish array in an external product
  • Check in the other arrays to make a report of what is missing.
  • Saves the errors found in an array, then saves the array to an errors.txt
  • Ofcourse the function that wraps everything together and when done, returns the report.

Upvotes: 16

Jimbo
Jimbo

Reputation: 26564

In SOLID, you should take a look at the Single Responsibility Principle.

In object-oriented programming, the single responsibility principle states that every class should have a single responsibility, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility.

You can also apply this to individual functions - each one should be readable and perform the main task for which is was created.

You also seem like you're talking about anonymous functions - one off throwaways.

From the PHP manual:

$greet = function($name)
{
    printf("Hello %s\r\n", $name);
};

You can see how $greet has a function return it's value.

If you're using this function multiple times in your code however, make it a real function that you can call multiple times as you wish.

Upvotes: 2

Related Questions