patternphpMinor
Ranking and storing scores to a database
Viewed 0 times
anddatabasescoresrankingstoring
Problem
I am developing a small website with PHP and am trying to receive user exam scores from the database. I rank it based on the highest score and then store the ranking to the database based on the
I have finished the code but it's too long. I need optimizations and reviews.
``
) ENGINE=InnoDB DEFAULT CHARSET=latin1 AUTO_INCREMENT=6 ;
//=========================functions=======================
function gradeScoreFormula($score1, $score2)
{
if ($score1 > $score2)
{
return true;
}else{
return false;
}
}
function gradeScore(array $values,$functionName, $dbc)
{
$newArr = array();
$dbc->autocommit(false);
uasort($values, $functionName);
$size = count($values);
foreach ($values as $k => $v)
{
$newArr[] = array($k => $v);
}
$reversedArr = array_reverse($newArr);
$complete = 0;
for($i=0; $i ';
$position = $a = $i + 1;
foreach($reversedArr[$i] as $k => $v)
{
//echo "UID = ".$k."----------> score = ".$v.'';
$q = "UPDATE USERS set subposition = $position WHERE uid = $k AND score = $v";
$r = $dbc->query($q);
if ($dbc->affected_rows)
{
$complete +=1;
}
}
}
if ($complete == $size)
{
$dbc->commit();
echo 'succesfully stored';
}else{
$dbc->rollback();
echo 'not stored try again';
}
$dbc->autocommit(true);
}
//===============================program start========================
$q1 = "SELECT uid, score FROM users";
$r1 = $dbc->query($q1);
$returnedVal = [];
while(list($id, $scores) = $r1->fetch_array())
{
$returnedVal[$id] = $scores;
}
print_r($returnedVal);
gradeScore($returnedVal,'gradeScoreFormula'
user_id returned from the database with the score.I have finished the code but it's too long. I need optimizations and reviews.
``
//=============init connection
$dbc = new mysqli('localhost','root','','test');
//==================database========================
CREATE TABLE IF NOT EXISTS users (
uid int(11) NOT NULL AUTO_INCREMENT,
user varchar(60) NOT NULL,
score int(11) NOT NULL,
subposition int(11) DEFAULT NULL,
PRIMARY KEY (uid`)) ENGINE=InnoDB DEFAULT CHARSET=latin1 AUTO_INCREMENT=6 ;
//=========================functions=======================
function gradeScoreFormula($score1, $score2)
{
if ($score1 > $score2)
{
return true;
}else{
return false;
}
}
function gradeScore(array $values,$functionName, $dbc)
{
$newArr = array();
$dbc->autocommit(false);
uasort($values, $functionName);
$size = count($values);
foreach ($values as $k => $v)
{
$newArr[] = array($k => $v);
}
$reversedArr = array_reverse($newArr);
$complete = 0;
for($i=0; $i ';
$position = $a = $i + 1;
foreach($reversedArr[$i] as $k => $v)
{
//echo "UID = ".$k."----------> score = ".$v.'';
$q = "UPDATE USERS set subposition = $position WHERE uid = $k AND score = $v";
$r = $dbc->query($q);
if ($dbc->affected_rows)
{
$complete +=1;
}
}
}
if ($complete == $size)
{
$dbc->commit();
echo 'succesfully stored';
}else{
$dbc->rollback();
echo 'not stored try again';
}
$dbc->autocommit(true);
}
//===============================program start========================
$q1 = "SELECT uid, score FROM users";
$r1 = $dbc->query($q1);
$returnedVal = [];
while(list($id, $scores) = $r1->fetch_array())
{
$returnedVal[$id] = $scores;
}
print_r($returnedVal);
gradeScore($returnedVal,'gradeScoreFormula'
Solution
Formatting
If we fix these issues, the first function for example would look like this:
Naming
Security
You should use prepared statements, as you may otherwise be vulnerable to second order SQL injection.
Even if the score and/or uid values in the users table are not currently user input, they may be in the future.
Additionally, prepared statements may increase performance in this case, as the query would only need to be prepared once.
Misc
Complexity
Your code is indeed overly complex, mainly because you perform unnecessary and complex transformations between data structures and because you are not using the correct functions.
The core of your program could be as simple as this:
- your indentation is off, making your code hard to read.
- it is customary to put the opening curly bracket on the same line as the statement it belongs to (except for functions and classes).
if (cond) return true; else return false;can be written asreturn cond.
- Your spacing is not consistent (use an IDE to fix this - and most other formatting issues - easily).
If we fix these issues, the first function for example would look like this:
function gradeScoreFormula($score1, $score2)
{
return $score1 > $score2;
}Naming
gradeScoreFormulais quite specific for such a generic function, yet it doesn't really tell me anything about what the function actually does.isLargerThanmight be better. [Although the function seems all wrong anyways. All thesortfunctions expect the function to return an integer; in that case, you could simply call the functioncompare]
Security
You should use prepared statements, as you may otherwise be vulnerable to second order SQL injection.
Even if the score and/or uid values in the users table are not currently user input, they may be in the future.
Additionally, prepared statements may increase performance in this case, as the query would only need to be prepared once.
Misc
WHERE uid = $k AND score = $vseems odd to me. Why are you checking for the score as well? If there is a reason, I would add a comment about it. If there is no reason, I would remove that check; the primary id should be enough, as far as I can tell.
- why have
positionanda? It doesn't seem necessary to have two variables here. For that matter, it doesn't seem necessary to have either one, you could simply usei + 1directly in the query.
- Your
foreach($reversedArr[$i] as $k => $v)is confusing and unnecessary. You only have it to access the key and value of a one-entry array. If you need the key, you can use thekeyfunction.
Complexity
Your code is indeed overly complex, mainly because you perform unnecessary and complex transformations between data structures and because you are not using the correct functions.
The core of your program could be as simple as this:
function gradeScore(array $values, $dbc)
{
arsort($values);
$position = 0;
$query = "UPDATE users set subposition = ? WHERE uid = ?";
$statement = $dbc->prepare($query);
foreach($values as $id => $score) {
$position++;
$statement->bind_param("ss", $position, $id);
$result = $statement->execute();
}
}Code Snippets
function gradeScoreFormula($score1, $score2)
{
return $score1 > $score2;
}function gradeScore(array $values, $dbc)
{
arsort($values);
$position = 0;
$query = "UPDATE users set subposition = ? WHERE uid = ?";
$statement = $dbc->prepare($query);
foreach($values as $id => $score) {
$position++;
$statement->bind_param("ss", $position, $id);
$result = $statement->execute();
}
}Context
StackExchange Code Review Q#106566, answer score: 4
Revisions (0)
No revisions yet.