2
\$\begingroup\$

I've started writing a small CSV parser class as I needed a method to group a CSV file by a given column.

class CSVParser {


    private $FileName;
    private $FileHandle;
    private $HasHeaderRow = false;
    private $RowsToIgnore = array();


    public function __construct ($FileName, $Mode = "r") {
        $this->FileName = $FileName;

        if (($this->FileHandle = fopen($this->FileName, $Mode)) === FALSE)
            return false; // that's all your getting for now.
    }


    public function hasHeaderRow ($Flag = false) {
        $this->HasHeaderRow = (bool) $Flag;
    }


    public function ignoreRows (array $Rows) {
        $this->RowsToIgnore = $Rows;
    }


    public function groupByColumn ($GroupColumn = 0, $RowLength = 1000, $Seperator = ",") {

        $CurrentRow = -1;
        $Classes = array();

        while (($Data = fgetcsv($this->FileHandle, $RowLength, $Seperator)) !== FALSE) {
            $CurrentRow++;

            // does the csv contain a header row?
            if ($this->HasHeaderRow)
                if ($CurrentRow < 1) continue;

            // are there any rows to ignore?
            if (in_array($CurrentRow, $this->RowsToIgnore))
                continue;

            // line 45
            $Classes[$Data[$GroupColumn]][] = array_filter($Data, function ($var) use ($GroupColumn, $Data) {
                return $var !== $Data[$GroupColumn];
            });
        }

        return $Classes;
    }


    public function __destruct() {
        fclose($this->FileHandle);
    }
}

// usage 
$csv = new CSVParser("mycsv.csv");
$csv->hasHeaderRow(true);

$classes = $csv->groupByColumn(0));

I'm a little unsure about the removal of the group by column from the array entries on line 45.

Can anyone see a better method of achieving this?

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

Ok, before I set of, there are 3 things to keep in mind here: Code review has to be tough, to be good, so if this hurts your feelings - sorry, but I'm trying to help.
The second thing is: Please, please, please try to adhere to the coding standards as defined by PHP-FIG (google them). Most, if not all, major players adhere to them, and you should, too. Sure, they're not official (yet), but there are no official standards... the more people adhere to a given set of unofficial rules, the more likely it is they become standard.
And finally: Please, document your code... you add some comments here and there, but not nearly as much as required... the main method of your class will bite you in the bum if you leave it be for a couple of months, and then decide to add something or debug it a little...

Now, to business. As I usually do, I'll be going through your code bit by bit, suggesting and commenting along the way

class CSVParser
{//next line, PHP-FIG style

