Diceware

Hi

Looks like a nice little module.

I did a review. Please work on the following things:
- Add this git command to your application: git clone http://git.drupal.org/sandbox/migmedia/1434214.git dice ware The one you provide isn't really working.

The automated PAReview has found some issues:
Use this link if you want to re-run the test yourself: http://ventral.org/pareview/httpgitdrupalorgsandboxmigmedia1434214git

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• README.txt is missing, see the guidelines for in-project documentation.
• Remove the translations folder, translations are done on http://localize.drupal.org
• Remove all old CVS $Id tags, they are not needed anymore.
  

  <br />
  diceware.info:1:; $Id: example.info,v 1.0.0.0 2011/02/09 10:11:15 MyName Exp $<br />
  diceware.module:2:// $Id: diceware.module Exp $<br />
  translations/de.po:1:# $Id$<br />
  

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

</p>
<p>FILE: ...7-pareview/sites/all/modules/pareview_temp/test_candidate/diceware.info<br />
--------------------------------------------------------------------------------<br />
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)<br />
--------------------------------------------------------------------------------<br />
 6 | ERROR | It's only necessary to declare files[] if they declare a class or<br />
   |       | interface.<br />
 7 | ERROR | Files must end in a single new line character<br />
--------------------------------------------------------------------------------</p>
<p>FILE: ...areview/sites/all/modules/pareview_temp/test_candidate/diceware.install<br />
--------------------------------------------------------------------------------<br />
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)<br />
--------------------------------------------------------------------------------<br />
 2 | ERROR | Missing file doc comment<br />
 3 | ERROR | Missing function doc comment<br />
--------------------------------------------------------------------------------</p>
<p>FILE: ...pareview/sites/all/modules/pareview_temp/test_candidate/diceware.module<br />
--------------------------------------------------------------------------------<br />
FOUND 46 ERROR(S) AND 3 WARNING(S) AFFECTING 39 LINE(S)<br />
--------------------------------------------------------------------------------<br />
   2 | ERROR   | You must use "/**" style comments for a file comment<br />
   2 | ERROR   | There must be no blank line following an inline comment<br />
   5 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br />
     |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br />
   9 | ERROR   | Array indentation error, expected 4 spaces but found 6<br />
  10 | ERROR   | Array indentation error, expected 8 spaces but found 6<br />
  11 | ERROR   | Array closing indentation error, expected 6 spaces but found 4<br />
  17 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br />
     |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br />
  28 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br />
     |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br />
  41 | ERROR   | Missing function doc comment<br />
  44 | ERROR   | Array indentation error, expected 4 spaces but found 6<br />
  45 | ERROR   | Array indentation error, expected 4 spaces but found 6<br />
  46 | ERROR   | Array indentation error, expected 4 spaces but found 6<br />
  47 | ERROR   | Array indentation error, expected 4 spaces but found 6<br />
  50 | ERROR   | Array indentation error, expected 4 spaces but found 6<br />
  51 | ERROR   | Array indentation error, expected 4 spaces but found 6<br />
  52 | ERROR   | Array indentation error, expected 4 spaces but found 6<br />
  53 | ERROR   | Array indentation error, expected 4 spaces but found 6<br />
  56 | ERROR   | Array indentation error, expected 4 spaces but found 6<br />
  57 | ERROR   | Array indentation error, expected 4 spaces but found 6<br />
  58 | ERROR   | Array indentation error, expected 4 spaces but found 6<br />
  59 | ERROR   | Array indentation error, expected 4 spaces but found 6<br />
  60 | ERROR   | Array indentation error, expected 8 spaces but found 10<br />
  61 | ERROR   | Array indentation error, expected 8 spaces but found 10<br />
  62 | ERROR   | Array indentation error, expected 4 spaces but found 6<br />
  67 | ERROR   | Missing function doc comment<br />
  75 | ERROR   | Missing function doc comment<br />
  77 | ERROR   | Expected "foreach (...) {\n"; found "foreach(...) {\n"<br />
  78 | ERROR   | Concat operator must be surrounded by spaces<br />
  78 | ERROR   | Concat operator must be surrounded by spaces<br />
  80 | ERROR   | Concat operator must be surrounded by spaces<br />
  80 | ERROR   | Concat operator must be surrounded by spaces<br />
  84 | ERROR   | Missing function doc comment<br />
  85 | ERROR   | Concat operator must be surrounded by spaces<br />
  85 | ERROR   | Concat operator must be surrounded by spaces<br />
  86 | ERROR   | Concat operator must be surrounded by spaces<br />
  94 | ERROR   | Missing comment for @return statement<br />
  96 | ERROR   | An operator statement must be followed by a single space<br />
  96 | ERROR   | There must be a single space before an operator statement<br />
 100 | ERROR   | An operator statement must be followed by a single space<br />
 100 | ERROR   | There must be a single space before an operator statement<br />
 101 | ERROR   | An operator statement must be followed by a single space<br />
 101 | ERROR   | There must be a single space before an operator statement<br />
 113 | ERROR   | An operator statement must be followed by a single space<br />
 113 | ERROR   | There must be a single space before an operator statement<br />
 114 | ERROR   | An operator statement must be followed by a single space<br />
 114 | ERROR   | There must be a single space before an operator statement<br />
 116 | ERROR   | Spaces must be used to indent lines; tabs are not allowed<br />
 116 | ERROR   | Line indented incorrectly; expected at least 10 spaces, found<br />
     |         | 3<br />
 128 | ERROR   | Files must end in a single new line character<br />
