Bastlynn [bastlynn]

Per the CVS application requirements, can you expand on your modules and its features more before someone reviews it. Thank you.

I'll take a look at this a bit later today - I've been playing RPGs (table-top and live - can't be bothered with computer RPGs) for over 30 years :-)

Anyway I'll gie it a review during my lunchbreak.

Well you certainly cleaned it up as far as "coder" is concerned since it reported nothing. But you can't get away scott free :-)

We're always keen to keep the code footprint as small as possible, so putting the settings form into a separate file (using 'file' => 'dice_roller.admin.inc' in hook_menu) would be good. Likewise you could put the dice rolling code (which is a fair chunk) in a separate file and just load it when you need to evaluate a roll which would be far less often than just displaying a previously calculated result.

In the same vein, your massive amount of help (a good thing, obviously) is not going to be needed most of the time - can you change it to, say, theme('dice_roller_help') and then that can go in a separate file as well.

And on the subject of themes: your _dice_roller_get_roll() function should be a theme - so another developer/administrator can display it a different way if they want to.

Minor issue, and not critical since it works fine, in your hook_nodeapi you check for !$teaser on every IF, wouldn't it be more efficient to just check that first and exit if TRUE? Also using 'elseif' would have a slightly quicker execution time :-) since only one op is going to be true at any given entry to the function. (Personally I prefer 'switch' - but that's just me.)

You might also want to take a look at hook_content_extra_fields, this allows you to integrate your fields with the "CCK Manage fields" pages so administrators can decide where in the sequence of fields your ones appear.

Hope that helps.

I almost wrote about having an API. However you were asking for approval on this code, so that's what I reviewed. Once you have the CVS account you can do all the changes you want without anyone interfering :-)

Nothing from me - but I can't do anything else.

I can finish this up. You are definitely in the right direction and getting close.

The following points are just a start and do not encompass all of the changes that may be necessary for your approval. Also, a specific point may just be an example and may apply in other places.

1. Your README.txt suggestions versions which is unnecessary as you have not realized the module yet. Also, your README.txt is more of a changelog. I would suggest removing the INSTALL.txt and just having a README.txt, then when new releases happen add a CHANGELOG.txt
2. Follow Drupal Coding Standards. Note that Drupal coding standards are more than just PHP code. Also, look at using Coder to help automate SOME of this. Examples (there are many):
   

           'description' => 'The primary identifier for a roll.',
           'type' => 'serial',
           'unsigned' => TRUE,
           'not null' => TRUE),
   

   You need proper docblocks for all functions and files.

