1
\$\begingroup\$

I've spent some time looking at various posts on Stack Overflow regarding PHP CSV file manipulation and have devised a script that combines data from two CSV files to create two new files.

The aim of the task was to combine voucher numbers from one file to the customer data in another, along with the current date. I achieved this by extracting all the necessary data and writing it to a new file.

As some additional security/record keeping I've also combined the voucher data with the customer ID and current date to another file, for reference if needed.

I'm relatively new to PHP so your feedback would be greatly appreciated so that I might improve my code/logic:

// Open files for reading
$customers = fopen("customerTest1.csv", "r");
$vouchers = fopen("vouchersExtra.csv", "r+");

// Create new files to write data
$customerOutput = fopen("customerUpdated.csv", "w");
$voucherOutput = fopen("vouchersUpdated.csv", "w");

// Get columns from customer data and voucher data
$customerColumns = fgetcsv($customers, 1000);
$voucherColumns = fgetcsv($vouchers, 1000);

// Add extra columns for the output files
array_push($customerColumns, "voucherNumber", "dateApplied");
array_push($voucherColumns, "id", "dateUsed");

// Write the column headers to both customer and voucher output files
if (!$voucherOutput && !$customerOutput) {
    die("$voucherOutput and $customerOutput BROKE");
} else {
    fputcsv($customerOutput, $customerColumns);
    fputcsv($voucherOutput, $voucherColumns);
}

// Open the input
if (!$customers) {
    die("$customers BROKE");
} else {

    while (!feof($customers)) {

        // Extract current customer row of data
        $currentCustomer = fgetcsv($customers, 1000);

        // Only run the code if a current customer row exists
        if ($currentCustomer != false) {

            // Extract current voucher row of data
            $currentVoucher = fgetcsv($vouchers, 1000);

            // Get current customer's ID
            $customerId = $currentCustomer[0];

            // Merge customer and voucher data and push a date string to the end of the array
            $newCustomerRow = array_merge($currentCustomer, $currentVoucher);
            array_push($newCustomerRow, date("l jS \of F Y h:i:s A"));

            // Merge voucher data, customer ID and push a date string to the end of the array
            $newVoucherRow = [];
            array_push($newVoucherRow, $currentVoucher[0], $customerId, date("l jS \of F Y h:i:s A"));

            // Write new customer data to output file
            fputcsv($customerOutput, $newCustomerRow);

            // Write new voucher data to output file
            fputcsv($voucherOutput, $newVoucherRow);

        }

    }

}
\$\endgroup\$
1
  • \$\begingroup\$ It would be nice if you could include a small sample of each input file, along with the corresponding output. \$\endgroup\$ Jul 27, 2016 at 14:31

2 Answers 2

1
\$\begingroup\$

There are a few things you should take into account.

  1. You never close your files. At the end, use fclose;
  2. Use try {...} catch(){} finally {} and close the file(s) in the finally
  3. Use functions to enhance readability
  4. Do not use die or exit, this kills the script and is bad practise. See this blog post, very valuable
  5. It might be wise to put "magic strings" in a constants file and including this with require - for example your datetime formats, csv columns and so forth. This makes sure that you do not have to edit the script when any of these would change but rather only change the constants.
  6. Comments rot very quickly, any change would leave you with "you have to update the comments too". Don't use comments, in stead use functions that have self explanatory names. i.e. pushCustomerDateToCsvRow(param1, param2,... )
