/trunk/scripts/split_ia_score.php (Diff revision 2)

This should probably have a column for deviation and a column for rating.

So it should be something like user_id, round_id, deviation, score.

/trunk/scripts/split_ia_score.php (Diff revision 2)

spaces.

/trunk/scripts/split_ia_score.php (Diff revision 2)

This should be removed when commiting

This is a big patch for an important change. Congrats for writing it!
We need an additional reviewer, maybe Mircea or Bogdan.

Unfortunately, this is only a PARTIAL review. I'm about halfway through, I need more time to finish it. Nevertheless, you can have a look at it anyway.

/trunk/common/db/db.php (Diff revision 21)

{ bracket coding style

/trunk/common/db/round.php (Diff revision 21)

$score_name is not _really_ used any more. Shouldn't it be removed?

/trunk/common/db/round.php (Diff revision 21)

Ev space

/trunk/common/db/round.php (Diff revision 21)

LEFT JOIN ON criteria should be based on values found in the table rows, not 'constants' (values you got from the user). Put those in the WHERE clause.


Also, make sure you have an index for (round_id, task_id)

1. 

   Savin Tiberiu May 5, 2009, 7:20 a.m.

   You sure this works? If I move those to the where clause, when I get the list of tasks for a round, tasks for which the user hasn't submited any solution won't be shown (there are no records in ia_score_user_round_task).

/trunk/common/db/round.php (Diff revision 21)

Do we have an index on the (user_id, round_id, task_id) tuple in table ia_score_user_round_task?

/trunk/common/db/round.php (Diff revision 21)

score.round_id = round_task.round_id

/trunk/common/db/round.php (Diff revision 21)

Evil space

/trunk/common/db/round.php (Diff revision 21)

Put this in the WHERE clause

/trunk/common/db/score.php (Diff revision 21)

Break this into

score_update($score, $user_id, $task_id, $round_id)

and

score_update_rating($rating, $deviation, $user_id, $task_id, $round_id)


We need to get rid of 'score name'

/trunk/common/db/score.php (Diff revision 21)

... this won't be needed anymore

/trunk/common/db/score.php (Diff revision 21)

I would prefer sprintf, since we're doing it everywhere else

/trunk/common/db/score.php (Diff revision 21)

db_query_value() does exactly this

/trunk/common/db/score.php (Diff revision 21)

You can do a single 'INSERT INTO  ... SELECT ... FROM' query instead of doing two separate ones

/trunk/common/db/score.php (Diff revision 21)

$score_name not needed?

/trunk/common/db/score.php (Diff revision 21)

Please write a FIXME: message

/trunk/common/db/score.php (Diff revision 21)

FIXME: message

/trunk/common/db/score.php (Diff revision 21)

Use PHP booleans,  false, not 'false'

/trunk/common/db/score.php (Diff revision 21)

Ev space

/trunk/common/db/score.php (Diff revision 21)

Use PHP booleans

/trunk/common/db/score.php (Diff revision 21)

true

/trunk/common/db/score.php (Diff revision 21)

Ev space

This is not a full review. Maybe if we do small little changes, we'll get more motivated :)

Regarding coding style, I noticed there are a lot of tabs instead of spaces used and spaces at the end of lines. You can just replace them fast with vim so I don't have to point them out :)

/trunk/common/db/db.php (Diff revision 22)

implode(", ", array_map(db_quote, $array)) is shorter. Using db_quote is better because it handles all types of data (for example null is NULL, not 'NULL')

/trunk/common/db/db.php (Diff revision 22)

Too many new lines :)

/trunk/common/db/round.php (Diff revision 22)

The score_name parameter here was used to not fetch the scores from the database if not needed (although it was badly used :)). Maybe this functionality should be kept (just renamed into $fetch_scores). If it is false, don't join with ia_score_*. If you do add it back make sure to add it back to the tasks macro also :)

/trunk/common/db/round.php (Diff revision 22)

Why do you fetch round title here? This would cause an extra join whenever somebody would open the "arhiva" page for example.

The round title would only be needed for the "detailed rankings" page and could just be fetched by fetching the rounds in a separate query. (rounds are also cached afaik, so it would be fast :))

/trunk/common/db/round.php (Diff revision 22)

Evil space

/trunk/common/db/round.php (Diff revision 22)

Use spaces instead of tabs and fix whitespace at the end of line

/trunk/common/db/round.php (Diff revision 22)

It shouldn't be '%s' here, it should be %s. It's a condition in a WHERE clause. I'm surprised it worked for you like this.

