HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Rendering articles with breadcrumb navigation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withnavigationbreadcrumbarticlesrendering

Problem

I'm wondering if perhaps I am doing this all wrong:

Questions:

  • How is my OOP?



  • Is it running multiple methods on a value {i.e.: is strip_tags($copy->truncateString($articles[$i]['body'], 250, " "))} a terrible way to manage resources?



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

echo '' . $articles[$i]['title'] . ' (' . $articles[$i]['date'] . ')' . strip_tags($copy->truncateString($articles[$i]['body'], 250, " ")) . ' Read more';


Notes:

  • pageTemplate is a class which will contain all HTML templates 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 '';
$articles = $copy->getRows('articles', 'date', 0, 2);
//print_r($articles);
for ($i = 0; $i ' . $articles[$i]['title'] . ' (' . $articles[$i]['date'] . ')' . strip_tags($copy->truncateString($articles[$i]['body'], 250, " ")) . ' Read more';
}
echo '';
}


Further Information to help provide insight:

`-[Folder] Lib

Solution

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 ''

          . htmlspecialchars($article['title'])

          . ' ('

          . $article['date']

          . ')'

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

         . ' Read more';

   }

}

Code Snippets

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>';

   }

}

Context

StackExchange Code Review Q#19888, answer score: 2

Revisions (0)

No revisions yet.