HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Ranking and storing scores to a database

Submitted by: @import:stackexchange-codereview··
0
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 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

  • 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 as return 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

  • gradeScoreFormula is quite specific for such a generic function, yet it doesn't really tell me anything about what the function actually does. isLargerThan might be better. [Although the function seems all wrong anyways. All the sort functions expect the function to return an integer; in that case, you could simply call the function compare]



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 = $v seems 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 position and a? 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 use i + 1 directly 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 the key function.



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.