matuck [matuck]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

What is the difference between ranks, and roles? Why would a site use ranks when Drupal already uses roles?

Given that Kiam is pretty much the only one doing it and the queue is long... It takes a while. I tried helping out but I suck at code reviews. :( If you're any good at doing code reviews, you could always help out and move things along faster. :)

Michelle

I know it's not much consolation but this is most definitely a case of "It's not you, it's us". See #703116: Our CVS account application requirements are obtuse and discourage contributions

Michelle

1.

$result = db_query(sprintf('SELECT rr.roster_id, rr.uid, UNIX_TIMESTAMP(rr.date)  FROM {ranks_roster} rr WHERE rr.rank_id=\'%s\' ORDER BY rr.date', $rank_id));

// Why not only

$result = db_query("SELECT rr.roster_id, rr.uid, UNIX_TIMESTAMP(rr.date)  FROM {ranks_roster} rr WHERE rr.rank_id='%s' ORDER BY rr.date", $rank_id);

2. This is wrong! Don't put variables in t like this.

  drupal_set_message(t(ranks_getUserName($form_values['values']['adduser']) .' was added.'));

  // use like this instead
  drupal_set_message(t('@user was added.', array('@user' => ranks_getUserName($form_values['values']['adduser']))));

3. Use check_plain() on variables that is user inputs.

$output .= "<table class=\"indranks\"><tr><td width=\"33.3%\">". ranks_getUserName($cur_user['uid']) ."</td><td width=\"33.3%\">$cur_user[date]</td><td width=\"33.3%\" align=\"right\">$thisrank[shortname]</td></tr></table>";

4. Use http://drupal.org/project/coder and check your code. I have not done that, but by only looking at your code there is some faults if you will following http://drupal.org/coding-standards

On your question, in your ex with a form "object" you don't need to use check_plain on #default_value.

example from http://drupal.org/node/28984

$form['bad'] = array(
'#type' => 'textfield',
'#default_value' => check_plain($u_supplied),  // bad: escaped twice
'#description' => t("Old data: !data", array('!data' => $u_supplied)), // XSS
);

$form['good'] = array(
'#type' => 'textfield',
'#default_value' => $u_supplied,
'#description' => t("Old data: @data", array('@data' => $u_supplied)),
);

- - - - -

1. Why use escape \ here, when you can use " " around the query and have ' ' inside?

$result = db_query('SELECT rr.roster_id, rr.uid, UNIX_TIMESTAMP(rr.date)  FROM {ranks_roster} rr WHERE rr.rank_id=\'%s\' ORDER BY rr.date', $rank_id);

// as this
$result = db_query("SELECT rr.roster_id, rr.uid, UNIX_TIMESTAMP(rr.date)  FROM {ranks_roster} rr WHERE rr.rank_id='%s' ORDER BY rr.date", $rank_id);

2. Why do you use " " sometimes and sometimes ' ' on SQL querys? Why not use " " all the time?

3. Another tips, not to use escape chars so much.

$output = "<div class=\"rankspageheader\">";

// do like this
$output = '<div class="rankspageheader">';

4. You might want to use theme('image', ...) here instead and on other places that you have img. See http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_image/6

$output .= "<div class=\"rankspageheaderimage\"><img src=\"" . ranks_imagepath('ranks.gif') . " \"></div>\n";

5. There is a theme('table', ...) for making tables, see http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_table/6

$output .= "<table class=\"indranks\"><tr><td>" . t('No one in this rank') . "</td></tr></table>";

6. Not secure!

db_query('DELETE FROM {ranks} WHERE rid = ' . $key);

// do like this
db_query("DELETE FROM {ranks} WHERE rid = %d", $key);

7. Why "rr", makes no seens.

db_query('SELECT rr.uid, rr.name  FROM {users} rr ORDER BY rr.name');

// Also, when only geting data from one table you can write like this. But what you did was not wrong, you were only very clear about it.
db_query('SELECT uid, name  FROM {users} ORDER BY name');

8. Why have a if and else, when you can change the if statment and remove the else.

  if ($users === FALSE) {
    //do nothing
  }
  else {

- - - - -

• The points reported in this review are not in order of importance / relevance.
• Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.

Some tips.

1. Why do you add this to almost all menu items? It's the same as the default so you could save some lines of code.

  'type' => MENU_NORMAL_ITEM,

2. Indenting is bad on some places, it gets easier to read if you do it little better.

  // ex.
  $form['sorting'] = array('#type' => 'hidden',
              '#value' => $sorting);

- - -
Errors

1.

  theme_image(ranks_imagepath('ranks.gif'))
  // should be, so it can be overrided
  theme('image', ranks_imagepath('ranks.gif'))

2.

  drupal_set_message(t('Delete user ') . check_plain(ranks_getUserName($uid)));
 
  // alternative, when using @ the variable will automatic passed through check_plain()
  drupal_set_message(t('Delete user @user', array('@user' => ranks_getUserName($uid)));

3. Use quotes around a string literal array index, this is not only a style issue, but a known performance problem

    $output .= '<div class="ranksheader"><div class="ranksheadername">' . $thisrank[name] . '</div>';

Since matuck has shown great patience and tenacity through this process, I went ahead and approved the account. If Scyther wants to keep reviewing and get all the kinks out, that would be great. In the mean time, matuck now has access to set up an account and get the code into CVS so he can make use of versioning and the issue queue. I'll leave this at needs review so I don't prematurely stop the great reviewing that's going on here but you folks can marked this as "fixed" whenever you're ready.

Michelle

I can give review on the module later on the project page. If you are intressted in a co-maintainer matuck, send me a message through my contact form.

I am changing the status as the CVS account has been already approved.