[D6] Tic Tac Toe

welcome,

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow the tips for a great project page. Also make sure your README.txt follows the guidelines for in-project documentation.

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxankitchauhan1556724git

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

Posted by patrickd on April 24, 2012 at 10:05pm

Thanks for your participation, but I'm afraid the comments you did are not very detailed and IMHO not really "reviews".
A good deep review can take up to an hour, this is not about just pointing out 2 things and set the issues to needs work.
Sorry, removing tag.
http://drupal.org/node/1540804#comment-5912808

project page

Please take a moment to make your project page follow tips for a great project page.

reviewed project links added

Regarding the .info file and using 'Games' as the package

Per: Writing .info files (Drupal 6.x)

In general, this field should only be used by large multi-module packages, or by modules meant to extend these packages, such as CCK, Views, E-Commerce, Organic Groups and the like. All other modules should leave this blank. As a guideline, four or more modules that depend on each other (or all on a single module) make a good candidate for a package. Fewer probably do not.

Looks like it would be pretty easy to pass for i18n (per Coder) by updating your JS:

TicTacToe.js
Line 109: Javascript strings should be passed through Drupal.t().
          alert('You Lost');
Line 114: Javascript strings should be passed through Drupal.t().
          alert('You Won');
Line 119: Javascript strings should be passed through Drupal.t().
          alert('The Game is Drawn');
Line 123: Javascript strings should be passed through Drupal.t().
          alert('Unknown status');

There's a few of instances of using double quotes when only single quotes are needed for passing string arguments in the PHP. Board.php, GameSearch.php and TicTacToe.php all have some.

The only other concern I have is the namespace issues for the additional files e.g. Board.php, GameSearch.php and TicTacToe.php as well as the Javascript files; it would be good if all these files had the namespace for the module e.g. tic_tac_toe_board.php etc. Also the functions contained in these files should also contain the module namespace. This is just so it's clear that all of these functions, files etc. belong to this module.

I installed the module and it seems to work fine.

Also, you might want to fix up your GIT repo clone instructions at the top of this issue as it doesn't work - this only applies to you. You need to use this instead if you want to review (listed on the version control tab off the sandpit anyway):

git clone --recursive --branch 6.x-1.x http://git.drupal.org/sandbox/ankitchauhan/1556724.git tic_tac_toe
cd tic_tac_toe

There's a number of errors with the js script raphael-min.js showing up in ventral: http://www.ventral.org/pareview/httpgitdrupalorgsandboxankitchauhan15567...

Nice to have

It would be a nice addition to have a button to enable you to clear a game and start a new one (i.e. in the block). Also a score tally would be good.

Cheers

project description updated

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

The file names do only matter for the module file and the info file, all other files do not have to be namespaced.

manual review:

1. tic-tac-toe-raphael-min.js appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
2. tic_tac_toe_game_action(): You should use drupal_json(), or at least set the correct header if drupal_to_js() does not work for you.
3. "abstract class GameSearch": all class names should be prefixed with your module name to avoid name collissions. Same for "Board".
4. tic-tac-toe-block-view.tpl.php: do not embed script stuff into the template. Use drupal_add_js() and Drupal.behaviors to manage your javascript, see http://drupal.org/node/304258
5. tic-tac-toe-block-view.tpl.php: do not embed style information, use dedicated CSS files and drupal_add_css() instead.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

@klausi - oh I didn't know. I thought the namespace thing was real. Silly me. Maybe I dreamt it.

Sorry for the confusion @ankitchauhan. Good luck!

I was looking for official documentation about README.txt being the only file that should be in the MASTER branch but I can't find it.

There are still files other than README.txt in the master branch...

http://drupal.org/node/1556812#comment-5971306

You can keep the master branch with only README.txt in it.

Also see: http://drupal.org/node/1074960

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: http://drupal.org/node/1556812
Project 2: http://drupal.org/node/1539536

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm afraid I've to remove that tag because 2 of your reviews are not really manual as required for review bonus, they are just pointing out issues that are / can be found by automated tools. Sorry.

project description updated

Sorry for the delay. Make sure to review more project applications and get a new review bonus to get this finished faster.

Review of the 6.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (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. You have to get a review bonus to get a review from me.

manual review:

1. tic-tac-toe-start.js: should use Drupal.behaviors, see http://drupal.org/node/304258
2. tic_tac_toe_admin_settings(): your module defines its own variables, so you should remove them in hook_uninstall().

Although you should fix those issues they are no blockers, so I think this is RTBC.

Hi ankitchauhan,

Thanks for your contribution and 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, at your discretion.

Now that you've experienced the full review process, please consider reviewing other projects that are still awaiting review. Anyone can help with reviews, following the guidelines.