Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm wondering if perhaps I am doing this all wrong.. see for yourself:

Question:

  1. Does my OOP suck?
  2. is running multiple methods on a value {ie: strip_tags($copy->truncateString($articles[$i]['body'], 250, " "))} a terrible way to manage resources?
  3. Should I create separate methods to use for the following line:

    echo '<div class="summary"><a href="/' . BreadCrumbs::getCrumb(1) . '/'. BreadCrumbs::getCrumb(2) . '/article/' . $articles[$i]['id'] . '"><h5>' . $articles[$i]['title'] . '</h5></a> (' . $articles[$i]['date'] . ')<p>' . strip_tags($copy->truncateString($articles[$i]['body'], 250, " ")) . '</p><p><a href="/' . BreadCrumbs::getCrumb(1) . '/'. BreadCrumbs::getCrumb(2) . '/article/' . $articles[$i]['id'] . '"> Read more</a></p></div>';
    

Notes:

  • pageTemplate is a class which will contain all html templats for this particular site
  • Two static classes take care of site wide configuration variables (not seen here) and tracking the url (BreadCrumbs::).
  • Calling BreadCrumbs::getCrumb(x) retrieves the 'value' at the x position of the url (ie: domain.com/"pos 1"/"pos 2"/"pos 3"). The values are managed at the top of the page and are checked if empty and replaced with a default value if they are.
  • GetCopy is a class which has a number of functions designed to run a query (based on various parameters) and retrieves an array (or mulch-dimensional array). The function used below retrieves a mulch-dimensional array with each second level array containing a whole article.
  • GetCopy has a parent. It extends a class called CopyMan which contains various functions such as adding paragraphs to a block of text, truncating, converting date forms, and pagination tools. Called through the GetCopy object.

    /**
     * homePage :: ()
     * PUBLIC method
     * = lay out the home page content
     *
     */
    
    public static function homePage() {
    
    // initialize GetCopy class
    $copy = new GetCopy();
    
    // special feature container
    echo '<div id="special_box"><div class="special_header"><img src="/images/layout/special_header.png"></div><div class="special_container">';
    $articles = $copy->getRows('articles', 'date', 0, 2);
    //print_r($articles);
    for ($i = 0; $i < count($articles); $i++) {
        echo '<div class="special_summary"><a href="/' . BreadCrumbs::getCrumb(1) . '/'. BreadCrumbs::getCrumb(2) . '/article/' . $articles[$i]['id'] . '"><h5>' . $articles[$i]['title'] . '</h5></a> (' . $articles[$i]['date'] . ')<p>' . strip_tags($copy->truncateString($articles[$i]['body'], 250, " ")) . '</p><p><a href="/' . BreadCrumbs::getCrumb(1) . '/'. BreadCrumbs::getCrumb(2) . '/article/' . $articles[$i]['id'] . '"> Read more</a></p></div>';
    }
    echo '</div></div>';
    }
    

EDIT: Just wanted to say thanks to those who have helped so far. I never went to university, took computer science, or ever worked in a team whereby I had a mentor. Learning everything on your own with no background to work from is hard, and its people like scrager and mseancole who become invaluable resources.

Further Information, to help provide insight:

