<?php
function moderateTopic($topic_id, $action = NULL) {
$locked_query = "SELECT topic_id FROM forum_topics WHERE status = 'locked' AND topic_id = '{$topic_id}'";
$locked_count = totalResults($locked_query);
$announcement_query = "SELECT topic_id FROM forum_topics WHERE topic_type = 2 AND topic_id = '{$topic_id}'";
$announcement_count = totalResults($announcement_query);
$sticky_query = "SELECT topic_id FROM forum_topics WHERE topic_type = 3 AND topic_id = '{$topic_id}'";
$sticky_count = totalResults($sticky_query);
if (is_null($action) == FALSE) {
switch ($action) {
case 1:
if ($locked_count > 0) {
$topic_query = "UPDATE forum_topics SET status = 'unlocked' WHERE topic_id = '{$topic_id}'";
} else {
$topic_query = "UPDATE forum_topics SET status = 'locked' WHERE topic_id = '{$topic_id}'";
}
doQuery($topic_query);
break;
case 2:
if ($announcement_count > 0) {
$topic_query = "UPDATE forum_topics SET topic_type = 1 WHERE topic_id = '{$topic_id}'";
} else {
$topic_query = "UPDATE forum_topics SET topic_type = 2 WHERE topic_id = '{$topic_id}'";
}
doQuery($topic_query);
break;
case 3:
if ($sticky_count > 0) {
$topic_query = "UPDATE forum_topics SET topic_type = 1 WHERE topic_id = '{$topic_id}'";
} else {
$topic_query = "UPDATE forum_topics SET topic_type = 3 WHERE topic_id = '{$topic_id}'";
}
doQuery($topic_query);
break;
case 4:
header('Location: ' . urlify(9, $topic_id));
break;
case 5:
header('Location: ' . urlify(11, $topic_id));
break;
}
header('Location: ' . urlify(2, $topic_id));
} else {
$locked = $locked_count > 0 ? 'Unlock' : 'Lock';
$announcement = $announcement_count > 0 ? 'Unannounce' : 'Announce';
$sticky = $sticky_count > 0 ? 'Unsticky' : 'Sticky';
return <<<EOT
<div style="float: right;">
<form method="POST">
<select name="action" onChange="document.forms[0].submit();">
<option value="">- - Moderate - -</option>
<option value="1"> => {$locked}</option>
<option value="2"> => {$announcement}</option>
<option value="3"> => {$sticky}</option>
<option value="4"> => Move</option>
<option value="5"> => Delete</option>
</select>
</form>
</div>
EOT;
}
}
?>
Refactorings
No refactoring yet !
Ants
December 19, 2010, December 19, 2010 10:02, permalink
A couple of things that need improvement:
1) Fix the SQL injection vulnerability with topic_id, please! If you are going to only do one change, please at least fix this. Remember this cartoon: http://xkcd.com/327/
2) I'm assuming that totalResults() performs the query and counts the number of rows returned. Why not use "SELECT COUNT() ..." instead which makes the database do the work of counting instead of sending the results back totalResults() which then does the counting?
3) You are doing 3 database queries always, but if action is not null, you'll only ever need 1 of those 3 queries. You've just wasted processing time. It'll be better to make those 3 queries into 3 helper functions that you call you need them.
4) There is a lot of duplicated strings for queries and for updates. These can be isolated down to about 4 helper functions: 2 helper queries and 2 helper updates.
5) There is a race condition if moderateTopic() is called with the same topic_id and action in response to a post from two different browsers at nearly the same time. Let's say two browsers go try to to to lock a topic. The first moderateTopic() call to get in will go lock the topic correctly, but the second moderateTopic() call will very quickly unlock it because it will discover that that topic is locked, and your current logic is to always just flip the state.
Was wondering if the following function can be improved/simplified? (its a function which controls moderation functionality)