3. /**
    * Implementation of hook_form_alter(), add the number input to both comment and form
    */
   function dice_roller_menu() {
   

   This is not accurate.

4.     return $form; /* Exit if this user can't use the dice roller */
   

   No need to return the form as it is passed by reference.

5.   if (drupal_strlen($pid)==0) {
       return '';
     }
   

   Use empty() and is_string() to be more accurate of what you are looking for.

6.           include_once('dice_roller.roller.inc');
   

   Please utilise http://api.drupal.org/api/function/module_load_include

7.     $str_rolls .= '<div class="dice_roll">I rolled '. $roll_command .', the result is '. $roll_results[$index] .'</div>';
   

   Utilize t()

8. The help tpl does not use t().

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article.

Hey @Bastlynn, thanks for your patience and understanding; you are actually one of the more understanding. We don't try to favor any application over the other, but it is all volunteer, so there probably is some sort of favoritism going on. Nonetheless, we look forward to your updates.

RPG package - what else is in there on the modules page? Is there a package of other similar RPG modules you could change it to or should it just be in Other?

Menu heading on admin/settings shouldn't say Configure and should have description. Note everything else is just a name because you're already in a configuration area.

Most of the headings on links like on admin/settings are ucfirst() format. Is it intentional that Dice Roller differs from that for some reason?

Since the only configuation for dice roller is what content types it should display on, those settings might fit better on the content type configuration forms rather than having to find them in a separate area. If you use hook_form_alter() to insert the activation for node_type_form, you could have a dice roller activated there. See the http://drupal.org/project/submitagain module for an example. If you move it to the content type configuration, then you can entirely remove the menu callback and configuration form, of course.

When I activate the module on node/add/page, again there's a ucfirst() issue, but there's also a complete lack of descriptions on the form fields. I have no idea what to type into the roll box, why I would make a note, and the help box is blank. If you could get the instructions clearly into one sentence, then the Help fieldset might not even be necessary, but if you keep it, it should have content. Afterall, even if I had read the README first, the site admin knowing what it does doesn't tell the users how to use it.

The first time I saved the node, nothing happened. That was expected behavior. Then I edited the node and I didn't know what to type, so I rolled "133432dd". As a result, I got:

* warning: implode(): Invalid arguments passed in /var/www/d6/sites/all/modules/dice_roller/dice_roller.roller.inc on line 25.
* warning: htmlspecialchars() expects parameter 1 to be string, array given in /var/www/d6/includes/bootstrap.inc on line 840.

The View tab lists:

This is a note.
I rolled 133432dd, the result is .

However, when I go back to edit the node again, I set the roll to "a" and got just

* warning: htmlspecialchars() expects parameter 1 to be string, array given in /var/www/d6/includes/bootstrap.inc on line 840.

Nothing changed on the View tab; it retains the 133432dd roll. Thinking perhaps I needed an integer, I switched tactics and to roll "100000000000":

* warning: implode(): Invalid arguments passed in /var/www/d6/sites/all/modules/dice_roller/dice_roller.roller.inc on line 25.
* warning: htmlspecialchars() expects parameter 1 to be string, array given in /var/www/d6/includes/bootstrap.inc on line 840.
* warning: htmlspecialchars() expects parameter 1 to be string, array given in /var/www/d6/includes/bootstrap.inc on line 840.

"I rolled 100000000000, the result is ." Numeric and alpha are both broken, so I'm not sure what to do from here.

On the code side, your docblock spacing is off in your section of database related functions. Each line of the docblock should have just one space before the asterisk. I saw this in .module and .inc.

There should be a space between the "Implementation of" and the comment of what the function does. re: hook_comment.

hook_content_extra_fields (). shouldn't have a space before the paren.

In your README, you list specific URLs for most locations, but then leave "on the admin page" for how to actually turn it on.

There are specific accepted examples for how a README should be written, things it should contain, and how it should be formatted. Morbus' style to match core is probably best. Same area for documentation for when you add descriptions to your node fields. http://drupal.org/node/632262

English pickyness in your .info file: "Provides an integrated, tamper-proof dice roller for nodes and comments."

Line 245 of dice_roller.module should be indented.

Many of your docblock sentences don't have periods on the end and should.

There's no point in having an empty default case in functions like your nodeapi implementation.

Space out your if statements, like at the bottom of dice_roller.roller.inc, drupal_strlen($extras) > 0

Line 311 of dice_roller.roller.inc has an extra space in implode(', ', $arr_rolls );.

Make your if statement spacing consistent. Line 253 of dice_roller.roller.inc has extra space in the paren where if ( $extra >= $hits ) should probably have those removed by the parentheses.

Add spaces on line 215 for ($i=0;$i<$number;$i++)

Maybe I just didn't analyze it close enough, but what's the point of the module_exists() in dice_roller_content_extra_fields()? In normal circumstances, that function is used to plugin to other modules if they're installed, not to check on the status of the very same module that's running.

Any reason for the extra space at the end of the query in _dice_roller_set_origin() or _dice_roller_get_roll()?

I saw multiple cases in dice_roller.roller.inc where variables were used like $func, $arr_funcs, lpara, $ctrl, and $err. As a general rule, don't abbreviate variable names. Spell out their usefulness so they can be self-documenting without extra mental translation.

dice-roller-basic-help.tpl.php says strings are copied versions of copyrighted text. Do you have written permission to redistribute under the GPL?

I was unclear about the docblocks. There should be a blank newline after the initial single line description, not a space. I don't see that misunderstanding as a good reason to keep this from being approved. Look for your approval email shortly.

When trying to save roll on drupal 6.x

Parse error: syntax error, unexpected ';' in /home/szaijan/public_html/avatar/modules/dice_roller/dice_roller.roller.inc on line 27

Suggestion:
Help or readme should clarify relationship to other comment fields and also any relationship to integrated rich text editor (which was on by default in my dice roll text entry area, but turning on or off had no error effect).

happy to provide additional information.

mm status should not be fixed on the report.