8
\$\begingroup\$

I've created and I manage a point of sale web application built in PHP which has thus far followed no clear guidelines or methodology for development; it's operation is completely procedural. In turn, because the department that's using it requests new and different features like it's a Las Vegas buffet, the software has become a mess which I'm terrified of (don't look it in the eyes). Thankfully, I'm the only developer and so no one else must feel the wrath of the beast I've created.

I've always had a hard time wrapping my head around OOP, but I think I'm finally beginning to understand the whole point behind encapsulating methods, protecting fields, and class inheritance. This brings me to my question: Given the object scheme posted below, am I doing this right? It works like it should and doesn't return any errors, but in terms of object design, I feel like a baby deer with wobbly legs, uncertain of the world around me.

To be a little more specific, should I have a separate class that encapsulates MySQL parameters - and where should it be included/inherited if many child classes, perhaps even on separate server requests, will need it?

Should these two classes be one? I thought to separate them for sake of file length - Is excessive file size a good indicator of when a class might need to be broken up?

Abstract, private, protected - I understand how this works in literal behavior, but in regard to use, I'm just swinging in the dark. Anyone care to shed a light on what I've done and whether it makes sense? I think that is the summation of my fears and concerns. Here is the code in question - Your replies will help guide my redesign/refactoring of everything I've spent the last 6 months on.

filterReports.class.php

date_default_timezone_set('America/Chicago'); // For use by 'date()' and 'strtotime()'

/*
*   First, we will create our appropriate file names for the dates in question,
*   then we will determine if today is a day to run said reports. If today is in fact
*   a fine day to create a report, we should then check to see if the desired
*   report has already been created. If it has not, we will create and save it.
*/

abstract class filterReports
{

    protected $reportFilenames = array(); // Store all file names in array, because it's fun
    protected $reportDirs = array(
        'daily-orders' => 'reports/daily/orders/',
        'weekly-orders' => 'reports/weekly/orders/',
        'monthly-orders' => 'reports/monthly/orders/',
        'daily-volume' => 'reports/daily/volume/',
        'weekly-volume' => 'reports/weekly/volume/',
        'monthly-volume' => 'reports/monthly/volume/',
    ); // Folders where we plan to store these reports

    protected function createFilenames()
    {

        // Comprehensive Order Data
        $this->reportFilenames['daily-orders']      = 'store-report-'
                                                    . date('Ymd', strtotime('-1 day'))
                                                    . '.csv'; // Yesterday's Report
        $this->reportFilenames['weekly-orders']     = 'store-report-'
                                                    . date('Ymd', strtotime('-8 days'))
                                                    . '-'
                                                    . date('Ymd', strtotime('-1 day'))
                                                    . '.csv'; // Last 7 Days
        $this->reportFilenames['monthly-orders']    = 'store-report-'
                                                    . date('Ymd', strtotime('first day of last month'))
                                                    . '-'
                                                    . date('Ymd', strtotime('last day of last month'))
                                                    . '.csv'; // Last Month

        // General Product Volume Data
        $this->reportFilenames['daily-volume']      = 'store-volume-'
                                                    . date('Ymd', strtotime('-1 day'))
                                                    . '.csv'; // Yesterday's Report
        $this->reportFilenames['weekly-volume']     = 'store-volume-'
                                                    . date('Ymd', strtotime('-8 days'))
                                                    . '-'
                                                    . date('Ymd', strtotime('-1 day'))
                                                    . '.csv'; // Last 7 Days
        $this->reportFilenames['monthly-volume']    = 'store-volume-'
                                                    . date('Ymd', strtotime('first day of last month'))
                                                    . '-'
                                                    . date('Ymd', strtotime('last day of last month'))
                                                    . '.csv'; // Last Month

    }

    protected $reportsToCreate = array();   // Based on what day it is, a different report may need to be created

    protected function chooseReports()
    {

        $this->reportsToCreate['daily-orders'] = TRUE; // Because 'every day' occurs every day.
        $this->reportsToCreate['daily-volume'] = TRUE;

        if (date('N', time()) == '1') { // If today is Monday, create weekly report
            $this->reportsToCreate['weekly-orders'] = TRUE;
            $this->reportsToCreate['weekly-volume'] = TRUE;
        } else {
            $this->reportsToCreate['weekly-orders'] = FALSE;
            $this->reportsToCreate['weekly-volume'] = FALSE;
        }

        if (date('j', time()) == '1') { // If today is the first day of the month, create monthly report
            $this->reportsToCreate['monthly-orders'] = TRUE;
            $this->reportsToCreate['monthly-volume'] = TRUE;
        } else {
            $this->reportsToCreate['monthly-orders'] = FALSE;
            $this->reportsToCreate['monthly-volume'] = FALSE;
        }

    }

