Dynamic Comments

welcome,

As most people rather look on project pages instead of readme's I'd suggest you to put the installation and usage instructions also onto your project page, may also have a look at the tips for a great project page. README.txt conventions: guidelines for in-project documentation.

I've seen you already used the automated review (http://ventral.org/pareview/httpgitdrupalorgsandboxmariano1538470git), great,
if you got questions on some of the left issues, feel free to ask.

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

Automated review:

• 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.

FILE: .../sites/all/modules/pareview_temp/test_candidate/dynamic_comments.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 60 | WARNING | Line exceeds 80 characters; contains 85 characters
 63 | WARNING | Line exceeds 80 characters; contains 104 characters
--------------------------------------------------------------------------------


FILE: ...pareview_temp/test_candidate/includes/dynamic_comments_post_handler.inc
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 100 | WARNING | Line exceeds 80 characters; contains 82 characters
 161 | WARNING | Line exceeds 80 characters; contains 82 characters
--------------------------------------------------------------------------------


FILE: ...les/pareview_temp/test_candidate/includes/dynamic_comments_settings.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 1 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 35 | ERROR   | Whitespace found at end of line
 42 | WARNING | Line exceeds 80 characters; contains 82 characters
--------------------------------------------------------------------------------

1. IMO: You have many ugly blank lines, I would recommand removing them for better readability.

2. You should use t() around the message, for allowing translations to work.

              $message = 'Dynamic Comments doesn\'t seem to have a JQuery selector set. Please visit the module\'s admin page and set your body classes in the "Content Class" section.';
              drupal_set_message($message, 'error');

3. You don't need $message above, just put it into the function.

4. you should put html into theme functions or files:

    // ui basic elements' markup
    sidebar_HTML : '<div id="dynamic-comments-sidebar" class="dynamic-comments-sidebar">\n\
                      <div class="dynamic-comments-sidebar-messages">\n\
                      </div>\n\
                      <div class="dynamic-comments-sidebar-inner">\n\
                      </div>\n\
                    </div>',
    icons_container_HTML : '<div class="dynamic-comments-icons">\n\
                            </div>',

5. You should use theme functions or .tpl files:

    $message .= '<ul>';
    
    foreach ($files_missing as $file_name => $file_path) {
      $message .= '<li>' . $file_name . ' in the path ' . $file_path . '</li>';
    }

    $message .= '</ul>';

6. don't use "code-separators" :

/**
 * HELPER FUNCTIONS.
 */

7. Where does the following comment belong to:

// Return node types in this site.
// (separated function just to keep this whole thing organized).

8. First line does not male sense:

  $types = array();
  $types = node_get_types('names');

---
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

you could maybe take a look at http://engineeredweb.com/blog/11/5/javascript-theme-functions-drupal/

Hello Mariano,
this is a sexy module imho, you got a nice idea.

Did you forget t() on dynamic_comments_settings.inc:62-67? :)

I have just some comments on computation.
In your nodeapi hook, 'view' branch, you are operating on your module's allowed content types. You could of course check this condition before trying anything else, because that's mandatory: since you're operating on every node view this could save some elaboration, but this could be just a silly thing.
What could impact more is the file_exists() question on line 67 and 75, and every drupal_add_* coming next: remember that your nodeapi-view hook is performed on each page load, and every file_exists trigger a potential unnecessary disk (or cache...whatever) hit. You have edge to optimize your module (remember M operation * 1 page load * N concurrent users could grow up naughty)

Keep on the good job
Regards

Hi Mariano,

There are still code indentation issues:
http://ventral.org/pareview/httpgitdrupalorgsandboxmariano1538470git
(http://ventral.org is still under development and changed frequently hence the quality of automated reviews can vary from day to day. :( )

Manual review :

#1 Have a look at http://drupal.org/project/libraries to see how you can integrate libraries in a better way.

#2 In dynamic_comments_settings.inc, you are building error messages (list items) for drupal_set_message(), you could use drupal_set_message() for individual message.

#3 I think your module does not delete it's variables on uninstallation.

#4 It is recommended that for admin settings you should use file name convention like [modulename].admin.inc

#5 specific permission names as 'delete single comments', should at least contain your module name.

#6 In some cases you can save extra variables eg:

function _dynamic_comments_get_types() {
  $types = node_get_types('names');

  return $types;
}

Instead

function _dynamic_comments_get_types() {
  return node_get_types('names');
}

Is better way.


I will have to look into the deep ...

Hello there,
it is a recommended practice to delete your variables in module uninstall hook.
See http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/varia...

Anyway i think this module is nearly rtbc as far as i can see.

Regards

Hi Mariano,

You should fix those issues listed in http://ventral.org/pareview/httpgitdrupalorgsandboxmariano1538470git since these are code standards. Exceptions are only allowed in case if 'pareview' throwing any wrong issue.

Also please update your code according to the manual review.

Thanks,
Rajan

Closing due to lack of activity. Feel free to reopen if you are still working on this application.