For your "logged in" checks, something like this would be better:
if (!isloggedin()) {
header('Location: login.php');
exit();
}
The reason for this is your current model would continue executing all the code after your else statement if the header redirect failed. It could fail for a number of reasons, usually stemming from output in code (like your isloggedin() method) that is called before the header redirect. It's best practice, then, to exit() your code after a header redirect.
A small performance issue is the quotes you use. When PHP encounters a string enclosed with double-quotes, it's going to tell the interpreter to look inside the double-quotes for any items it can interpret, like variable names. If you aren't using any inline interpretation that requires double-quotes, stick with single-quotes and concatenation. For the large SQL query you use at the start of the script, use single quotes and the . operator instead of double-quotes and newlines. It'll speed your script execution up a tiny bit and if it's being called very often, every little bit helps.
I don't know what sql_error() in your code actually does, as it looks like a user-defined function, but if it's dying and throwing raw SQL errors to your end users, that can be a security concern in that raw SQL errors often contain usernames, table structures, and whether your account is using a password or not. This information can be used to make an attack against your database more effective, so you should double check that - try and die() gracefully without exposing core information to the visitor. You can't ever assume an error won't happen or that a user won't intentionally trigger one looking for information.
while($row = mysql_fetch_array($result))
{
$c1 = $row['points1'];
$c2 = $row['points2'];
$c3 = $row['points3'];
$c4 = $row['points4'];
$stamina = $row['stamina'];
}
Two things here - you're in a while loop, but you're assigning directly with =, not concatenating with .=, so if you have more than one result being returned, $c1, $c2, $c3, $c4, and $stamina will only have the last row's worth of information in them. If you're only handling one row worth of data, don't use a while, just do a $row = mysql_fetch_array(). The other issue is that you're duplicating memory - if you've only got the one row, you're copying $row['points1'] into $c1 and now have two copies of that variable in memory. If you just want to assign $row['points1'] to a name that's easier for you to use and it doesn't matter if you modify $row['points1'] itself, you can pass the variable by reference, which prevents copying the data and essentially gives you two names to the same data:
$c1 = &$row['points1']; // the & operator tells PHP to pass $row['points1'] by reference
if($c1 != 0) { $c1 = $c1 + $default; }
if($c2 != 0) { $c2 = $c2 + $default; }
if($c3 != 0) { $c3 = $c3 + $default; }
if($c4 != 0) { $c4 = $c4 + $default; }
Here, you should be able to do this, which makes it a little easier to edit/modify in the future:
if ($c1 != 0) $c1 += $default;
The { } braces aren't required for single-line statements like that and just clutter it up, in my opinion, but that's a stylistic preference. The += operator adds $c1 and $default, then assigns the result of the addition back into $c1, functionally equivalent to yours but less typing and a little cleaner, to me.
Gotta go to class. Hope that helped.
