2
\$\begingroup\$

I think my code is sloppy can anyone give me advice to do it better?

This is a page pagination that get rows from mysql db and calculate the number of pages.

<div class="row">
    <div class="twelve columns tac">
        <?php

        //Check for the current page and select one of the two versions of "back" link.
        if (isset($_GET["page_num"]) && $_GET["page_num"] >= 2): ?>
            <a href="?page=blog&page_num=<?php echo($_GET["page_num"] - 1); ?>">Back</a>
        <?php else : ?>
            <span>Back</span>
        <?php endif;

        //looping through the number of pages and list page numbers.
        for ($i = 1; $i <= $totalPages; $i++): ?>
            <?php if ($i == $_GET["page_num"]) : ?>
                <span><?php echo $i; ?></span>
            <?php else : ?>
                <a href="?page=blog&page_num=<?php echo $i; ?>"> <?php echo $i; ?></a>
            <?php endif; ?>
        <?php endfor;

        //Check for the current page and select one of the two versions of "Next" link.
        if ($_GET["page_num"] < $totalPages): ?>
            <a href="?page=blog&page_num=<?php echo($_GET["page_num"] + 1); ?>">Next</a>
        <?php else : ?>
            <span>Next</span>
        <?php endif; ?>
    </div>
</div>
\$\endgroup\$
  • \$\begingroup\$ Hi and welcome to Code Review. "how to improve my code" is implied for every question on this site. Please edit your title to describe what your code is/does. In addition, it helps us when reviewing your code if you can give a brief explanation of what its purpose is and a general overview of the program flow. \$\endgroup\$ – Kaz Oct 14 '15 at 8:33
  • \$\begingroup\$ Thanks! I've edited the title and added some comments that describe what the code does. \$\endgroup\$ – Patiphan Mahamat Oct 14 '15 at 14:30
  • 1
    \$\begingroup\$ Just a note, don't add comments into the code that weren't there originally. Code commenting is one of the aspects that a review might pick up on. You can just give a brief paragraph before your code block instead. \$\endgroup\$ – Kaz Oct 14 '15 at 14:56
2
\$\begingroup\$

Checking if a variable is set

            if (isset($_GET["page_num"]) && $_GET["page_num"] >= 2): ?>

Here you check if $_GET["page_num"] is set before using it, which is correct. However, you don't do so in any of the later tests. Consider starting with something the code block with something like

$currentPage = isset($_GET['page_num']) ? $_GET['page_num'] : 1;

Now you can just use $currentPage instead. And it will always be set. It defaults to 1, as that is the normal pagination behavior (no selected page means the first page).

I also changed the double quotes to single quotes. In PHP, double quotes means that a string is open to variable interpolation. You aren't using variable interpolation, so you might as well use single quotes instead.

