5
\$\begingroup\$

This code produces HTML from the results of a database query.

Self-taught, I have concerns about developing bad habits and I wrestle with the principles of OOP and MVC.

I would be grateful for your opinion on a number of issues:

  1. The use of global. My HTML page is generated from a template which requires a config file that contains the database connection. Adding global $apcDatabase was a workaround but I don't understand why the connection was not present.
  2. The HTML code is returned from the PHP class and then displayed with an echo statement. Is this MVC separation?
  3. The use of extract in getDocuments. I've read that indiscriminate use of extract can be problematic particularly in code review and finding out where variables have come from. I've backed myself into a corner with this and can't see an alternative way.

All comments gratefully received.


Within the HTML page:

<?php
    global $apcDatabase;

    require_once(LIBRARY_PATH . "/populate_documents.php");

    $recommendation = new ApcDocs($apcDatabase,'recommendation');

    $documents = $recommendation->getDocuments();

    foreach ($documents as $document) {

        echo $recommendation->addSymbols($document['header'])
                . $recommendation->addSymbols($document['label'])
                . $recommendation->addSymbols($document['title'])
                . $recommendation->addSymbols($document['text']);
    }

?>

Supporting PHP code:

class ApcDocs
{

    private $apcDatabase;
    private $documentType;
    private $documents;
    private $firstLetter;
    private $lastLetter;
    const URL = "http://www.website.com/";
    const RECOMMENDATION = "folder/path/";

    public function __construct($apcDatabase, $documentType)
    {
        $this->apcDatabase = $apcDatabase;
        $this->documentType = $documentType;
        $this->docQuery();
        $this->docResults();
    }

    private function docQuery()
    {
        $definitions = array(
            'recommendation' => "SELECT rag_status.rag_status,
                                        rag_status.rag_label,
                                        apc_documents.document_title,
                                        apc_documents.document_text,
                                        apc_documents.file_name
                                 FROM apc_documents
                                 JOIN rag_status
                                 ON apc_documents.rag_status_id_fk = rag_status.rag_status_id
                                 WHERE document_type_id_fk = 1
                                 ORDER BY apc_documents.document_title ASC"
        );

        $this->apcDatabase->query($definitions[$this->documentType]);
    }

    private function docResults()
    {
        $this->documents = $this->apcDatabase->resultSet();
    }

    public function getDocuments()
    {
        $docDetails  = array();
        $this->lastLetter = null;

        foreach ($this->documents as $document => $details) {

            extract($details, EXTR_OVERWRITE); // create variables from $details

            $this->firstLetter = $this->getFirstLetter($document_title);

            $docDetails[$document]['header'] = $this->headerCode();
            $docDetails[$document]['label'] = $this->labelCode($rag_label, $rag_status);
            $docDetails[$document]['title'] = $this->linkCode(self::URL . self::RECOMMENDATION . $file_name,
                                                              $document_title);
            $docDetails[$document]['text'] = $this->docText($document_text);

            $this->lastLetter = $this->firstLetter;
        }

        return $docDetails;

    }

    private function getFirstLetter($wordToCheck)
    {
        return substr($wordToCheck, 0, 1);
    }

    private function headerCode()
    {
        if ($this->sameHeader()) {
            return FALSE;
        } else {
            return  "<div class='page-header' id='$this->firstLetter'><h1>$this->firstLetter</h1></div>\n";
        }
    }

    private function sameHeader()
    {
        return $this->firstLetter == $this->lastLetter;
    }

    private function labelCode($label, $staus)
    {
        return "<span class='label label-$label'>$staus</span>&nbsp;";
    }

    private function linkCode($link, $title)
    {
        return "<a href='$link'>$title</a>\n";
    }

    private function docText($text)
    {
        return "<p>$text</p>\n";
    }

    public function addSymbols($text)
    {
        $superscript = str_replace("(r)", "<sup>&#174;</sup>", $text);
        return str_replace("(bt)", "&#9660;", $superscript);
    }

}

?>

Screen shot of product

\$\endgroup\$
2
  • \$\begingroup\$ Building html from raw data, on the one hand, and fetching data from a database, on the other, are separate concerns, and shouldn't be in the same class. Also, it's not immediately clear to me what kind of page this code is building -- which isn't a good sign, but might be okay depending on your domain. A sample screenshot would make the code easier to read. \$\endgroup\$
    – Jonah
    Commented Dec 6, 2015 at 1:18
  • \$\begingroup\$ Thanks Jonah. The end product of this code is an alphabetised list of monographs with corresponding links and labels, \$\endgroup\$
    – k_smd
    Commented Dec 6, 2015 at 9:16

1 Answer 1

2
\$\begingroup\$

To elaborate on my comment, building html from raw data, on the one hand, and fetching data from a database, on the other, are separate concerns.

So your first step should be splitting out the interaction with the database into a single class, let's call it Repository. That class can be initialized with your database connection details, and then it will be the only class that needs to know about those. Repository can have a static method like apcReportData, which returns the data you'll need for this report. You can of course add other methods for different kinds of pages or reports that you may need in the future.

As for your view, let's break it down into it's logical sections:

  1. The whole page itself, which consists of a list of sections, with letters as titles.
  2. A "lettered section", which consists of a title (a letter) and a list of articles.
  3. An individual article, which consists of a tag, an article title, and a summary.

I would break down your views correspondingly.

Consider using a template engine for creating views -- php has many.

However, in your case, the views are so simple, I'd consider using an extremely lightweight solution like the one described here. I've used it myself, it's a single short file include, and it works great.

You'd create a template for each of the three sections listed above. The whole page would make use of the "letter section" template, and the letter section template would make use of the individual article template.

Finally, you'd need one extremely simple class whose job would be to wire together these elements:

  1. Grabbing data from the Repository
  2. Transforming that data, if necessary, for the top level, whole page view.
  3. Calling the whole page view, feeding in the data, and returning the resulting html

This would be called something like MonographListBuilder, or AssembleMonographList.

\$\endgroup\$

Your Answer

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

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