Ladder

Hi lang14!

Can you explain me what is a ladder exactly ?

I just install your module to test it.

A readme.txt would be good to explain how to use your module.

You talk about leaderboards in #1, the main goal of your module is to manage these leaderboards or it will be more than that ?

Thank you for your answers :)

Alright you have some big changes to make with formatting.

- Missing @file in some files.

- Your indentation is all wrong. All of your code is indented four and five spaces? You don't need to indent all of your code at all and especially not that much. i.e. this

    /**
     * @file
     * Main file for Ladder.
     */

should just be

/**
 * @file
 * Main file for Ladder.
 */

plus indentation inside your functions is three spaces in .install and four spaces elsewhere. All indentation should be two spaces.

- Many of your functions have no documentation at all. In other words this is all wrong:

 
 
    function ladder_theme() {
        return array(
            'ladder' => array(
                'template' => 'ladder',
                'variables' => array(
                    'game' => false,
                    'games' => array(),
                    'leaderboard' => false,
                    'leaderboards' => array(),
                )
            ),
        );
    }

Whether it's a hook or it's not you still need to document the function. Please see documentation documentation (haha, not a typo) for more information.

- This is not correct either:
}else{

- As I said, many many functions have no documentation at all, but especially functions like this:
function ladder_games_admin($op = '', $gid = 0) { need documentation. Here is the documentation on that documentation: http://drupal.org/node/1354
It will explain how to document parameters and return values which are often necessary to make your module's API functions understandable.

        /*
         * Get the form that will be rendered on this page.
         */
        $build['ladder_games_form'] = drupal_get_form('ladder_games_form', $op, $gid);
        /*
         * Destination to go to after operations are executed.
         */
        $destination = drupal_get_destination();

- These types of inline comments should preferrably be of the // Variety. Full sentence comments should start with a capital letter and end with a period.

    /**
     * Form function, called by drupal_get_form()
     * in ladder_menu().
     */
    function ladder_games_form($form, &$form_state) {

-This is precisely what an @see reference is for.

echo $g->name.'<br />';
- String concatenation should have spaces around the operator so it's easier to read.

In hook_menu()...
$items['admin/ladder'] = array(
and
$items['admin/ladder/settings'] = array(

- I'm almost certain you do not want that path. This should be placed in a more appropriate place like admin/config/somewhere I dunno, but certainly not there.

        $form['ladder_enabled'] = array(
            '#type' => 'radios',
            '#title' => t('Enable'),
            '#default_value' => variable_get('ladder_enabled', LADDER_DISABLED),
            '#description' => t('Enable the Ladder module.'),
            '#options' => array(
                LADDER_ENABLED => t('Enabled'),
                LADDER_DISABLED => t('Disabled')
            ),
        );

- Why is this necessary? Drupal already provides the ability to enable/disable modules so this seems like it may be a bit misleading as this does not actually do anything but set a variable. I haven't been able to find anywhere that this variable is used either.

- I think you should have an empty line at the bottom of each file as well.

- Need a README.txt file with instructions on how to use the module if possible.

- Other than that, I would just say it looks like a lot of spacing for me but it could just be the indentation issues, and also there are some empty lines (with spaces).

- This last part is just my opinion, but do you think the name Ladder might impact the success of people looking for a module like this? Ladder just doesn't describe what it does to me at all. But I guess the good names are already taken. Maybe someone can make a suggestion, but I'm not that creative!

So, fix those issues and set it back to needs review when you're ready! Good luck!

Little more:

    /*
     * Validate function for ladder_games_form()
     */
    function ladder_games_form_validate($form, &$form_state) {
        if (strlen(trim($form_state['input']['name'])) == 0) {
            form_set_error('', t('You must enter a game name.'));
        }
    }

- Why not just use empty() and beyond that why not just make the 'name' field required?

drupal_set_message($form_state['input']['name'] . ' was saved.', 'status');

This should use placeholders not string concatenation and still need to use t() like:

drupal_set_message(t('%name was saved.', array('%name' => $form_state['input']['name'])), 'status');

... as should watchdog()...

watchdog('ladder', '%name was saved.', array('%name' => $form_state['input']['name']));

Line 260 in ladder_leaderboards.module has a form_set() function:
form_set('', t('Your starting date must be before your end date'));

        $modules = module_list();
        $game  = isset($modules['ladder_games']);

- Again, why not just use module_exists()? This is exactly what it does, and that's what API functions are for. This is just duplicating code in my opinion.

Updating all these issues should get you right on track!

Lang14,

While the initiative is quite a ways behind compared to where you are with your Drupal 7 module and plans, please consider joining the 'Sports Club (League) Management' group (http://groups.drupal.org/sports-club-league-management), both to keep tabs on and participate in the discussions regarding development of a centralized 'league' API initiative within D7 (http://groups.drupal.org/node/150659).

It's an awfully big dream ... and I could really use involvment from a few more developers to pull it off.

@lang14 great! I'm sure that got a lot of the issues out of the way. For the best examples on correct Drupal coding practices refer to the core modules. That's how I figured out what the Drupal way is. Or the Examples for Developers module has some good stuff as well and they work hard to make sure it's done right.

I've taken a quick look through this module to see if there are any other issues to fix. A few comments:

• Your README.txt should wrap lines at 80 characters, as per the module documentation guidelines.
• In ladder_page(), it's recommended that you put braces around the if() blocks even though they're only one line -- prevents mistakes later.
• ladder.tpl.php does not use check_plain() to escape data from the database, so any game title or leaderboard entry containing HTML will be injected right into the page. You should escape the data.
• ladder.tpl.php also hardcodes the words "Games" and "Leaderboards". You should make these translatable with t().
• In ladder_games_admin() and ladder_leaderboards_admin(), your watchdog() call doesn't include the name of the game which was removed, so an admin viewing the logs will not know what happened.
• In ladder_games_admin() and ladder_leaderboards_admin(), you don't seem to sanitize any of the data retrieved from the database before inserting it into the table. I think you need to run it through check_plain, although I'm not sure -- try creating games with HTML entities in the titles and see what happens.
• In forms, #value and #title fields must be translated with t(). This problem occurs in several places.

In general, ensure you pass everything through t() that needs to be translatable, and check that you properly escape your HTML output so users cannot insert malicious HTML.

Once you've corrected these issues, set the status back to "needs review" and I'll try to take another look through.

No activity in several months. Reopen and set the status to "needs review" if you are still pursuing this application.