Views Slideshow J360

Hii,

Smells like a great module. However here are some points found by a quick manual review:

1. Remove LICENSE.txt file. It will be automatically added by Drupal druing packaging.
2. Remove version, project, datestamp from .info file. These will also be be automatically added by Drupal.
3. Drupal is pretty strict with coding style and indentation issues. There are presently lots of them in the module. Please take time to fix them.

Its good idea to run you module through the coder module and automatic code sniffer and fix the errors.
http://ventral.org/pareview/httpgitdrupalorgsandboxsardbaba1821032git

Also install and use coder module to fix the coding style issues.

Good Luck!!..
Aritra

Ciao Mauro,
Some considerations regarding your points in #3, the review application:

• is a one-time process (once inside, no more hassle)
• avoids D.O. being cluttered with (duplicate) modules (clean)
• is educational (free advice)
• leads to readable code, being forced to follow standards and include comments (facilitates cooperation afterwards)
• gets accelerated (a lot) once you get a review bonus (fair).

From your personal site can be deducted that you are an active user of several CMS. It's only logical you compare the process. In fact it's a valuable asset for the community to have members with a broad experience like you. Comparing is a good way to spot possible improvement.

The feedback on your module so far is indeed a bit meager, despite the fact the module offers an interesting feature set (wow, what a nice demo). I thought it was due to a certain complexity (Javascript and theme functions are involved), but it's actually not that hard. In any case I think that a simple module sometimes has an advantage in the review process because it's easier accessible for reviewers.

"Do I have to consider Ventral.org stuff?"
Yes, but in your case it seems that they are most minors. Consider it might report false positives in some rare cases.

I'm feeling a bit guilty for my off topic chatting instead of delivering a to-the-point technical review. I hope you are persistent, because especially you can know Drupal is worth it. As well the hard work of many people in it's community makes it worthwhile. In any case you're not alone in your opinion, but everything has a flip-side.

Aah, and another reason the feedback might be scarce, once you fixed some issues like you indicate in #2, setting the status back to "needs review" gives it a better exposure in the issue queue.

I don't see any issues with the code but there are a few issues with the project in general:

1. You don't have a release branch in your git repo (see http://drupal.org/node/1015226)
2. I'm not an expert on licensing but there may be a conflict between the cc-by-nc license used by the spritespin plugin & the gpl that distributed Drupal modules use. At the very least, the license file added by the drupal.org packaging scripts may cause confusion about the license. It may be best to not distribute it with the module & instead prompt users to download & install it separately.
3. You should consider using the libraries module to include spritespin. Going along with #2, that page also provides some more info about why 3rd party libraris shouldn't be included with modules.

Nice idea for a module.

The result for the automated review is attached. There's a lot to fix, use the PAReview script to validate as you go through the fixing process.

Result of the manual review:

1. You don't have the necessary branch on Git repository, it's still a master, but should be referring to a version, see Moving from a master to a major version branch.
2. Automated review revealed that you're using tabs instead of spaces for indenting the lines across all files. Drupal coding standard is 2 spaces per a tab. Configure your code editor for it. Both Netbeans and Eclipse have settings to remove trailing whitespace or handle tab width. There are good resources available on Community Docs, e.g. Configuring NetBeans.
3. views_slideshow_j360.module ln 33: glob() call does not make any sense there as you're not expecting back a list of files, but confirming a single file. It is overly expensive to do it like this, use file_exists() instead and you can also dump the array_shift().
4. views_slideshow_j360.theme ln 25: you're using extract() to populate local variables for an associative array of settings. While it's convenient, It's not transparent and code becomes challenged in terms of readability. One has no idea if the variables assigned to $settings exist or derive from $options. Please make it more transparent and obvious. Follow the KISS principle and try to keep it DRY.
5. The same transparency issue is keeping methods in different files, e.g. _views_slideshow_j360_library_path() from views_slideshow_j360.module is used in views_slideshow_j360.theme.inc only. While separation is often a good thing, it does not deliver here. You could easily join these 2 files and it'd all become a lot clearer, plus it's one file less to access the disk for.
6. In the installation documentation, you advise user to download an external library and upload it to the module folder. This is generally a bad idea and you don't want user juggling with your module layout. I'd suggest advising a user to upload under sites/all/libraries (sites/sitename/libraries for multisite) instead and using Libraries API in the module to fetch it.

Good luck!

Automated review

The module still has errors, see http://ventral.org/pareview/httpgitdrupalorgsandboxsardbaba1821032git-7x...

Manual review

1. Good README.
2. Re views_slideshow_j360.module ln 33 glob() usage: code in approved module doesn't necessarily mean it's a totally valid. Please keep in mind that this approval process here is not about a particular project approval, but full project access, citing Apply for permission to create full projects:
   

   After contributing a useful and working module or theme as a sandbox project, you may go through a one-time approval process to get permission to promote it (and future projects) to a full project.

