Morse code converter

Hi prathK,

I reviewed your project and created a couple issues in the project issue queue. Please change the status back to "needs review" when those are fixed.

welcome!

naming the branch 6.x-2.x makes no sense for me, is there a special reason for this?

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 6.x-2.x branch:

• README.txt is missing, see the guidelines for in-project documentation.
• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• Remove "project" from the info file, it will be added by drupal.org packaging automatically.
• ./morse.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function convert_to_morse() {
  

• Remove all old CVS $Id tags, they are not needed anymore.
  

  arebic.js:1:// $Id$ arebic.js
  morse.info:1:; $Id$ morse.info,v 1.0 2011/08/17 23:25:00 prathK Exp $
  morse.js:1:// $Id$ morse.js
  morse.module:2:// $Id$ morse.module,v 1.0
  

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See http://ventral.org/pareview/httpgitdrupalorgsandboxprathk1282520git.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See http://ventral.org/pareview/httpgitdrupalorgsandboxprathk1282520git.

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. Go and review some other project applications, so we can get back to yours sooner.

Couple things:
1. Both of the functions below contain the same entire alphabet to morse array.

function morse_convert_to_arebic($num) {}
function morse_convert_to_morse($num) {}

Why not move the array into its own helper function and then just call the function when you need it.

function _morse_alphabet() {
  $alphabet = array(
    "A" => ".-",
    "B" => "-...",
    "C" => "-.-.",
    "D" => "-..",
    "E" => ".",
    .....

  return $alphabet;
}

function morse_convert_to_arebic($num) {
  $MORSE_CD = _morse_alphabet();
...
}

This will avoid duplication.

2. Your hook_help and info file state that it only converts numbers.
morse_help:

The morse module converts the normal numbers into morse numbers

morse.info:

converts the simple number into morse code.

Your module converts more than just numbers. It should say something like your @file block in your module file:

Converts the alphanumeric values into morse code.

3. Your submit buttons should be in an "actions" array element.

  $form['actions'] = array('#type' => 'actions');
  $form['actions']['submit'] = array(
    '#type' => 'submit',
    '#value' => t("Convert"),
    '#weight' => 30,
  );

It's always good to put (submit) buttons in an "actions" form element.

Automtated review:

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 6.x-2.x branch:

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.

Code too short

This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project.

Even so your project has more than 5 functions and more than 120 lines, I find it diffcult for doing a serious review... don't know what other think...

Manual review:

function morse_perm() {
  return array('access morse converter');
}

The permission is never used!
See: 'access callback' => TRUE

forgot status change

The response time for a review is now approaching 4 weeks.
Get a review bonus and we will come back to your application sooner.
See: http://drupal.org/node/1410826

You could for example start by evaluating my own project:
http://drupal.org/node/1302786

This looks good to me. I don't see any new issues and all previous issues have been addressed with the exception of the 'actions' #type, which I believe was only added in Drupal 7, so can't be used in this Drupal 6 module.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Removing review bonus tag, you need to do 3 manual reviews of other projects, not just one. See #1410826: [META] Review bonus. And please add your review links to your issue summary.

updated comment.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

manual review:

1. project page is too short. How does the module work and where is the code conversion displayed? See also http://drupal.org/node/997024
2. morse(): this is not a hook, but a form building function and should be documented as such. See http://drupal.org/node/1354#forms
3. morse(): why do you need to read/write the user session? Submitted form values are available in $form_state['values'] in your form constructor, see http://drupal.org/node/751826
4. morse_convert_to_morse(): doc block for parameters/return values is wrong, see http://drupal.org/node/1354#functions . Also elsewhere, it is called "@param" not "@params".
5. morse_submit(): if wonder if you need that function if you use $form_state in the form contructor.
6. morse_convert_to_arebic(): I think you should reverse the array returned by _morse_alphabets() with array_flip(), then you don't need to search the array and can just access with $morse_codes[$v]. Which is also more efficient.
7. "$output .= "";": this has no effect, why do you need that?

I bump this back to needs work, because I think writing to the session is really wrong here. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

prathK, morse() not having $form and $form_state parameters is a problem you can fix simply by adding those parameters. You're implementing hook_form(), but not following the documented parameters for hook_form().

just want to take the time here and say that I really like module idea ;-)

I like it!

If you do three more reviews, you should get this promoted pretty quickly.

manual review:

1. "'#value' => "Convert",": all user facing text must run through t() for translation.
2. morse_form(): you do not implement a node type, so it does not make sense to implement hook_form(). Please rename that function so that there is no confusion. And $node should be $form, $form_states should be $form_state.
3. morse_arebic_form(): wrong doc block, this is not a hook but a form building function. See http://drupal.org/node/1354#forms
4. Both your form building functions will never get a node passed, so that is really confusing.
5. morse_form(): displaying the message should be done if $form_state['values']['alphabet'] is set. Similar in morse_arebic_form().

I have reviewed and tested the module. It appears that all previous indicated issues have been resolved. I do not see any code-specific issues which need to be addressed.

The module works as described.

manual review:

• morse_submit(): why do you need check_plain() here? $num is not printed to the user? Same in morse_arebic_submit().

Not an application blocker, so ...

Thanks for your contribution, prathK!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

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.

Thanks to the dedicated reviewer(s) as well.