2
\$\begingroup\$

Okay, I am wanting to expand my armoury within PHP, so I've been researching OO PHP. With my researched knowledge I went on to create a quick script that reads a CSV file and outputs the results.

CSV Class:

class CSV {

    private $file; 

    public function __construct($filename, $mode) {
        $this->file = fopen($filename, $mode); 
        return $this->file;
    }

    public function endFile() {
        return feof($this->file); 
    }

    public function getCSV($mode) {
        return fgetcsv($this->file, $mode);
    } 

    public function close() {
        fclose($this->file); 
    }
}

The test file:

require('class.csv.php');

$csv = new CSV('postcodes.csv', 'r'); 

while(!$csv->endFile()) {

    $postcode = $csv->getCSV(1024); 
    echo $postcode[0] . "<br />"; 

}

$csv->close();

I am just wondering if there are... or what are the improvements I could make in regards to the OO approach. This is purely a script for me to put the knowledge i've learnt together. I'm not another 'following the crowd person' who creates every script in an OO approach because they 'can'.

I understand it is a short script but I am wanting to make sure I have the correct approach before moving forward.

\$\endgroup\$
2
\$\begingroup\$

While I think this is a very solid foundation, I think you could expand this to make it easier to use.

The first thing which caught my eye was the possibility to make a file handle open longer than necessary. You have already implemented the functionality, but to have the handle closed before the script is terminated is a responsibility of the developer. I would add a __destruct() magic method which checks if the file handle has been closed when the object is out of scope.

public function __destruct() {
    if(is_resource($this->handle)) { // Property name changed, see next.
        $this->close();
    }
}

The next thing is to provide a default $mode. As this class would mainly be used to read the file you could easily set a default value. As of now your code does not utilize any other functionality provided by the different file handle modes, so this should not change anything, other than making your class easier to use.

public function __construct($filename, $mode = 'r') {...

While we are at the constructor I would also rename your property $this->file into $this->handle and save the opened file name in a separate property. This is to write better error messages.

private $file;
private $handle;

public function __construct($file, $mode = 'r') {

    if(!is_string($file)) {
        throw new InvalidArgumentException('Parameter $file must be a string. Provided type: ' . gettype($file));
    }

    if(!is_file($file) || !is_readable($file)) {
        throw new RuntimeException('The provided file could not be opened for reading. File: ' . $file);
    }

    $this->file   = $file;
    $this->handle = fopen($file, $mode);
}

I would also write some error checking. The most obvious place something bad could happen is when opening the file handle. If you first make sure the file actually exists and throw a well written exception if not, you will save yourself some time when errors occur.

The next place I think could use some error checking is when you are reading a line using fgetcsv() in method getCSV(). If an error occurs it returns false, but the current state of the code is completely unaware this may have happened. A simple check to see if it returned false and then throw an exception would suffice.

public function getCSV($mode)
{
    $value = fgetcsv($this->handle, $mode);

    if($value === false) {
        throw new RuntimeException('Encountered an error while reading CSV file: ' . $this->file);
    }

    return $value;
}

Misc (naming)

I feel like the name of the class is somewhat misleading. If I only look at the like name I might also get the impression I can save values into the specified file. Consider changing the name to reflect its exact behavior. I would consider something like ReadCSV sufficient. I also think this is true for the method getCSV(). Do I read the entire file or just one line? It can be hard to determine just by looking at the name. I would rename it to readLine(). This name also skips the CSV part, as the class name states its associated with a csv file, so it should not come as a surprise.

I also have a little trouble understanding your argument $mode in the method getCSV(). If you look at the documentation for fgetcsv() it has more than one optional parameters, but none named $mode. Consider changing this to the same names, as this would make it easier to understand for other developers (or yourself in six months) who knows the method signature or looks at the documentation.

You should also consider writing documentation using the DocBlock feature. This can help reduce confusion when argument names are ambiguous and provide a short description of the behavior of the method as well as which exceptions it can throw.

Additional

You could also write methods for reading the entire file. A method that looks something like the following could be useful.

public function readAll()
{
    $array = [];

    while(($columns = fgetcsv($this->handle)) !== false) {

        $rows = count($columns);

        for($i = 0; $i <= $rows; $i++) {
            $array[] = $columns[$i];
        }

    }

    return $array;
}

Hope this helps. Please keep in mind none of this code has been tested.

Happy coding!

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for this! I did use DockBlock within the script on my local machine. I just didn't copy it across to the example - apologies. Now you have mentioned a lot of these they seem such simple mistakes but I guess that's part of the learning curve. Thanks again! \$\endgroup\$ – Jacob Hallas Jun 2 '15 at 18:57
  • \$\begingroup\$ I wouldn't think of them as mistakes, but as improvements. As I said I think this is a solid foundation to improve on :D \$\endgroup\$ – AnotherGuy Jun 2 '15 at 20:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.