    protected $reportsExist = array(); // Now let's see which reports have already been created

    protected function searchReports()
    {

        foreach ($this->reportsToCreate as $key => $val) {

            if ($val != FALSE) {                
                if (!file_exists($this->reportDirs[$key] . $this->reportFilenames[$key])) {
                    $this->reportsExist[$key] = FALSE;
                } else {
                    $this->reportsExist[$key] = TRUE;
                }
            }           

        }

    }

}

manageReports.class.php

include('filterReports.class.php');

/*
*   As an extension of the previous class 'filterReports', if a desired report has
*   not been found, we will create and save it.
*/

class manageReports extends filterReports
{

    public $newReport;

    private $dbConfig = array();
    private $con;   

    private function dbParams() // This should all probably go somewhere else, but I haven't decided where just yet
    {

        $this->dbConfig = array(
            'host' => 'hostname',
            'user' => 'username',
            'pass' => 'password',
            'name' => 'database',
        );
        $this->con = mysql_connect(
            $this->dbConfig['host'],
            $this->dbConfig['user'],
            $this->dbConfig['pass']
        ) or die('MySQL Error: ' . mysql_errno() . ' - ' . mysql_error());

    }

    private function createDailyOrdersReport() // Collect data and build the report body
    {

        // Do things to create $dataHeading and $dataContent strings
        $this->newReport = $dataHeading . $dataContent;

    }

    private function createDailyVolumeReport()
    {
        $this->newReport = $dataHeading . $dataContent;
    }

    private function createWeeklyOrdersReport()
    {
        $this->newReport = $dataHeading . $dataContent;
    }

    private function createWeeklyVolumeReport()
    {
        $this->newReport = $dataHeading . $dataContent;
    }

    private function createMonthlyOrdersReport()
    {
        $this->newReport = $dataHeading . $dataContent;
    }

    private function createMonthlyVolumeReport()
    {
        $this->newReport = $dataHeading . $dataContent;
    }

    private function writeReport($key)
    {

        // Save the report to its appropriate folder
        $writeReportName = $this->reportDirs[$key] . $this->reportFilenames[$key]; // I don't know why this works, seems to me to be out of scope for these array fields
        $writeReportOpen = fopen($writeReportName, 'w');
        fwrite($writeReportOpen, $this->newReport) or die('Unable to write file: ' . $writeReportName);
        fclose($writeReportOpen);

    }

    public function createReports() // Finally, resolve which reports should be created - then create them.
    {

        parent::createFilenames();  // Create the report file names
        parent::chooseReports();    // Decide whether a report should be run
        parent::searchReports();    // Find out if the report exists already

        foreach ($this->reportsExist as $key => $val) { // Any reports that should be created today, T or F for 'exists'
            if ($val != TRUE) {
                if ($key = 'daily-orders') {
                    $this->createDailyOrdersReport();
                    $this->writeReport($key);
                }
                if ($key = 'daily-volume') {
                    $this->createDailyVolumeReport();
                    $this->writeReport($key);
                }
                if ($key = 'weekly-orders') {
                    $this->createWeeklyOrdersReport();
                    $this->writeReport($key);
                }
                if ($key = 'weekly-volume') {
                    $this->createWeeklyVolumeReport();
                    $this->writeReport($key);
                }
                if ($key = 'monthly-orders') {
                    $this->createMonthlyOrdersReport();
                    $this->writeReport($key);
                }
                if ($key = 'monthly-volume') {
                    $this->createMonthlyVolumeReport();
                    $this->writeReport($key);
                }
            }
        }

    }

}

Finally, I do this to make it go.

include('manageReports.class.php');

$initReports = new manageReports;
$initReports->createReports();

I preemptively appreciate any and all assistance you can provide as it will undoubtedly make me less bogus of a web developer.

EDIT: Another question I just thought of in after-thought; Should I even bother declaring my fields and methods in filterReports as protected, seeing as though this class cannot be instantiated in the first place?

EDIT: I have made some revisions to my code based on responses. For now, forget about the above two classes - Here is my new code. The first class instantiates each of my report generating sub-classes and executes a public function contained in each of them. The second class is a single report generation class.

manageReports.class.php

include('store/library/reports/dailyOrdersReport.class.php');

/*
* Having included the desired reporting classes,
* we now need a uniform process for access and
* execution of these classes.
*/

class manageReports
{

    private $reports = array();

    public function __construct()
    {

        $this->createReports();

    }

    private function createReports()
    {

        $this->createReport('dailyOrdersReport');

    }

    private function createReport($class)
    {

        $this->reports[] = new $class;

    }

