Using PHP to create a file structure

Question

I am using PHP to create a file structure in which to store XLSX files, for this I have created this class:

<?php
class XLSX
{
    /** @var resource */
    private $handle;
    /** @var array */
    private $data;
    /** @var array */
    private $headings;
    /** @var bool */
    private $finished = true;
    /** @var string */
    private $filename;

    /**
     * Creates the class, must be done each time a file is written to as the handle is stored.
     * @param string $filename The file to be written to, will create the file if it does not exist
     */
    public function __construct($filename)
    {
        $dirname = dirname($filename);
        if (!is_dir($dirname))
        {
            mkdir($dirname, 0777, true) ?: die("Failed to create the required directory.");
        }
        if (file_exists($filename))
        {
            for ($i = 1; $i <= 200; $i++)
            {
                $fname = explode(".", $filename);
                if (!file_exists("{$fname[0]}_{$i}.{$fname[1]}"))
                {
                    $this->handle = fopen("{$fname[0]}_{$i}.{$fname[1]}", "w");
                    $this->filename = str_replace(" ", "%20", "{$fname[0]}_{$i}.{$fname[1]}");
                    break;
                }
            }
        }
        else
        {
            $this->handle = fopen($filename, "w");
            $this->filename = $filename;
        }
        $this->finished = false;
    }

    /**
     * Set the headers for the XSLX
     * @param array $headers
     * @return bool
     */
    public function SetHeaders($headers)
    {
        return $this->AddToArray("headings", $headers);
    }

    /**
     * Get the headers that were set, if not set return null.
     * @return array|null
     */
    public function GetHeaders()
    {
        return !empty($this->headings) ? $this->headings : null;
    }

    public function SetData($data)
    {
        return $this->AddToArray("data", $data);
    }

    /**
     * Get the data that was set, if not set return null.
     * @return array|null
     */
    public function GetData()
    {
        return !empty($this->data) ? $this->data : null;
    }

    public function GetFilename()
    {
        return $this->filename;
    }

    /**
     * Set the handle and all data back to default, ready to recall a new instance.
     * @return void
     */
    public function Finish()
    {
        fclose($this->handle);
        $this->handle = null;
        $this->data = null;
        $this->headings = null;
        $this->finished = true;
        $this->filename = null;
    }

    public function Write()
    {
        if (!$this->finished && isset($this->handle))
        {
            if ($this->GetHeaders() == null || $this->GetData() == null)
            {
                return false;
            }
            foreach ($this->GetHeaders() as $header)
            {
                fwrite($this->handle, $header . "\t");
            }
            fwrite($this->handle, "\n");
            foreach ($this->GetData() as $data)
            {
                if (is_array($data))
                {
                    foreach ($data as $d)
                    {
                        fwrite($this->handle, $d . "\t");
                    }
                    fwrite($this->handle, "\n");
                }
                else
                {
                    fwrite($this->handle, $data . "\t");
                }
            }
        }
        return false;
    }

    /**
     * Add items to an array
     * @param string $arrayName
     * @param mixed $values
     * @return bool
     */
    private function AddToArray($arrayName, $values)
    {
        if (!$this->finished)
        {
            foreach($values as $val)
            {
                $this->{$arrayName}[] = $val;
            }
        }
        return !empty($this->{$arrayName}) ? true : false;
    }
}

Currently I am only testing the __construct method, and this works, but I wanted to know if there is a better way to do what I am doing?

I know it is incomplete, but I need to know if this is right for the __construct method

Edit
I have completed the class and tested, it works perfect, but it seems there has to be a better way to do the __construct method and also the Write method. I have updated the code to show these changes

For anyone who may want to know, here is how this is used:

<?php require_once 'xlsx/XLSX.php'; $xlsx = new XLSX("downloads/" . date("j M Y") . "/test.xls"); ?>
<?php $xlsx->SetHeaders(['firstname', 'lastname', 'middlename']); ?>
<?php $xlsx->SetData([['FName', 'LName', 'MName'], ['FName', 'LName', 'MName'], ['FName', 'LName', 'MName']]); ?>
<?php $xlsx->Write(); ?>
<?php $xlsx->Finish(); ?>

I started writing an answer to this question, but I haven't finished it. However, in a nutshell. There's too much logic in your constructor, and it could be moved to another method. The constructor is not the place to create files, or determine if a file exists, etc. Constructor should initialize all the information the object will need to work. Also, you should consider following PSR-2 coding standards. github.com/php-fig/fig-standards/blob/master/accepted/… — ILikeTacos, Commented Dec 2, 2015 at 15:09

kph0x1 · Accepted Answer · 2015-12-30 15:13:29Z

As already mentioned, could move code from constructor, implementing something where have control over construction; i.e., a design pattern. Can refactor the tests for available file name, directory creation, etc into separate predicate methods; i.e., isFileNameAvailable, isReady.

Following can be resource expensive, specifically the file_exists call in a loop.

for ($i = 1; $i <= 200; $i++)
{
    $fname = explode(".", $filename);
    if (!file_exists("{$fname[0]}_{$i}.{$fname[1]}"))

Perhaps replace with a one-time test for existence rather than using a loop. May could be done using existing file system utilities, depending on one' OS. Concatenate some input for ls command with shell_exec or similar to get string returned. Suggest determining what is in the folder as a first step rather than trying to increment and test for available filenames using this character by character outward approach.

If nothing else, remove all code from loops which can be placed outside it, such as $fname = explode(".", $filename);

Community · Accepted Answer · 2017-05-23 12:41:05Z

I'll quickly throw out an answer. It will lack a lot, but hopefully you'll find some points helpful.

Like @AlanChavez said in the comments, your constructor has too much logic. A constructor does exactly that, it sets up the object. It construct it so that you can begin using it. Typically, with multiple if statements is a sign of a bad constructor. Consider breaking some of that code into other methods. Perhaps makeDirectory() and checkFile() or createFile(). As it is, I can imagine your constructor looking as simple as this:

public function __construct($filename)
{
    $this->dirname = dirname($filename);
    $this->filename = $filename;
    $this->finished = false;
}

Formatting is more important than you might believe. For example, in PHP, I have noticed that a wide variety of developers agree that methods should be in camelCase.

Instead of a Finish() function, I might suggest utilizing the __deconstruct() or the __unset() magic methods. They're actually quite useful.

Here are some pieces you might benefit from:

Stack Exchange Network

Using PHP to create a file structure

2 Answers 2

Your Answer

Hot Network Questions

Using PHP to create a file structure

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

Related

Hot Network Questions