--------------------------------------------------------------------------------<br />

Source: http://ventral.org/pareview - PAReview.sh online service

MANUAL REVIEW
--------------
I installed and enabled the module without a problem, and verified the block shows up.

1. .module line 124: I think the error meant to say that the "file is not found"?

2. _diceware_contents(): Doesn't it make more sense to first check if the file exists?

3. .module line 115: It seems that using a static value for the number of lines in the files is asking for problems later on. Can't you determine the number of lines in the file via programming? This makes it much easier if the file gets updated, or if the language files end up having a different number of lines.

AUTO REVIEW
------------
- No errors from Coder or the online PAReview utility.

Manual review... This is an interesting idea for a module. It seems like a great way to educate users on choosing passphrases -- and perhaps you could make this an opportunity to teach them a bit more about Diceware's easy-to-use method.

• To teach end-users a bit more about "Diceware," you could include a link, or perhaps static text at the bottom of your block.
• In diceware.info, there's a small typographical error; instead of description = Shows a block with sample passwords based on a the <a target="_blank" href="http://world.std.com/~reinhold/diceware.html">diceware-philosophy</a>., I would suggest description = Shows a block with sample passwords based on the <a target="_blank" href="http://world.std.com/~reinhold/diceware.html">Diceware method</a>.
• In diceware.info, the "package" property should be removed. "In general, this property should only be used by large multi-module packages... All other modules should leave this blank."

- I suggest that you name the block "Sample passwords" -> "Diceware sample passwords" to find it easily between all blocks if you have it a lot.
- Please say more in README.txt about how to use this module. (Install -> Go to blocks -> Enable (Diceware) Sample password).
- I also see some coding standard issues in your code:

/**
 * returning a default descriptional-text.
 */
function _diceware_docu() {
  $text = "Use one of the samples above without the blanks within the password. " .
      "Find more infos about the <a target=\"_blank\" href=\"http://world.std.com/~reinhold/diceware.html\">" .
      "Diceware-Method</a>.";
  return array(
    'format' => 'filtered_html',
    'value' => t($text),
  );
}

Like non-capital letter comment, indentation.
- use l() function for the links in code:
"Find more infos about the <a target=\"_blank\" href=\"http://world.std.com/~reinhold/diceware.html\">" .

A simple and fairly cleanly coded module. There are still a couple of very minor coding standards issues, based around indentation, but these are mainly where line-concatenation has been used.

The module installed and uninstalled cleanly, and worked as expected while enabled.

manual review:

1. Please don't assume that Drupal users are male on your project page.
2. diceware_init(): why do you need hook_init()? Why do you need to add your CSS to every single Drupal page, even if your module is not involved?
3. "t($text, array('!url' => $link))": do not pass dynamic variables to t() where possible, as the text cannot be detected by translation extraction tools.
4. "t('Wordlist not found: :file', array(':file' => $filename))": the ":" placeholder character does not exist for t().

But that are just minor issues, so ...

Thanks for your contribution, migmedia! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.