views:

63

answers:

2

I'm working on a real frankensite here not of my own design. There's a rudimentary CMS and one of the pages shows customer records from a MySQL DB.

For some reason, it has no probs picking up the data from the DB - there's no duplicate records - but it renders each row twice.

<?php
$limit = 500;
$area = 'customers_list';
$prc = 'customer_list.php';

if($_GET['page'])
{
    include('inc/functions.php');
    $page = $_GET['page'];
}
else 
{
    $page = 1;
}

$limitvalue = $page * $limit - ($limit);

$customers_check = get_customers();
$customers = get_customers($limitvalue, $limit);
$totalrows = count($customers_check);

?>
<!-- pid: customer_list -->

<table border="0" width="100%" cellpadding="0" cellspacing="0" style="float: left; margin-bottom: 20px;">
    <tr>
        <td class="col_title" width="200">Name</td>
        <td></td>

        <td class="col_title" width="200">Town/City</td>
        <td></td>

        <td class="col_title">Telephone</td>

        <td></td>
    </tr>

    <?php
    for ($i = 0; $i < count($customers); $i++)
    {
    ?>
    <tr>
        <td colspan="2" class="cus_col_1"><a href="customer_details.php?id=<?php echo $customers[$i]['customer_id']; ?>"><?php echo $customers[$i]['surname'].', '.$customers[$i]['first_name']; ?></a></td>
        <td colspan="2" class="cus_col_2"><?php echo $customers[$i]['town']; ?></td>
        <td class="cus_col_1"><?php echo $customers[$i]['telephone']; ?></td>

        <td class="cus_col_2">
            <a href="javascript: single_execute('prc/customers.prc.php?delete=yes&id=<?php echo $customers[$i]['customer_id']; ?>')" onClick="return confirmdel();" class="btn_maroon_small" style="margin: 0px; float: right; margin-right: 10px;"><div class="btn_maroon_small_left">
                <div class="btn_maroon_small_right">Delete Account</div>
            </div></a>
            <a href="customer_edit.php?id=<?php echo $customers[$i]['customer_id']; ?>" class="btn_black" style="margin: 0px; float: right; margin-right: 10px;"><div class="btn_black_left">
                <div class="btn_black_right">Edit Account</div>
            </div></a>
            <a href="mailto: <?php echo $customers[$i]['email']; ?>" class="btn_black" style="margin: 0px; float: right; margin-right: 10px;"><div class="btn_black_left">
                <div class="btn_black_right">Email Customer</div>
            </div></a>
        </td>
    </tr>
    <tr><td class="col_divider" colspan="6"></td></tr>
    <?php
    };
    ?>
</table>

<!--///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////-->
<!--// PAGINATION-->
<!--///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////-->
<div class="pagination_holder">

<?php
if($page != 1)
{
    $pageprev = $page-1;
?>
    <a href="javascript: change('<?php echo $area; ?>', '<?php echo $prc; ?>?page=<?php echo $pageprev; ?>');" class="pagination_left">Previous</a>
<?php
}
else
{
?>
    <div class="pagination_left, page_grey">Previous</div>
<?php
}
?>
<div class="pagination_middle">
<?php
$numofpages = $totalrows / $limit;

for($i = 1; $i <= $numofpages; $i++)
{
    if($i == $page)
    {
    ?>
        <div class="page_number_selected"><?php echo $i; ?></div>
    <?php
    }
    else
    {
    ?>
        <a href="javascript: change('<?php echo $area; ?>', '<?php echo $prc; ?>?page=<?php echo $i; ?>');" class="page_number"><?php echo $i; ?></a>
    <?php
    }
}

if(($totalrows % $limit) != 0)
{
    if($i == $page)
    {
    ?>
        <div class="page_number_selected"><?php echo $i; ?></div>
    <?php
    }
    else
    {
    ?>
        <a href="javascript: change('<?php echo $area; ?>', '<?php echo $prc; ?>?page=<?php echo $i; ?>');" class="page_number"><?php echo $i; ?></a>
    <?php
    }
}
?>
</div>
<?php
if(($totalrows - ($limit * $page)) > 0)
{
    $pagenext = $page+1;
?>
    <a href="javascript: change('<?php echo $area; ?>', '<?php echo $prc; ?>?page=<?php echo $pagenext; ?>');" class="pagination_right">Next</a>
<?php
}
else
{
?>
    <div class="pagination_right, page_grey">Next</div>
<?php
}
?>

</div>
<!--///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////-->
<!--// END PAGINATION-->
<!--///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////-->

I'm not the world's best PHP expert but I think I can see an error in a for loop when there is one... But everything looks ok to me. You'll notice that the customer name is clickable; clicking takes you to another page where you can view their full info as held in the DB - and for both rows, the customer ID is identical, and manually checking the DB shows there's no duplicate entries. The code is definitely rendering each row twice, but for what reason I have no idea.

All pointers / advice appreciated.

+1  A: 

Like jayrub wrote, StackOverflow is probably more appropriate for this.

But since you've already asked it, I'll make these recommendations:

1.) Use foreach() to look through arrays like this:

foreach ($customers as $i => $customer) {
    echo $customer['name'];
    ...
}

2.) Use var_dump() as Zoredache suggests, on $customers and $customers_check.

3.) Use semantic markup; so, for tables, you'll want something like:

<table>
    <thead>
        <tr>
            <th>Name</th>
            <th>Town</th>
            <th>Phone</th>
        </tr>
    </thead>
    <tbody>
        <!-- your php code here -->
    </tbody>
</table>

This will make it easier for screen readers and also make it easier to use certain scripts to generate rich web interfaces. See this page for more info: http://www.ferg.org/section508/accessible_tables.html

4.) Oh, and avoid inline CSS as much as possible. It's just bad form.

Lèse majesté
Absolutely agree - I wouldn't use inline CSS at all, I didn't design this site though! (Just trying to pick my way through it and fix all of its problems, one by one)Noted also about the semantic markup suggestion, I always try to design to conform with recommended accessibility standards whenveer possible. Fortunately I'm not so concerned about it for this particular design as the CMS is only used internally by a handful of people at present.
Christopher
A: 

Try the following, this is an updated version of your code.

Please read the comments that I made in it, this is just a sample of some good practices.

<?php
$limit = 500;
$area = 'customers_list';
$prc = 'customer_list.php';

if($_GET['page'])
{
    include('inc/functions.php');
    $page = (int)$_GET['page']; // safety (page always needs to be an integer
}
else 
{
    $page = 1;
}

$limitvalue = $page * $limit - ($limit);

$customers_check = get_customers();
$customers = get_customers($limitvalue, $limit);
$totalrows = count($customers_check);

?>
<!-- pid: customer_list -->

<table border="0" width="100%" cellpadding="0" cellspacing="0" style="float: left; margin-bottom: 20px;">
    <tr>
        <td class="col_title" width="200">Name</td>
        <td></td>

        <td class="col_title" width="200">Town/City</td>
        <td></td>

        <td class="col_title">Telephone</td>

        <td></td>
    </tr>

    <?php
    foreach($customers as $i => $customer) // foreach is easier to use, manage and troubleshoot
    {
    ?>
    <tr>
        <td colspan="2" class="cus_col_1"><a href="customer_details.php?id=<?php echo $customer['customer_id']; ?>"><?php echo $customer['surname'].', '.$customer['first_name']; ?></a></td>
        <td colspan="2" class="cus_col_2"><?php echo $customer['town']; ?></td>
        <td class="cus_col_1"><?php echo $customer['telephone']; ?></td>

        <td class="cus_col_2">
            <a href="javascript: single_execute('prc/customers.prc.php?delete=yes&id=<?php echo $customer['customer_id']; ?>')" onClick="return confirmdel();" class="btn_maroon_small" style="margin: 0px; float: right; margin-right: 10px;"><div class="btn_maroon_small_left">
                <div class="btn_maroon_small_right">Delete Account</div>
            </div></a>
            <a href="customer_edit.php?id=<?php echo $customer['customer_id']; ?>" class="btn_black" style="margin: 0px; float: right; margin-right: 10px;"><div class="btn_black_left">
                <div class="btn_black_right">Edit Account</div>
            </div></a>
            <a href="mailto: <?php echo $customer['email']; ?>" class="btn_black" style="margin: 0px; float: right; margin-right: 10px;"><div class="btn_black_left">
                <div class="btn_black_right">Email Customer</div>
            </div></a>
        </td>
    </tr>
    <tr><td class="col_divider" colspan="6"></td></tr>
    <?php
    }
    ?>
</table>

<!--///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////-->
<!--// PAGINATION-->
<!--///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////-->
<div class="pagination_holder">

<?php
if($page != 1)
{
    $pageprev = $page-1;
?>
    <a href="javascript: change('<?php echo $area; ?>', '<?php echo $prc; ?>?page=<?php echo $pageprev; ?>');" class="pagination_left">Previous</a>
<?php
}
else
{
?>
    <div class="pagination_left, page_grey">Previous</div>
<?php
}
?>
<div class="pagination_middle">
<?php
$numofpages = $totalrows / $limit;
if(($totalrows % $limit) != 0) // using this avoids the second for-loop (avoids redundant code)
  $numofpages++;

for($i = 1; $i <= $numofpages; $i++)
{
    if($i == $page)
    {
    ?>
        <div class="page_number_selected"><?php echo $i; ?></div>
    <?php
    }
    else
    {
    ?>
        <a href="javascript: change('<?php echo $area; ?>', '<?php echo $prc; ?>?page=<?php echo $i; ?>');" class="page_number"><?php echo $i; ?></a>
    <?php
    }
}

?>
</div>
<?php
if(($totalrows - ($limit * $page)) > 0)
{
    $pagenext = $page+1;
?>
    <a href="javascript: change('<?php echo $area; ?>', '<?php echo $prc; ?>?page=<?php echo $pagenext; ?>');" class="pagination_right">Next</a>
<?php
}
else
{
?>
    <div class="pagination_right, page_grey">Next</div>
<?php
}
?>

</div>
<!--///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////-->
<!--// END PAGINATION-->
<!--///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////-->
Weboide