3. views_slideshow_j360_spritespin_path() return type undefined, should be mixed, e.g. @return mixed.
4. template_preprocess_views_slideshow_j360_main_frame() parameters undefined.

Hi Sardbaba.

I manually reviewed your module (7.x-1.x+1-dev):

1. 7.x-1.x+1-dev branch name is incorrect, use 7.x-1.x as your main development branch: http://drupal.org/node/1015226
2. In views_slideshow_j360.module on line 37 you call the _views_slideshow_j360_library_path() function, but this function does not exist (should't be views_slideshow_j360_spritespin_path()?)
3. Your default branch is 'master', change default branch: http://drupal.org/node/1659588, and also remove master: http://drupal.org/node/1127732
4. Your module depends on views and libraries, but there is no dependencies in the .info file.
5. views-slideshow-j360-main-frame.tpl.php - I do not know whether these are coding standards, but all of template files that I've seen look different, instead of doing things like this:
   echo '<div id="views-slideshow-j360-images-' . $id . '" class="views-slideshow-j360-images ' . $classes . '">';
   you should do this:
   <div id="views-slideshow-j360-images-<?php print $id; ?>"class="views-slideshow-j360-images <?php print $classes; ?>">, also that 'foreach' can be done in theme_preprocess_views_slideshow_j360_main_frame(&$variables);
6. views_slideshow_360.js - comment: "Debug settings values..." - I think that it is no longer needed.

If I'm wrong, forgive me, but this is my first review :)
Looking forward to this module, very useful for displaying commerce products.

Hi Sardbaba, good work!
I mean, for example, the developer for each view may have its own template, and each time it has to copy the long code, so a better idea is to move it to preprocess functions. To better understand what I mean, I enclosed patch.

Deeper manual review :)

1. views_slideshow_j360.views_slideshow.inc line 11, 12:
   

   Do not document parameters and return value though -- these are documented on the hook definition referenced in the summary line. http://drupal.org/node/1354#hookimpl

   Just

    /**
   * Implements hook_views_slideshow_slideshow_info().
   */

   is sufficient here.

2. views_slideshow_j360.views_slideshow.inc line 45, 46 - You used C style comments within function.
   

   C style comments (/* */) and standard C++ comments (//) are both fine, though the former is discouraged within functions (even for multiple lines, repeat the // single-line comment). Use of Perl/shell style comments (#) is discouraged. http://drupal.org/node/1354#inline

3. I don't know why coder module report this:
   views_slideshow_j360.views_slideshow.inc
   Line 42: Potential problem:drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar. (Drupal Docs) [security_3]
   drupal_set_message(t('You must install the spritespin plugin. You can find it at !website', array('!website' => $link)), 'error');
   but if change '!website' to '@website', warning no longer appears.
4. views_slideshow_j360.js - variables like orig_width should be lowerCamelCased - origWidth
   

   Unlike the variables and functions defined in Drupal's PHP, multi-word variables and functions in JavaScript should be lowerCamelCased. The first letter of each variable or function should be lowercase, while the first letter of subsequent words should be capitalized. There should be no underscores between the words. http://drupal.org/node/172169#camelcasing

5. views-slideshow-j360-main-frame.tpl.php - inline css: style="display:none" - is it possible to move it to a .css file?
6. Add git clone link to Issue Summary.

But these are trivial errors, installed and tested the module, works as expected, so for me it is RTBC.
But the last word belongs to the git administrator, so to speed up the review process, participate in the review bonus.

Good luck Sardbaba, keep the great work.

Do not RTBC your own issues, see http://drupal.org/node/532400

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

Oh, just saw that drupik argued for RTBC, so this is probably right. Sorry for the noise :-)

Added link to git

manual review:

• "(int) check_plain($frametime)": that check_plain() is useless here if you convert the string to an integer anyway.

But that is not an application blocker, so ...

Thanks for your contribution, sardbaba!

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.

Congrats sardbaba, all the best :)

Well done. Time for an Ichnusa, I guess. :)

Hi sardbaba.
I'm not sure, but I think you should add tag to project: git tag 7.x-1.0 or for dev git tag 7.x-1.x and push the changes.

Go to http://drupal.org/node/1821032/edit/releases. Check "Show snapshot release".

To create a stable relaease follow the instructions under "Version control", then http://drupal.org/node/add/project_release/1821032 (also at the bottom of the project page (in View mode). It takes some time for a new release to show up. Look at http://drupal.org/node/1821032/release. If they are red, they're not published yet (visible for you but not for others).