    public function go()
    {

        foreach ($this->reports as $report) {
            $report->execReport();
        }

    }

}

dailyOrdersReport.class.php

include('store/library/dbConfig.class.php'); // May need these parameters for dependent methods

/*
* Validates the need for specified report creation,
* and if true - does so.
*/

class dailyOrdersReport extends dbConfig
{

    private $newReport; // Variable to store report data

    private $reportPath;
    private $reportName;

    private $reportTestResult = FALSE;

    public function __construct() // Preprocess validation answers, "Should we create this report?"
    {

        parent::__construct(); // MySQL Parameters

        $this->reportPath   = 'store/reports/daily/orders/';
        $this->reportName   = 'store-orders_'
                            . date('Ymd', strtotime('-1 day'))
                            . '.csv';
        $this->reportTest();

    }

    private function reportTest() // We don't need to test against the date for this report, just if it's already been created
    {

        if (!file_exists($this->reportPath . $this->reportName)) {
            $this->reportTestResult = TRUE;
        } else {
            return;
        }

    }

    private function createReport()
    {

        // About 100 lines of csv report generating madness
        $this->newReport = $dataHeading . $dataContent;

    }

    private function writeReport()
    {

        // Save the report to its appropriate folder
        $writeReportName = $this->reportPath . $this->reportName;
        $writeReportOpen = fopen($writeReportName, 'w');
        fwrite($writeReportOpen, $this->newReport) or die('Unable to write file: ' . $writeReportName);
        fclose($writeReportOpen);

    }

    public function execReport()
    {

        if ($this->reportTestResult) { // Evaluates to 'true' if report aught be generated and saved to file
            $this->createReport();
            $this->writeReport();
        } else {
            return;
        }

    }

}

Once again, the trigger.

error_reporting(E_ALL);

date_default_timezone_set('America/Chicago'); // For use by 'date()' and 'strtotime()'

include('store/library/manageReports.class.php');

$manageReports = new manageReports;
$manageReports->go();

Is this perfect yet, or are there still miles to go before I sleep?

\$\endgroup\$
  • 1
    \$\begingroup\$ Just a style thing, but it seems really weird having verbs for class names. Classes describe objects, which are inherently "things" (ie: nouns). \$\endgroup\$ – cHao Feb 27 '11 at 4:02
  • \$\begingroup\$ So in effect, it would be more appropriate style to call manageReports -> ReportsManager and filterReports -> ReportsFilter? Good to know, and a helpful tool to stay in an objects mindset. \$\endgroup\$ – 65Fbef05 Feb 28 '11 at 13:04
7
\$\begingroup\$

When I see a bunch of repeated if tests of a value against several constants, I think "make these classes." You have a Report base class screaming to get out with one subclass per report type. If you think of each report in a general way, you'll start to see what operations it needs to support:

  • Decide if it should be run given the date
  • Check if it exists on disk
  • Generate its file name and title
  • Extract the data from MySQL into a text block
  • Write itself to disk

Here are the above requirements in an abstract base class, one designed to be extended by subclasses to fill out the specifics.

abstract class AbstractReport {
    private $directory;
    private $date;

    public function __construct($directory, $date) {
        $this->directory = 'reports/' . $directory;
        $this->date = $date;
    }

    public abstract function getTitle() ;
    public abstract function getFileName() ;
    public abstract function isNeeded() ;

    public function hasBeenRun() {
        return file_exists($this->key . $this->getFileName();
    }

    public function runIfNeeded() {
        if ($this->isNeeded() && !$this->hasBeenRun()) {
            $this->run();
        }
    }

    public function run() {
        $this->connectToDatabase();
        file_put_contents($this->getTitle(), $this->buildReport());
    }

    protected function connectToDatabase() {
        // ... mysql_connect() ...
    }

    protected abstract function buildReport() ;

    protected function formatDate($offset, $format='Ymd') {
        return date($format, strtotime($offset, $this->date));
}

Here is an example subclass for one of the reports.

class DailyOrdersReport extends AbstractReport {
    public function __construct($date) {
        parent::__construct('daily/orders/', $date);
    }

    public function getTitle() {
        return 'Daily Orders';
    }

    public function getFileName() {
        return 'store-report-' . $this->formatDate('-1 day');
    }

    public function isNeeded() {
        return true;  // or use $this->date to make determination
    }

    protected abstract function buildReport() {
        // ... pull data from database and return formatted text ...
    }
}

Hopefully this gives you a start on some OOness. :) I highly recommend the book Clean Code as it's a great help as you work to answer these questions for yourself.

Edit As you write the report classes, you may find that all orders reports share some functionality in common that volume reports don't and vice versa. If it's significant you may want to create more abstract classes AbstractOrdersReport and AbstractVolumeReport. If the only difference between the time frames is the dates passed to the database queries, you could gain a lot from this.

Of course what's missing now is a way to run the reports! The following is more procedural than OO, but it could be driven by a file or something similar.

class ReportManager {
    private $reports = array();