    /**
     * @var string
     **/
    protected $fileName = null;//camelCase plz
    /**
     * @var resource <file handle>
     **/
    protected $fileHandle = null;
    /**
     * @var bool
     **/
    protected $hasHeaderRow = false;
    /**
     * @var array<mixed>
     **/
    protected $rowsToIgnore = array();

Now, I'd initialize the properties to null, simply because it's a good habit if you're ever in the situation where you find yourself switching from one language to another.
I'd also add doc-blocks per property to document what types these properties will be assigned
I'd also strongly suggest defining the properties as protected, since a class that reads files should be quite generic, and easy to extend from. If you extend, therefore, private is not always very useful...

Now, the methods:

public function __construct ($FileName, $Mode = "r")
{
    $this->FileName = $FileName;
    if (($this->FileHandle = fopen($this->FileName, $Mode)) === FALSE)
        return false;///????
}

This one is a big problem: At no point are you checking the type of any of the arguments that you're receiving. Is $FileName even a string? is it null?
Worse still: $Mode is the mode in which you want to open the file... fine, there's a limited amount of valid strings that can be passed here. For the ease of the user, and to improve readability of the code that uses your class, I'd create constants for the $Mode parameter.
Then: return false? Seriously? From a constructor? A constructor returns an instance of the given class. All other return statements are pointless, they are ignored. If this were to work, this requires the user to write code like this:

$reader = new CSVParser($file, 'r');
if ($reader === false)
{
    //handle error
}

Which is just messy. If you must, throw an exception, but better yet: allow the user to set the file that needs to be read later on. That way, the user can create the instance, and re-use it throughout his code. Here's what I'd write:

const OPEN_READ = 'r';
const OPEN_WRITE = 'w';
const OPEN_APPEND = 'a';
const OPEN_BIN_READ = 'rb';
const OPEN_BIN_WRITE = 'wb';
//and so on
public function __construct($file = null, $mode = self::OPEN_READ)
{
    if ($file)
    {
        if (is_string($file))
        {
            $this->fileName = $file;
            if (!($this->fileHandle = fopen($file, $mode))
                throw new RuntimeException('Opening file '.$file.' failed');
            return $this;//success
        }
        throw new InvalidArgumentException('File argument is not a string');
    }
    //nothing passed => just fine...
}

But this is far from perfect: you should provide support for the SplFileInfo class (users passing an instance of SplFileInfo should work, too) and perhaps allow them to pass a file handle they opened already.
What you've also failed to do is allow the user to set the hasHeaderRow in the constructor. Now, I'm no big fan of constructors requiring too many arguments, so what I'd probably write is something like:

public function __construct($mixed)
{
    if ($mixed instanceof SplFileInfo) return $this->openSplFile($mixed);
    if (is_resource($mixed)) return $this->useFileHandle($mixed);
    //is string, array, some other instance...
}

But that's what I would do, not what you must do.
Onwards:

public function hasHeaderRow ($flag = false)
{
    $this->hasHeaderRow = (bool) $flag;
    return $this;
}

public function ignoreRows (array $Rows)
{
    $this->rowsToIgnore = $Rows;
    return $this;
}

I've added the return $this; statements simply because I like chainable API's. Now the user can write:

$instance->hasHeaderRow(true)
        ->ignoreRows($someArray);

Other than that, the hasHeaderRow is fine, but the ignoreRows method is, to my eye a tad vague, and I don't like how it doesn't allow me to merge in new rows to the ignore array. Perhaps consider writing

public function ignoreRows(array $rows, $merge = false)
{
    if ($merge){
        $this->rowsToIgnore = array_merge(
            $this->rowsToIgnore,
            $rows
        );
        return $this;
    }
    $this->rowsToIgnore = $rows;
    return $this;
}

Now, I'll skip the groupByColumn method for now, because there's a lot of things I to review there, so first, let's look at this:

public function __destruct()
{
    fclose($this->fileHandle);
}

Nice, that's a sensible use of the constructor method. But what about any of the other magic methods? __sleep and __clone to name just 2? All those methods, if a file-handle is concerned should probably be implemented to close the file, or (__clone for example) should return false, or throw an exception.
I've also hinted at this in the beginning of this answer: what if I were to write some app that had to parse several files? Using your class, I'd have to create a new instance for each file. That's just adding pointless overhead. I'd propose you add a openFile method, that works similarly to the constructor (ideally accepting SplFileInfo and the like, too). This openFile method, then could look like this:

public function openFile($mixed)
{
    if ($this->fileHandle !== null && is_resource($this->fileHandle))
    {
        $this->fileName = null; //<<this is never used throughout your code!
        fclose($this->fileHandle);
        //and so on, re-initialize all properties
    }
    //add same if's and else's as in __constructor here
    return $this;
}

Now, I'll post this answer because it's gotten quite lengthy already. Meanwhile, I'll examing the groupByColumn method to actually get to grips with it... it's short, sure, but would you be able to maintain it 6 months from now? If yes: right... ;-P


Now the main function... not only is it a nightmare to read, it again fails to validate any of the params it received, and it contains bugs!
A line by line once-over:

public function groupByColumn ($groupColumn = 0, $rowLength = 1000, $seperator = ",")
{
    $currentRow = -1;//why, you don't need this
    $classes = array();//what do you mean "classes"
    while ($data = fgetcsv($this->fileHandle, $rowLength, $seperator))
    {//no need for the !== false
        $currentRow++;//
        if ($this->hasHeaderRow)//Why check this on each iteration?
            if ($currentRow < 1) continue;//ouch continue
        if (in_array($currentRow, $this->RowsToIgnore))
                continue;//ouch again
        //maintain this... good luck
        $classes[$data[$groupColumn]][] = array_filter(//should issue notice
            $data,
            function ($var) use ($groupColumn, $data)
            {//think Gloves (you're over-complicating)
                return $var !== $data[$groupColumn];
            }
        );
    }
    return $classes;
}

Now without too many words, I'll just re-write the same code to do what -I think- it is you're trying to acchieve

public function groupByColumn ($groupColumn = 0, $rowLength = 1000, $seperator = ",")
{
    $lines = array();
    if ($this->hasHeaderRow)
        fgetcsv($this->fileHandle,
                $rowLength,
                $separator);
    $count = 0;
    while ($data = fgetcsv($this->fileHandle, $rowLength, $seperator))
    {
        if (!in_array(++$count, $this->rowsToIgnore))
        {
            $idx = $data[$groupByColumn];
            unset($data[$groupByColumn]);
            if (!isset($lines[$idx])) $lines[$idx] = array();
            $lines[$idx][] = $data;
        }
    }
    return $lines;
}

Now not only is this code smaller, it's safer, it is also not producing notices, less resource hungry, and it is, IMO, a lot easier to read. It's not difficult to understand the code, but let me back my claims up:

  • safer: your array_filter was removing duplicate Values but if I were to group by a "column" that contained a number, and my data looked like this: 123,4364, foobar,123, I could be removing 2 values from the array. You only need to remove 1: the value associated with the group-by column, of which you have the offset. Using unset is faster, and safer
  • No notices: well, for each new $data[$groupByColumn] value, you need to create a new array. You never do so explicitly. $classes[$data[$groupByColumn]][]<-- the latter just assumes there is an array. If it doesn't exist, PHP will create one for you, but it will produce a notice, too, which slows you down.
  • less resources required: simply because I'm calling less functions and not using a lambda function (which, in PHP is an instance of the Closure class anyway). You create an instance on each iteration... calling a constructor each time... I use unset + the index of an array, which is a simple HashTable lookup internally.
    Sure, PHP will probably optimize this, and create a single instance of Closure but this instance will be GC'ed once this method returns. So you're still creating instances each time this method gets called.

Well, that's my review, hope you find a thing or 2 useful in this verbal diarrhea. I've written this all in one go, off the top of my head, and I didn't check any code I posted here, so there might be some syntax errors (and grammar errors) in here, but hey...

\$\endgroup\$
  • \$\begingroup\$ Firstly, thanks for taking the time to answer. Maybe I should have pointed out that this was just a helper class to tidy up a problem being solved procedurally, hence the poor standard. Everything you have pointed out are things I would slate colleagues for as well but I will definitely bear everything in mind when posting code sample to Code review. Secondly, I am unsure what the purpose of the statement after the if ($this->hasHeaderRow) condition. Your method would make sense if hadHeaderRow was pushed into rowsToIgnore as the purpose of the property is to Ignore the first row. \$\endgroup\$ – Ian Brindley Jan 17 '14 at 14:46
  • \$\begingroup\$ @IanBrindley: Well, before the actual loop starts, and $this->hasHeaderRow is true, I already call fgetcsv once, so the first row is ignored no matter what. There's no need to push anything to $this->rowsToIgnore, simply because the header was read before the actual data-processing began: the header never makes it to the loop. \$\endgroup\$ – Elias Van Ootegem Jan 17 '14 at 14:54
  • \$\begingroup\$ I'm with you now. Personally I would refactor this and completely remove that statement by pushing $this->hasHeaderRow (0) to $this->rowsToIgnore. I don't feel that it will impact readability if the class followed commenting conventions. Also, in the working copy of this class I had made a note of simply using unset on $data[$groupByColumn]. Would I be correct in saying that array_values would need to be called on $lines[$idx][] = $data; to reset the array keys thus removing any issues in using the returned $lines? \$\endgroup\$ – Ian Brindley Jan 17 '14 at 15:04
  • \$\begingroup\$ @IanBrindley: calling array_values would reset the keys, yes... would that really be an issue, though? If so, though I could be wrong here, I think that -generally- sort() is a faster way to reset the keys, but if the order matters, too, also check explode('!@$#$', implode('!@$#$', $data)); or some other wacky delimiter... \$\endgroup\$ – Elias Van Ootegem Jan 17 '14 at 15:33

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.