\$\endgroup\$
4
  • \$\begingroup\$ I agree with almost everything, but the comment part. Agreed they rot (but only if you make a significant change - so a moot point, since changing comments whilst doing a significant change it's a drawback, in my eyes anyway) \$\endgroup\$
    – ʰᵈˑ
    Jul 27, 2016 at 15:25
  • \$\begingroup\$ Commenting should only be used exceptionally in my opinion. Say that you have a sort of IE hack, you can put a comment on this line going //IE X+ hack; But if you are placing comments on functions, you basically did not do a good job writing said function and should rethink. most of the time the name of any function and the way it is written should allow you to read through it like a book having no need for any comment at all. People just tend to shorten names or lengthen function bodies which is not recommended in the first place. \$\endgroup\$
    – Tikkes
    Jul 28, 2016 at 7:31
  • \$\begingroup\$ Sure, that's true, I get you. Though in a corporate environment, mostly (in my experience anyways) it's needed for documentation to be generated (by something like phpdoc) - so comments are needed to explain the function, parameters, and return values. \$\endgroup\$
    – ʰᵈˑ
    Jul 28, 2016 at 7:36
  • \$\begingroup\$ That is true in some environments but not keeping those into account I believe comments should be handled with care and used only if there is no better way to explain something which is rare. But indeed, environments can handle different standards. I'm one of the happy ones who can decide these standards on projects I do, hence no comments will be found unless there is no other way. \$\endgroup\$
    – Tikkes
    Jul 28, 2016 at 8:12
0
\$\begingroup\$

Note: I haven't tested this code. It might have some bugs or typos.

Using an object oriented API will make your code cleaner:

class CSVFileObject extends \SPLFileObject implements IteratorAggregate {

    /**
     * @var string
     */
    private $delimiter;

    /**
     * @var string
     */
    private $enclosure;

    /**
     * @param string $file the file name
     * @param string $delimiter CSV column Delimiter
     * @param string $enclosure special chars enclosure
     */

    public function __construct( $file , $openMode = 'r' , $delimiter = ',' , $enclosure = '"' ){
        parent::__construct( $file , $openMode );

        $this->delimiter = $delimiter;
        $this->enclosure = $enclosure;
    }

    /**
     * @return CSVIterator
     */

    public function getIterator(){
        return new CSVIterator( $this );
    }

    /**
     * @return array
     */

    public function getCSVLine(){
        return $this->fgetcsv( $this->getLine() , $this->delimiter , $this->enclosure );
    }

    /**
     * @return integer
     */
    public function writeCSVLine(array $data){
        return $this->fputcsv( $data , $this->delimiter , $this->enclosure );
    }
}

class CSVIterator implements Iterator {

    /**
     * File handler
     * @var CSVFileObject
     */
    private $csvFile;

    /**
     * Current line number
     * @var integer
     */
    private $key = -1;

    /**
     * Current line data
     * @var array
     */
    private $current = array();

    /**
     * Constructs a new CSV Iterator
     * @param CSV $csv O manipulador do arquivo CSV
     */

    public function __construct( CSVFileObject $csvFile ){
        $this->csvFile = $csvFile;
        $this->rewind();
    }

    /**
     * @return array
     */
    public function current(){
        return $this->current;
    }

    /**
     * Recupera a linha atual
     * @return integer
     */
    public function key(){
        return $this->key;
    }

    /**
     * Avança para o próximo elemento
     */
    public function next(){
        $this->current = $this->csvFile->getCSVLine();

        ++$this->key;
    }

    /**
     * @return void
     */

    public function rewind(){
        $this->key = -1;
        $this->current = array();
        $this->csvFile->seek( 0 );
        $this->next();
    }

    /**
     * @return boolean
     */
    public function valid(){
        return !$this->csvFile->eof();
    }
}

Then you can do:

$originalCustomerFile = new CSVFileObject("/path/to/originalCustomer.csv");
$originalVoucherFile = new CSVFileObject("/path/to/originalVoucher.csv");

$customerIt = $originalCustomerFile->getIterator();
$voucherIt = $originalVoucherFile->getIterator();

$customerIt->next();
$voucherIt->next();

if (!$customerIt->valid()) {
    throw new Exeption("Customer file is empty");
}

if (!$voucherIt->valid()) {
    throw new Exeption("Voucher file is empty");
}

$customerHeaders = $customerIt->current();
$voucherHeaders = $voucherIt->current();

// ...

$customerOutputFile = new CSVFileObject("/path/to/customer-output.csv", "w+");
$voucherOutputFile = new CSVFileObject("/path/to/voucher-output.csv", "w+");

$customerOutputFile->writeCSVLine($customerHeaders + ["voucherNumber", "dateApplied"]);
$voucherOutputFile->writeCSVLine($customerHeaders + ["id", "dateUsed"]);

$customerIt->next();
$voucherIt->next();
while ($customerIt->valid() && $voucherIt->valid()) {
    $currentCustomer = $customerIt->current();
    $currentVoucher = $voucherIt->current();

    $customerId = $currentCustomer[0];

    $newCustomerRow = $currentCustomer + $currentVoucher + [date("l jS \of F Y h:i:s A")];

    $newVoucherRow = [$currentVoucher[0], $customerId, date("l jS \of F Y h:i:s A")];

    $customerOutputFile->writeCSVLine($newCustomerRow);
    $vohcerOutputFile->writeCSVLine($newVoucherRow);

    $customerIt->next();
    $voucherIt->next();
}

Since now we're using the OO API, we don't have to worry about opening and closing the file, since this is handled by constructors and destructors.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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