3
\$\begingroup\$

The first thing I would like to say is that I do have a fully functioning script, but the script is ugly, and I am wondering if there is a way to optimize it.

The script looks to see if I have at least 6 news articles in my database.

  • If I have at least 6 news articles, it branches into a nested if statement.
  • If the total count is equal to or less than three, it will only show up to three.
  • If the count is more than 3 it will load the next three.

I have a jQuery pagination system, and what I want is for the first three to load, and if there is more than three after, then have the next three load, with a maximum of 6.

What I would like to know is if there is a cleaner way to do what my code does? I am converting my site to a whole new design, and a lot of my sites information is displayed in different ways. What I am not sure how to do, at least not cleanly, is something like accessing all entries in a database, and every three trigger a line break, and after nine trigger another event, etc. Not sure if this makes sense. Anyways here is the script I have currently.

<?php
$get_article_count = mysql_query("SELECT count(*) as total FROM table WHERE location='public' AND process='Published'");
$article_count = mysql_fetch_assoc($get_article_count);

if($article_count['total'] <= 6){
?>  
<div class="item active">
<div class="row">

    <?php 
        $get_public_articles = mysql_query("SELECT * FROM table WHERE location='public' AND process='Published' ORDER BY articleid DESC LIMIT 0,3");
        while($public_articles = mysql_fetch_array($get_public_articles)){ 
    ?>

    <div class="col-md-4">
        <div class="item news-item">
            <img alt="" src="upload/blog1.jpg">
            <h2><a href="http://www.shiningashes.com/<?=$langauge?>/articles/<?=$public_articles['articleid'];?>"><?=$public_articles['title'];?></a></h2>
            <p>
            <?php
                $pos = str_replace("\\", "", strpos($public_articles['message'], ' ', 170));
                if ($pos !== false) {
                    echo str_replace("\\", "", substr($public_articles['message'], 0, $pos));
                }
            ?>...
            </p>
            <ul class="blog-tags">
                <li><a class="date" href="#"><i class="fa fa-clock-o"></i><?=date('l dS \of F Y', $public_articles['date']);?></a></li>
            </ul>
        </div>
    </div>

    <?php
    }
    ?> 


</div>
</div>

<div class="item">
<div class="row">

    <?php 
        $get_public_articles = mysql_query("SELECT * FROM sa_news WHERE location='public' AND process='Published' ORDER BY articleid DESC LIMIT 3,6");
        while($public_articles = mysql_fetch_array($get_public_articles)){ 
    ?>

    <div class="col-md-4">
        <div class="item news-item">
            <img alt="" src="upload/blog1.jpg">
            <h2><a href="http://www.shiningashes.com/<?=$langauge?>/articles/<?=$public_articles['articleid'];?>"><?=$public_articles['title'];?></a></h2>
            <p>
            <?php
                $pos = str_replace("\\", "", strpos($public_articles['message'], ' ', 160));
                if ($pos !== false) {
                    echo str_replace("\\", "", substr($public_articles['message'], 0, $pos));
                }
            ?>...
            </p>
            <ul class="blog-tags">
                <li><a class="date" href="#"><i class="fa fa-clock-o"></i><?=date('l dS \of F Y', $public_articles['date']);?></a></li>
            </ul>
        </div>
    </div>

    <?php
    }
    ?> 


</div>
</div>
<?php
}
?> 

I know that if I put the HTML within echo tags, so it is all PHP, it would be cleaner, but I wanted to get it functioning before converting the whole news "template" to PHP as well.

\$\endgroup\$
3
\$\begingroup\$

First, putting all HTML into echo does not necessarily make things cleaner. More typically, it makes them full of escaped quotes and difficult to edit in an IDE. I personally only use echo for output that will span less than a full line. I use php tags and heredocs for multiple lines.

A lot of people don't know about heredocs, so I will use them in my example below. The nice thing about heredoc is that it forces you to keep your calculations separate from your outputs, as when I find $date and $message below. That kind of pattern makes for more readable code. However, there are a lot of places where heredoc doesn't make much sense -- usually when you are outputting things conditionally.

If you use heredoc, you must escape literal dollar signs. Keep in mind that the longer the string, the more PHP memory is required for it. Also, it messes up your indentation in PHP because you have to put the final delimiter ("EOT" in my example) on its own line. For this reason, I also put the original delimiter starting its own line, even though that's not technically necessary, and I always indent the text inside by at least one indentation level. It just makes it easier to find the beginning of the heredoc. Note that you can also assign your heredoc to a variable etc. instead of echoing it.

Second, I really hope you are getting rid of the use of the deprecated mysql extension during your upgrade. I will use mysqli in my example, but PDO is also a good alternative.

Third, if you want to do something every three rows, all you have to do is save an iterator variable to count for you. You can use the modulus operator (%) to find out if it is multiples of 3 or not.

Fourth, it's pointless to have two copies of the same HTML one right after another, and two copies of nearly-the-same database query. Just do it once.

$i = 0; 
$result = $mysqli->query("SELECT * FROM sa_news 
    WHERE location='public' AND process='Published' 
    ORDER BY articleid DESC ");
$numrows = $result->num_rows;
while ($public_articles = $result->fetch_assoc()) {
    if($i == 0 || $i%3 == 0) {  //beginning of a row
        echo 
<<<EOT
    <div class="item active">
    <div class="row">
EOT;
    }

    $message = '';
    $pos = str_replace("\\", "", strpos($public_articles['message'], ' ', 170));
    if($pos !== false) {
        $message = str_replace("\\", "", substr($public_articles['message'], 0, $pos));
    }
    $date = date('l dS \of F Y', $public_articles['date']);

    echo 
<<<EOT
    <div class="col-md-4">
    <div class="item news-item">
        <img alt="" src="upload/blog1.jpg">
        <h2><a href="http://www.shiningashes.com/{$language}/articles/{$public_articles['articleid']}">{$public_articles['title']}</a></h2>
        <p>{$message}</p>
        <ul class="blog-tags">
            <li><a class="date" href="#"><i class="fa fa-clock-o"></i>{$date}</a></li>
        </ul>
    </div>
    </div>
EOT;

    $i++;   //the column is now complete

    if($i%3 == 0 || $i == $numrows) {   //end of a row
        echo 
<<<EOT
    </div>
    </div>
EOT;
    }
}
\$\endgroup\$

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.