Have I done anything wrong?

[Noximity]

Hey guys I've made this simple php code to fetch stuff for my website I'm pretty sure that I've done nothing wrong can anyone see any mistakes?

<?php
if($showclosedgames==1){$query1="SELECT * FROM games";}
elseif($showclosedgames==0){$query1="SELECT * FROM games WHERE winner=''";}
$record_count = $conn->query($query1);
$per_page= $limit;
$pages = ceil($record_count->num_rows / $per_page);
if(!isset($_GET['page']))
{
$page = 1;
}
else
{
$page = $_GET['page'];
}
if($page <= 0)
{
$start = 1;
}
else
{
$start = $page * $per_page - $per_page;
}
$prev = $page -1;
$next = $page +1;


if($showclosedgames==1){$query2="SELECT * FROM games ORDER BY amount DESC LIMIT $start, $per_page";}
elseif($showclosedgames==0){$query2="SELECT * FROM games WHERE winner='' ORDER BY amount DESC LIMIT $start, $per_page";}
$query = $query2;


if ($result = $conn->query($query))
{
while ($row = $result->fetch_assoc())
{
if(empty($row['winner'])){$status='open';}
elseif(!empty($row['winner'])){$status='closed';}
?>

[ginerjm]

What makes you think anything's wrong? Try turning on PHP error checking while developing. See my signature.

 

See my comments in code below:

if($showclosedgames==1)
{
$query1="SELECT * FROM games";
}
elseif($showclosedgames==0) // why this check???
{
$query1="SELECT * FROM games WHERE winner=''";
}
$record_count = $conn->query($query1); // why is this called record_count???
$per_page= $limit;
$pages = ceil($record_count->num_rows / $per_page);
if(!isset($_GET['page']))
{
$page = 1;
}
else
{
$page = $_GET['page'];
}
if($page <= 0)
{
$start = 1;
}
else
{
$start = $page * $per_page - $per_page;
}
$prev = $page -1;
$next = $page +1;
//
if($showclosedgames==1)
{
$query2="SELECT * FROM games ORDER BY amount DESC LIMIT $start, $per_page";
}
elseif($showclosedgames==0)
{
$query2="SELECT * FROM games WHERE winner='' ORDER BY amount DESC LIMIT $start, $per_page";
}
$query = $query2;
//
if ($result = $conn->query($query))
{
while ($row = $result->fetch_assoc())
{
if(empty($row['winner']))
{
$status='open';
}
elseif(!empty($row['winner']))
{
$status='closed';
}
// WHERE'S THE REST OF THIS BLOCK???

Edited December 25, 2016 by ginerjm

[mac_gyver]

Have I done anything wrong?

 

 

actually, yes. there are a number of programming practices and missing logic, that your code needs.

 

1) $showclosedgames - don't write variable names strung together like this. use underscores to separate the words that make up a variable name - $show_closed_games

 

2) if there are only two possible values for a variable, don't use an elseif() test, just use an else() or you can use a Ternary Operator.

 

3) don't use variables that end in 1, 2, ... For the posted code, you are writing out more variables than you need and then having more lines of code copying the extra variables to other variables. you should finish with the operations within one block of code before going on to the next block of code. you can then reuse common variable names like $query.

 

4) don't copy one variable to another. just use the original variable name in the code.

 

5) don't repeat yourself (DRY.) the parts of the sql query statement that are static should not be repeated. only the WHERE clause is dynamic/conditional and it is the only thing that should be part of the conditional logic. you should build the WHERE clause in a php variable, then use that variable in all the sql query statements. there will only be two sql query statements - 1) to get a count of the matching rows, and 2) to retrieve the actual data.

 

6) the sql query to get a count of the matching rows should use SELECT COUNT(*) so that it doesn't retrieve all the data from the table. you would fetch the count value into a php variable.

 

7) the purpose of getting the count of the matching rows is so that you can limit the maximum requested page number and you limit or determine if there is a next link in the pagination. the current code is not doing either of these things.

 

 if the requested page is 1, there is no previous link for the pagination. the current code is unconditionally setting up a value for a previous link.

 

9) the whole block of logic for the page value and sql query limiting needs help. you should get the requested page number (validating or casting it as an integer) or use page 1 as the default, then limit the page number between 1 and the maximum number of pages, then just use the final limited requested page number in the rest of the code.

 

10) the sql query to fetch the actual data has some error checking logic around it (which the end of wasn't included in the post, so we don't know what your code does do when there is a query error), but the first sql query to get a count of the matching rows doesn't have any error checking logic. consistency counts in programming. you should always have error checking/handling logic in your code. the easiest and most consistent way of handling sql statement errors is to use exceptions.

 

11) i have doubts about your $showopengames ($show_open_games), WHERE winner='' sql, and the $status open/closed logic. when $showopengames == 1, your queries are matching all the rows in the games table. when $showopengames == 0 (presumably to show only closed games), your queries are matching rows in the games table that have an empty winner value (which seems to mean games that are open.) you are then treating an empty winner value as being open and a non-empty winner value as being closed. this makes no sense. are there actually three possibilities for $showopengames - all, open, and closed, in which case the variable is not named very well, and program and query logic isn't doing what you expect?

 

most of these points will actually simplify and reduce the amount of php and query logic, making it easier to write, test, and correct problems in your code and queries.

Edited December 25, 2016 by mac_gyver