-[Folder] Lib
    -[Folder] bespoke
        --> [Class] DisplayEngine
                    private static $instance;
                    public static function getInstance()
                    + Methods for laying out divs.
        --> [Class] PageTemplate
                    public function homePage()
    -[Folder] copy
        --> [Class] CopyMan
                    public function truncateString($string, $limit, $break='.', $pad='...')
                    public function convertToPara($string)
                    public function convertToLongDate($date)
                    protected function paginateList($table, $current_page, $list_length)
                    protected function setLimits($table, $page, $numArticles, $featured)
        --> [Class] GetCopy extends CopyMan
                    require_once(Config::getAbsPath() . '/lib/omc_frmwrk/copy/CopyMan.php');
                    public function getFieldContents($table, $where_field, $where_match, $column)
                    public function getRow($table, $field_array, $where_field, $where_match)
                    public function getRows($table, $order_by, $limit_start, $limit_end)
                    public function getRowsWhere($table, $order_by, $where_field, $where_match, $limit_start, $limit_end)
    -[Folder] database
        --> [Class] DbMan
                    private static $db_host = Config::db_host;
                    private static $db_user = Config::db_user;
                    private static $db_pass = Config::db_pass;
                    private static $db_name = Config::db_name;
                    private $mysql_connection;
                    private $mysql_db;
                    private $query;
                    private $query_result;
                    private function dbConnect()
                    protected function executeQuery($query)
        --> [Class] DbQuery extends DbMan
                    private $query;
                    public function prepareQuery($query_request)
    -[Folder] helpers
        --> [Class] ArrayHelp
                    public function recValueCheck($needle, $haystack)
                    public function recValueReturn($needle, $haystack)
    -[Folder] layout
        --> [Class] LogoMan
                    private static $site_logo;
                    public static function setSiteLogo($x) { self::$site_logo = $x; }
                    public function printLogo()
    -[Folder] navigation
        --> [Class] BreadCrumbs
                    private static $bread_crumbs;
                    private static $instance;
                    public static function getCrumbs() { return self::$bread_crumbs; }
                    public static function getCrumb($x) { return self::$bread_crumbs[$x]; }
                    public static function setCrumbs($x)        { self::$bread_crumbs = explode("/", $x); }
                    public static function getInstance() { if (!self::$instance) { self::$instance = new self(); } return self::$instance; }
                    public static function setEmptyCrumb($crumb, $fallback)
        --> [Class] NavMan
                    private static $links;
                    private static $nav_id;
                    public static function setLinks($x) { self::$links = $x; }
                    public static function setNavId($x) { self::$nav_id = $x; }
                    public function printNav($pre_link = "", $post_link = "")
    --> [Class] Config
                const db_host =
                const db_user =
                const db_pass =
                const db_name =
                private static $abs_path;
                private static $include_css = array ()
                private static $include_classes = array ()
                private static $include_js = array ()
                private static $site_logo = array ()
                private static $nav_primary = array ()
                private static $nav_secondary = array ()
                + Getters and Setters for all above
    --> [Class] initialize
                private static $include_css;
                private static $include_classes;
                private static $include_js;
                public function __construct() //*sets session, runs include methods below *//
                private function loadStylesheets()
                private function loadRequiredClasses()
                public function loadJavascript()
share|improve this question

2 Answers 2

up vote 2 down vote accepted

Does my OOP suck?

I don't know, I'm not seeing much OOP. I am seeing a bunch of static calls, which are completely contrary to the whole OOP concept. As far as the singleton suggested by scragar, you should avoid those as well. They are usually considered bad practice. See this answer on SO for more details. For the purposes you are employing, I would suggest that a simple helper function should suffice. No need for a full class if all you are doing is parsing the URL.

is running multiple methods on a value a terrible way to manage resources?

Let's take a look.

strip_tags($copy->truncateString($articles[$i]['body'], 250, " "));

//compared to...
$maxLength = 250;//should be a config constant
$body      = $article[ 'body' ];
$truncated = $copy->truncateString( $body, $maxLength, ' ' );
$synopse   = strip_tags( $truncated );

It's a little more verbose, but much easier to read and understand what is going on. You have labeled your magic number, you have labeled your text to be modified, and you have separated the modifications (allowing for further steps to be added/removed if/as necessary). This goes along with that "Max Line Length" scragar mentioned in his answer. Usually you will want to limit your lines of code to 80 characters, including indentation. Why 80? Well, its commonly accepted that most displays will display at least 80 characters and older machines/tools used to limit the display to just 80. Asides from this, the main reason for this rule of thumb is to avoid complex, difficult to read, code. Usually this is easiest to see with long lines of code, but can just as easily be seen with the smaller, such as this.

Something you may want to consider is creating a method to accomplish this for you. You can probably create these synopsis when you create the $articles array and append it to that allowing for much easier access during rendering. ($article[ 'synopse' ]; ).

Should I create separate methods to use for the following line:

I'd suggest separating the logic from the display. These are two entirely separate concerns, and as such violate the Separation of Concerns (SoC) Principle. The rule of thumb here is that you should avoid having PHP render your HTML in case something happens and it stops working, otherwise you could just end up with a blank page. Your pages should always, minimally, work without PHP. Usually you would use HTML with only minor, absolutely necessary, PHP to render your view and default text should something happen. This also helps with legibility and allows for features such as tag matching/highlighting and proper auto-complete to work in your IDE.

<?php foreach( $articles AS $article ) : ?>

<div class="summary">
    <a href="<?php echo $breadcrumbs . $article[ 'id' ]; ?>">
        <h5>$article[ 'title' ]</h5>
    </a>
    (<?php echo $article[ 'date' ]; ?>)
    <p>
        <?php
            //enter code from above here
        ?>
    </p>
    <p>
        <a href="<?php echo $breadcrumbs . $article[ 'id' ]; ?>">
            Read more
        </a>
    </p>
</div>

<?php endforeach; ?>

Edit for clarity on includes

article.inc

<div class="summary">
    <a href="<?php echo $breadcrumbs . $article[ 'id' ]; ?>">
        <h5>$article[ 'title' ]</h5>
    </a>
    (<?php echo $article[ 'date' ]; ?>)
    <p>
        <?php
            //enter code from above here
        ?>
    </p>
    <p>
        <a href="<?php echo $breadcrumbs . $article[ 'id' ]; ?>">
            Read more
        </a>
    </p>