    public function __construct($date) {
        $this->date = $date;
        $this->createReports();
    }

    public function createReports() {
        // could read these from disk or a table
        $this->createReport('DailyOrders');
        $this->createReport('DailyVolume');
        $this->createReport('WeeklyOrders');
        $this->createReport('WeeklyVolumn');
        $this->createReport('MonthlyOrders');
        $this->createReport('MonthlyVolumn');
    }

    protected function createReport($class) {
        $this->reports[] = new $class($this->date);
    }

    public function runIfNeeded() {
        foreach ($this->reports as $report) {
            $report->runIfNeeded();
        }
    }

    public function run() {
        foreach ($this->reports as $report) {
            $report->run();
        }
    }
}

// ... and to kick it off ...

$date = time();
$manager = new ReportManager();
$manager->runIfNeeded();
\$\endgroup\$
  • \$\begingroup\$ +1 for pointing out my clear "area for improvement": Finding Objects. To verify I understand a few of your concepts, in createReport() you are creating an object for each report, then storing these in an array while the "what day is today/should this report be generated" logic is stored in each report class. When I created this thread, I had identified "automated report creation" as an object. Clearly from your response, this is too broad. What are the signs of an appropriately responsible class? What's too much or too little? \$\endgroup\$ – 65Fbef05 Feb 15 '11 at 21:07
  • \$\begingroup\$ Also, regarding protection - As a rule of thumb, should every field or method begin as private to then have restrictions loosened as the need arises? This question is more or less on common convention as I'm sure literal behavior changes from language to language. PS. I've edited my classes to reflect your suggested changes and posted them above. \$\endgroup\$ – 65Fbef05 Feb 15 '11 at 21:08
  • \$\begingroup\$ @65Fbef05 - Deciding class responsibilities takes practice, and often the classes tend to be abstract concepts like "a regularly-run report" that need subclasses to make them complete such as "a weekly volume report" and finally instances to make them useful such as "the weekly volume report for January 12th, 2011". One goal I try to follow is for each class to have a single major responsibility. Here AbstractReport is tasked with two: generate a report and manage its storage on disk. In this case I think it's okay, and you could always split them apart later. \$\endgroup\$ – David Harkness Feb 15 '11 at 21:41
  • \$\begingroup\$ As for access level, any public method/property becomes part of the external contract (API) that client classes will use and becomes difficult to change over time. If you start with private, there are no external dependencies to fix if you need to change it or elevate it to public status. The same is true of protected with subclasses. \$\endgroup\$ – David Harkness Feb 15 '11 at 21:43
  • 1
    \$\begingroup\$ I've just started reading 'Object Design: Roles, Responsibilities and Collaborations' - From reading the forward, the ability to define responsibility clearly seems to be an important skill to develop. \$\endgroup\$ – 65Fbef05 Feb 15 '11 at 21:55
0
\$\begingroup\$

The first comment is that you have six functions in manageReports which are identical, which each call a seventh one, createReports. While I understand that you mean to have logical function naming, given that you have all the logic as to when each program should be made located in the chooseReports and createReports function, you might as well leave off all the unnecessary ones.

For a more general answer, it doesn't seem like these two classes need to be separate. Class length isn't the main thing to consider here; you want to group things intelligently. Broadly speaking, you subclass only when you're making a more specific instance of the superclass. In your case, if you find that different types of reports require different functionality, could subclass different types of reports.

\$\endgroup\$
  • \$\begingroup\$ createDailyOrdersReport() and createDailyVolumeReport(), etc... will contain different behavior (I just left out that behavior for sake of brevity). Should this class be rewritten to exclude the actual report creation, bringing that functionality into a subclass? If so, I think I would have to duplicate the decision/execution logic before instantiation. \$\endgroup\$ – 65Fbef05 Feb 14 '11 at 19:23
  • \$\begingroup\$ I would venture that this depends on the complexity of the "different behavior". If its just a few small changes, then go ahead and put each in its own function. If each is drastically different, I would combine the two current classes into the base class, and subclass each report type. \$\endgroup\$ – eykanal Feb 14 '11 at 19:40
  • \$\begingroup\$ Granted, it's the longest of the functions, but createDailyOrdersReport() is 147 lines. If I wanted to place this function in a createDailyOrdersReport class and combine the above classes into one, how would I go about extending/strapping my new class to the other? \$\endgroup\$ – 65Fbef05 Feb 14 '11 at 20:19

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.