I am reading the book Clean Code by Rob Martin and am trying to write the cleanest possible code. I wrote this script as part of my project.  Could you please criticise it and let me know how it could be improved?/** * File Downloading, Unzipping and extracting Content - PHP *  * Usage: * * $fileImporter = new FileImporter($fileReference); * $nextRowAsArray = $fileImporter->readFileRowAsArray(); *  * where $fileReference is File Name without Extension * $nextRowAsArray is associative Array representing File Entry *  * If $fileReference.zip is not found, it is downloaded using FILE_DOWNLOAD_PATH * The archiv $fileReference.zip is automatically extracted whenever needed, * and first archiv Entry $file.txt is taken * * The File $file.txt is expected of the form: *  * field_name1|field_name2 * entry1_field1|entry1_field2 * entry2_field1|entry2_field2 * ... * entryN_field1|entryN_field2 * * Here FILE_ENTRY_SEPARATOR is '|' *  * Then the returned arrays $nextRowAsArray are subsequently of the form: *  * array( 'field_name1' => 'entry1_field1', 'field_name2' => 'entry1_field2', ) * array( 'field_name1' => 'entry2_field1', 'field_name2' => 'entry2_field2', ) * ... * array( 'field_name1' => 'entryN_field1', 'field_name2' => 'entryN_field2', ) */class FileImporter {   private $_download_path = FILE_DOWNLOAD_PATH;   private $_separator = FILE_ENTRY_SEPARATOR;   private $_fileReference;   private $_fileHandleCache = null;   private $_fileHeadRow = array();   public function __construct ($fileReference) {      $this->_fileReference = $fileReference;   }   public function readFileRowAsArray () {      $fileHandle = $this->_getCachedOrNewFileHandle();      $fileEntryArray = $this->_getNextRowAsArray($fileHandle);      if ( $this->_fileHeadRow ) {         $keys = $this->_fileHeadRow;         $values = $fileEntryArray;      } else {         $this->_fileHeadRow = $fileEntryArray;         $keys = $fileEntryArray;         $values = $this->_getNextRowAsArray($fileHandle);      }      return array_combine($keys, $values);   }   private function _getCachedOrNewFileHandle () {      return $this->_fileHandleCache ? $this->_getCachedFileHandle() :  $this->_getNewFileHandle();   }   private function _getNextRowAsArray ($fileHandle) {      $fileNextLine = fgets($fileHandle);      if ( feof($fileHandle) ) return array();            $fileNextLineTrimmed = trim($fileNextLine);      return explode($this->_separator, $fileNextLineTrimmed);   }   private function _getCachedFileHandle () {      return $this->_fileHandleCache;   }   private function _getNewFileHandle () {      $zipFileName = $this->_getZipFileName();      if (!file_exists($zipFileName)) $this->_downloadFile($zipFileName);              return $this->_unzipAndGetFileHandle($zipFileName);         }   private function _getZipFileName () {//      return {$this->_fileReference . '.zip';      return {$this->_fileReference}.zip;   }   private function _downloadFile ($fileName) {      $sourceStream = fopen($this->_downloadPath . $fileName, 'r')  or die(Could not download $fileName);      $destinationStream = fopen($fileName, 'w');      echo \n Downloading $fileName\n;      stream_copy_to_stream($sourceStream, $destinationStream);   }   private function _unzipAndGetFileHandle ($zipFileName) {      $firstZipEntryName = $this->_getFirstZipEntryName($zipFileName);      if ( !file_exists($firstZipEntryName) ) $this->_ZipExtract($zipFileName);      $fileHandle = fopen($firstZipEntryName, rt) or die( Can't Read File $firstZipEntryName);      $this->_fileHandleCache = $fileHandle;      return $fileHandle;   }   private function _getFirstZipEntryName ($zipFileName) {      if ( !file_exists($zipFileName) ) {         throw new Exception(Attempting to read non-existing Zip File $zipFileName, exitting ....);      }      $zipArchiv = zip_open($zipFileName);      $firstZipEntry = zip_read($zipArchiv);      $firstZipEntryName = zip_entry_name($firstZipEntry);      zip_close($zipArchiv);      return $firstZipEntryName;   }   private function _zipExtract ($zipFileName) {      echo \n Begin extraction at , date('r')., file $zipFileName;      $zipArchiv = new ZipArchive;      $zipArchiv->open($zipFileName);            $zipArchiv->extractTo(.);      $zipArchiv->close();           }}Here are my tests, which have all passed:/** * Testing FileImporter */// case 0: both Zip and Text files exist - creatingfunction createTextAndZipFiles ($fileRef, $textData) {   $textFile = $fileRef . '.txt';   $zipFile = $fileRef . '.zip';   // creating text file   file_put_contents($textFile, $textData);   // creating zip file   $zip = new ZipArchive;   $zip->open($zipFile, ZipArchive::CREATE);   $zip->addFile($textFile);   $zip->close();}$testFile = 'file-test';$testText = <<<EOTday|year10|201131|1843EOT;createTextAndZipFiles($testFile, $testText);$importer_test = new FileImporter($testFile);$rowArray = $importer_test->readFileRowAsArray();$rowArray = $importer_test->readFileRowAsArray();assert( $rowArray['year'] === '1843' );// empty row$testText = <<<EOTday|year10|201131|1843EOT;createTextAndZipFiles($testFile, $testText);$importer_test = new FileImporter($testFile);$rowArray = $importer_test->readFileRowAsArray();assert( $rowArray['day'] === '10' );$rowArray = $importer_test->readFileRowAsArray();assert( $rowArray['year'] === '1843' );$rowArray = $importer_test->readFileRowAsArray();assert( $rowArray === array() );// too long row$testText = <<<EOTday|year10|2011|ha31|1843EOT;createTextAndZipFiles($testFile, $testText);$importer_test = new FileImporter($testFile);$rowArray = $importer_test->readFileRowAsArray();assert( $rowArray === array() );$rowArray = $importer_test->readFileRowAsArray();assert( $rowArray['year'] === '1843' );$rowArray = $importer_test->readFileRowAsArray();assert( $rowArray === array() );// too short row$testText = <<<EOTday|year201131|1843EOT;createTextAndZipFiles($testFile, $testText);$importer_test = new FileImporter($testFile);$rowArray = $importer_test->readFileRowAsArray();assert( $rowArray === array() );$rowArray = $importer_test->readFileRowAsArray();assert( $rowArray['year'] === '1843' );$rowArray = $importer_test->readFileRowAsArray();assert( $rowArray === array() );// case 1: Zip exists but Text doesn'tcreateTextAndZipFiles($testFile, $testText);// delete Textunlink($testFile . '.txt');$importer_test = new FileImporter($testFile);$rowArray = $importer_test->readFileRowAsArray();assert( $rowArray === array() );$rowArray = $importer_test->readFileRowAsArray();assert( $rowArray['year'] === '1843' );$rowArray = $importer_test->readFileRowAsArray();assert( $rowArray === array() );// case 2: Text exists but Zip doesn't// faking download by using local subdirectory// creating subdirectory 'test'$subdirName = 'test';// make sure directory exists and is not a fileif ( file_exists($subdirName) ) {   if ( !is_dir($subdirName) ) {      unlink($subdirName);      mkdir($subdirName);   }} else {   mkdir($subdirName);}createTextAndZipFiles($testFile, $testText);// now move zipfile into the subdirectory$zipFileName = $testFile . '.zip';rename($zipFileName, $subdirName . '/' . $zipFileName);$importer_test = new FileImporter($testFile, $subdirName . '/');$rowArray = $importer_test->readFileRowAsArray();assert( $rowArray === array() );$rowArray = $importer_test->readFileRowAsArray();assert( $rowArray['year'] === '1843' );
File-downloading, unzipping and extracting content with a PHP script and tests
php;object oriented;classes
I haven't read the book you mention, although it looks good so I might buy it and have a read.A few things for meThere are echo statements in the code, I am assuming that is for debugging and you are going to remove them.In a couple of places you use die(),  and then elsewhere you throw an exception. Personally I would throw exceptions and let it bubble up to somewhere that can handle them properly. Your class is called fileImporter, but it also has a download aspect to it as well. I am wondering if fileDownloader should be separated out.You have a private getter function   private function _getCachedFileHandle () {      return $this->_fileHandleCache;   }Which I find unusual, I normally only use publicly accessible getters.// It is just as easy to use this internally $f= $this->_fileHandleCache;// as it is to use, so why make it more complex$f = $this->_getCachedFileHandle();Code reuse, when I design stuff I only create a separate method if I can reuse the same code, or due to it's complexity/size of the function it is best to break it into smaller parts to make it more readable.// this bit of code is only called once and is simple, so it could be included inlinepublic function readFileRowAsArray () {    $fileHandle = $this->_getCachedOrNewFileHandle();// like thispublic function readFileRowAsArray () {    $fileHandle = $this->_fileHandleCache ? $this->_getCachedFileHandle() :  $this->_getNewFileHandle();There also appears to be no way to overide these private variables which are set to constant values.class FileImporter {    private $_download_path = FILE_DOWNLOAD_PATH;    private $_separator = FILE_ENTRY_SEPARATOR;If that is the case, then why re-declare then, why not use FILE_DOWNLOAD_PATH and FILE_ENTRY_SEPARATOR directly in your codeIf you do want to allow them to be overridden, then you could extend your constructorpublic function __construct ($fileReference, $download_path = FILE_DOWNLOAD_PATH, $separator = FILE_ENTRY_SEPARATOR) {    $this->_fileReference = $fileReference;    $this->_download_path = $download_path ;    $this->_separator = $separator ;}Hope that gives you some ideas to think about