I also don't understand why it's been db_escaped until now. From what I can tell, db_get_task_filter_clause is meant to return something like `score`.`score` = 100, so escaping should be removed from here.

/trunk/common/db/score.php (Diff revision 22)

Use better comments :)

score_update_rating updates a rating, while score_update updates a task score.

/trunk/common/db/score.php (Diff revision 22)

Fix your tabs.

/trunk/common/db/score.php (Diff revision 22)

Remove whitespace at the end of line and add "{" and a comment

/trunk/common/db/score.php (Diff revision 22)

Find and remove is_score_name and all appearances of it in the code. :)

Run "find -type f | xargs grep is_score_name" to find them all.

/trunk/common/db/score.php (Diff revision 22)

user, task and round should not be allowed to be null anymore in the new table scheme.

/trunk/common/db/score.php (Diff revision 22)

Change tab into spaces.

/trunk/common/db/score.php (Diff revision 22)

Remove whitespace at the end of the (4) lines.

/trunk/common/db/score.php (Diff revision 22)

This function is not used anywhere in the code and can be safely removed. It would have been useful before this code change since it returned multiple types of scores.

Not a complete review, just a few things.

/trunk/common/db/round.php (Diff revision 25)

The previous default was to not fetch scores. Better keep it that way. :)

/trunk/common/db/round.php (Diff revision 25)

Don't force "false" for $fetch_scores, from what I see it's being called with null in macro_tasks. !$fetch_scores is ok.

/trunk/common/db/round.php (Diff revision 25)

filter_clause is a SQL condition, don't add 's

/trunk/common/db/score.php (Diff revision 25)

return db_affected_rows() 

/trunk/common/db/score.php (Diff revision 25)

// Update :)

/trunk/common/db/score.php (Diff revision 25)

Remove $groupby parameter.

/trunk/common/db/score.php (Diff revision 25)

Isn't it supposed to be 'deviation'?

/trunk/common/db/score.php (Diff revision 25)

Is this necessary now?

/trunk/common/db/task.php (Diff revision 25)

Add a function in common/db/round.php to recompute scores for a certain round.

/trunk/common/db/task.php (Diff revision 25)

Why don't you query just ia_score_user_round_task and group by user_id ?

/trunk/scripts/db-strip (Diff revision 25)

Ugly spaces and tabs.

/trunk/www/macros/macro_rankings.php (Diff revision 25)

Remove extra 'user_id' parameter (that was the groupby parameter that is no longer needed).

Not a complete review, just a few things.

Almost done with the review, only the macro left. I'll wait until after you update the diff though. :)

/trunk/common/db/score.php (Diff revision 25)

"SELECT

/trunk/common/db/score.php (Diff revision 25)

db_quote($start), db_quote($count)

/trunk/common/db/score.php (Diff revision 25)

Do you really need to rebuild this? score_build_where_clauses executes an extra query to determine which rounds have public_eval. If so, maybe you can move that query somewhere else (closer to the controller where it is supposed to be)

/trunk/common/db/score.php (Diff revision 25)

Please add more comments inside the whole method. For someone that doesn't know the code well, this part is not really clear :)

/trunk/common/db/score.php (Diff revision 25)

Doesn't getattr($task_score[$user_id], $task_id, 0) work?

/trunk/common/db/score.php (Diff revision 25)

getattr($round_scores[$user_id], $round_id, 0)

/trunk/common/db/score.php (Diff revision 25)

Add a comment for what this does.

/trunk/scripts/update-ratings (Diff revision 25)

Uncomment this and remove score_get_range?

/trunk/www/format/format.php (Diff revision 25)

Revert this file.

/trunk/www/macros/macro_rankings.php (Diff revision 25)

Replace tabs with spaces

/trunk/common/db/score.php (Diff revision 25)

Your'e pushing it :)

/trunk/common/db/score.php (Diff revision 25)

It won't work because $task_score[$user_id] may not be set.

/trunk/common/db/score.php (Diff revision 25)

Same comment as for tasks.

Seems fine. :) I'll "Ship It" once I test it at home.

/trunk/common/db/round.php (Diff revision 26)

Are you sure this function gets called with $fetch_scores === false in the tasks macro and not with $fetch_scores === null?

That's what I was referring to in my previous review: why the "==="?

/trunk/scripts/db-strip (Diff revision 26)

Use round_recompute_score.

/trunk/scripts/db-strip (Diff revision 26)

Three empty lines here, only one needed.