Later you can change the original line of code to

            if ($currentPage > 1) {
?>

Note that I also moved the closing ?> to its own line. This makes it much easier to tell when you switch from code to HTML.

I also switched from the non-standard : notation. Very little PHP code is written that way. The only time that I've actually seen it is in WordPress templates.

And I switched from >= 2 to > 1 because 2 is not a significant number here. What you are saying is that you don't want allow the back link on the first page, only on pages after the first. This translates to > 1 more directly.

Separating PHP and HTML

I'd tend to write this as

<?php
    $currentPage = isset($_GET['page_num']) ? $_GET['page_num'] : 1;
?>
    <div class="row">
      <div class="twelve columns tac">
<?php
    echo getPaginationLinks($currentPage, $totalPages);
?>
      </div>
    </div>

and define getPaginationLinks in a separate file with most of the original block. The getPaginationLinks function would build and return a string.

Note that I put the <?php> and ?> on separate lines and at the beginning of each line. This makes it easier to tell when you are switch from HTML to PHP code.

Don't switch context for no reason

You have

            for ($i = 1; $i <= $totalPages; $i++): ?>
                <?php if ($i == $_GET["page_num"]) : ?>

You could just as well write this

            for ($i = 1; $i <= $totalPages; $i++):
                if ($i == $_GET["page_num"]) :
?>

Then the compiler isn't switching from PHP context to HTML context just to print out some meaningless whitespace. As @tim said, in this case it would be even better to build the string in PHP code rather than mixing PHP and HTML. In other situations this construct would be more acceptable. But there's still no point in switching out of PHP context unless you want to display something before you switch back into it.

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

Yes, mixing PHP and HTML can get messy quite quickly.

If you do not want to switch to some templating engine, the easiest way to clean this up is to build the entire HTLM block in a PHP variable and then echo it:

$paginationHTML = '
<div class="row">
    <div class="twelve columns tac">';

if (isset($_GET["page_num"]) && $_GET["page_num"] >= 2) {
    $paginationHTML .= getPaginationLink($_GET["page_num"] - 1, "Back");
} else {
    $paginationHTML .= 'Back';
}

for ($i = 1; $i <= $totalPages; $i++) {
    if ($i == $_GET["page_num"]) {
        $paginationHTML .= $i;
    } else {
        $paginationHTML .= getPaginationLink($i, $i);
    }   
}

if ($_GET["page_num"] < $totalPages) {
    $paginationHTML .= getPaginationLink($_GET["page_num"] + 1, "Next");
} else {
    $paginationHTML .= 'Back';
}

$paginationHTML .= '
    </div>
</div>
';

echo $paginationHTML;

function getPaginationLink($number, $linkText) {
    return '<a href="?page=blog&page_num=' . htmlspecialchars($number, ENT_QUOTES, 'UTF-8') . '">' 
        . htmlspecialchars($linkText, ENT_QUOTES, 'UTF-8') . '</a>';
}

It is of course a matter of taste, but I find this a lot easier to read.

I also extracted some duplication to a function and removed the span tags which did not seem to have any purpose.

You could generalize the function even more, resulting in less code, but also possibly reduced readability:

$paginationHTML = '
<div class="row">
    <div class="twelve columns tac">';

$paginationHTML .= getPagination($_GET["page_num"] - 1, "Back", isset($_GET["page_num"]) && $_GET["page_num"] >= 2);

for ($i = 1; $i <= $totalPages; $i++) {
    $paginationHTML .= getPagination($i, $i, $i != $_GET["page_num"]);
}

$paginationHTML .= getPagination($_GET["page_num"] + 1, "Next", $_GET["page_num"] < $totalPages);

$paginationHTML .= '
    </div>
</div>
';

echo $paginationHTML;

// TODO comment
function getPagination($number, $linkText, $doLink) {
    if ($doLink) {
        return '<a href="?page=blog&page_num=' . htmlspecialchars($number, ENT_QUOTES, 'UTF-8') . '">' 
            . htmlspecialchars($linkText, ENT_QUOTES, 'UTF-8') . '</a>';
    } else {
        return htmlspecialchars($linkText, ENT_QUOTES, 'UTF-8');
    }
}

The XSS protection is not strictly necessary, but when writing a generic function which builds HTML, it's recommended, as the function may be reused in different contexts.

\$\endgroup\$
1
\$\begingroup\$

You should really consider using an orm like RedBean. In my own project, i got tired of writing hundreds of paginated queries. the methods were huge and complex and just a nightmare to manage. I replaced them all with two convenience functions:

// returns number of pages, current page, and results in an associative array.
public function paginateWithPageCount($sql,$params=[],$page=1, $per_page=20)
{
    //first we need to prepare our query to count the number of rows

    $sql2 = $sql; //copy sql at this point.
    // to calculate the total number of rows for the paginater, 
    // we need to delete the sql up to the `FROM` keyword
    $test = 'FROM';
    $n = strpos($sql2,$test);
    $keep = substr($sql2,$n+strlen($test));
    $sql2 = 'SELECT COUNT(id) AS num '.$keep;


    //we know that php is 0 based, but for presentation purposes pagination should be 1 based.
    //citing this knowledge, we always need to subtract `1` from $page;
    $page = (int) $page;
    if($page < 1){
        $page = 1;
    }
    $offset = abs($per_page * ($page - 1)); // page - 1 * per_page = offset

    //now we append our limit to the $sql var
    $sql.=' LIMIT '.$offset.','.$per_page;

    if(empty($params)){
        $data = \R::getAll($sql);
        $num = \R::getCell($sql2); //the getCell method returns the value of a single column of a single row query.
    }else{
        $data = \R::getAll($sql,$params);
        $num = \R::getCell($sql2,$params);
    }
    $num = $num > $per_page ? ceil($num/$per_page) : 1; //tada, now we have the total number of pages.

    return ['pages'=>$num,'current'=>$page,'results'=>$data];
}

//returns only results
public function paginate($sql,$params=[],$page=1, $per_page=20)
{
    //we know that php is 0 based, but for presentation purposes pagination should be 1 based.
    //citing this knowledge, we always need to subtract `1` from $page;
    $page = (int) $page;
    if($page < 1){
        $page = 1;
    }
    $offset = abs($per_page * ($page - 1)); // page - 1 * per_page = offset
    $sql.=' LIMIT '.$offset.','.$per_page;
    if(empty($params)){
        $data = \R::getAll($sql);
    }else{
        $data = \R::getAll($sql,$params);
    }
    return $data;
}
\$\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.