</div>

page where include is used

<?php foreach( $articles AS $article ) :
    include article.php;
endforeach; ?>
share|improve this answer
    
+1 Advice given here is superior to the accepted answer –  Rob Apodaca Dec 24 '12 at 15:27
    
Thanks for taking the time mseancole. ::1:: What do you mean you only see static calls? Have I not understood the object model enough? I figured I was passing data and other objects to objects that react to what has passed onto them? Not OOP? –  obmon Dec 25 '12 at 9:35
    
::2:: I see how the more verbose code is easier to expand, I'm a little wary of having too many lines of code.. but I guess that i should stick to the "readability is better than efficiency" adage. I am creating a separate class with the method to handle this though, as per scrager (and your) suggestion. –  obmon Dec 25 '12 at 9:36
    
::3:: The problem with separating the HTML is that i will be repeating myself a lot. On a page with a list of articles, each article is a divs within divs.. I understand the possibility of ending up with blank pages, but how else do I achieve DRY? Is DRY only for the PHP? and not for the HTML? Thanks for your suggestions and the link. I will read more on it, and hopefully reach a better technique. –  obmon Dec 25 '12 at 9:38
1  
The only way that would ever be a resource hog is if you were to use the *_once() versions numerous times. And I mean quite a few times, hundreds maybe even thousands. I don't know the exact limit, I've never reached it. But that's only because PHP has to ensure that the included page hasn't already been included. That's why the *_once() versions should be avoided unless absolutely necessary. –  mseancole Dec 26 '12 at 20:01

I have a few good suggestions for you.

Max Line Length

Try to cut down on the length of your lines, at my current resolution(running from a cheap laptop on the train) stackexchange displays 90 characters per code line, your longest lines are far above 300, for sharing code try to keep it manageable.

Foreach

You're using a for loop for an array, which is fine, but the foreach function is more efficient if you're iterating all objects, and makes things a little cleaner.

Abstraction

In this instance I think you should abstract your display, maybe make a class to handle that stuff?

Put logic where it belongs

Put simply put the act of displaying the article in your display class, and leave the logic where it is.

Demo of what I think it should look like

public static function homePage() {

  $copy = new GetCopy();

  $display = DisplayEngine::GetInstance();

  $display->printSpecialHeader();

  $display->openSpecialContainer();

  $articles = $copy->getRows('articles', 'date', 0, 2);

  foreach( $articles AS $article ){

    $display->printArticleLink($article);

  }

  $display->closeSpecialContainer();

}

...

class DisplayEngine {

   public function printArticleLink($article){

     echo '<div class="special_summary"><a href="/'

         . BreadCrumbs::getCrumb(1)

         . '/'

         . BreadCrumbs::getCrumb(2)

         . '/article/'

         . $article['id']

          . '"><h5>'

          . htmlspecialchars($article['title'])

          . '</h5></a> ('

          . $article['date']

          . ')<p>'

          . $copy->truncateString(
                  strip_tags($article['body']),
                  250,
                  " "
            )

         . '</p><p><a href="/'

         . BreadCrumbs::getCrumb(1)

         . '/'. BreadCrumbs::getCrumb(2)

         . '/article/'

         . $article['id']

         . '"> Read more</a></p></div>';

   }

}
share|improve this answer
    
That's beautiful man. Thank you! It was that long echo statement that really bugged me. I will do as you suggest and use a separate class for it. Should I keep the function static? Hows the OOP path I'm on? –  obmon Dec 24 '12 at 12:09
    
Also, DisplayEngine::GetInstance(); is the singlton pattern, correct? Isn't that the same as using a static class? .. a quick google later and I think this looks like a better way of handling my Config and BreadCrumbs classes (currently just static classes) –  obmon Dec 24 '12 at 12:15
    
or maybe not.. moisadoru.wordpress.com/2010/03/02/… –  obmon Dec 24 '12 at 12:22
    
@obmon Yes it's a singleton, and they are slower, however it's commonly accepted that you use singletons over static classes for things that can hold config data, since this was a display class I imagined it having some sort of options specifying a stylesheet for the page, maybe a language or similar. If speed is essential use a static class, otherwise I suggest the speed difference is likely negligible. –  scragar Dec 24 '12 at 13:08
    
@obmon as for your style of oop, it's not bad, I'd suggest using a class for the articles so you can use type hinting and maybe move your truncating of the string onto the article. In fact you could probably have an ArticleManager class to overlook this stuff for you if it implements the Iterator interface you could make it work exactly the same but call it as $articles = new ArticleManager(); $articles->getArticles( 0, 2, 'date' ); foreach( $articles AS $article ){ echo $article->previewText; } –  scragar Dec 24 '12 at 13:30

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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