tcniki [tcniki]

Please provide the code of the module here, so it can be reviewed.

1. function bannerconnect_adspace_block($op = 'list', $delta = 0, $edit =  array()) {
     switch ( $op) {
       // ...
       if ($section_id == '') {
         $demo = 1;
         // ...
     }
     // ...
   }
   

   The IF-statement can be simply wrote

       if (!$section_id) {
         // ...
       }
   

2. In the same function, the consecutive IF-statements, which check for increasing values, can be replaced by a SWITCH-statement.
   Also, if ($key_exist == 1) { is equivalent of if ($key_exist), if you just need to check for 1 against 0 (or TRUE against FALSE).
   To notice that array_key_exists() returns TRUE, or FALSE.

3. function bannerconnect_adspace_get_ad_sizes() {
     $bc_ads =  array();
     $result = db_query( "SELECT  * FROM {variable} WHERE name = '%s'", 'bannerconnect_adspaces');
     while ($data = db_fetch_object($result)){ // <-- Add a space before {
     	$adspace = $data->value;
     }
   

4.   for ($i = 0;  $i < count($pieces); $i += 1)
     {
       if ($pieces[$i] != 0 && $pieces[$i] != NULL) {
         array_push($adspaces,  trim($pieces[$i]));
   	}
     }
   

   The indentation character should not be the tab, but two spaces.
   The loop can be simply written as

     $adspaces =  array();
     foreach (explode(",", $adspace) as $piece) {
       if (!empty($piece)) {
         $adspaces[] = trim($pieces[$i]));
       }
     }
   

   Check also the coding standards for how to write Drupal compliant code.

5.   while ($data = db_fetch_object($result)) {
       $section_id = $data->value;
   	if ($section_id == NULL) {
         $section_id = '';
       }
     }
   

   The code can be simply written as

     while ($data = db_fetch_object($result)) {
       $section_id = isset($data->value) ? $data->value : '';
     }
   

   The code should not try to access a variable, or an object property without to verify if it is set.

6. function bc_adspace_main_settings_submit($form, &$form_state) {
     $publisher_id =  $form_state['values']['pubid'];
     $numbers .= $form_state['values']['numbers']['120 X 600'];
     $numbers .= ', ';
     $numbers .= $form_state['values']['numbers']['160 X 600'];
     $numbers .= ', ';
     $numbers .= $form_state['values']['numbers']['300 X 250'];
     $numbers .= ', ';
     $numbers .= $form_state['values']['numbers']['336 X 280'];
     $numbers .= ', ';
     $numbers .= $form_state['values']['numbers']['728 X 90'];
     $numbers .= ', ';
     $numbers .= $form_state['values']['numbers']['468 X 60'];
     // ...
   }
   

   The code can probably be simplified in

     $numbers = implode(', ', $form_state['values']['numbers']);
   

7. function bannerconnect_adspace_get_ad_sizes() {
     return array('120 X 600' => '120 X 600', '160 X 600' =>  '160 X 600', '300 X 250' => '300 X 250', '336 X 280' => '336 X 280' , '468 X 60' => '468 X 60' , '728 X 90' => '728 X 90');
   }
   

   If the function is used in just a place, and it is not supposed to be called from third-party modules, then it can be removed.

8.     '#required' => FALSE,
   

   That is the default value; there is no need to explicitly set it.

9. /*
    * <!-- BEGIN STANDARD TAG - 300 x 250 - ROS: Run-of-site - DO NOT MODIFY -->
   <SCRIPT TYPE="text/javascript" SRC="http://ad.bannerconnect.net/st?ad_type=ad&ad_size=300x250&section=405846"></SCRIPT>
   <!-- END TAG -->
   
    */
   

   What is the purpose of that comment?

10. <?php
    ; $Id$
    name = BannerConnect Adspace
    description = A module where you can place a ad on your site.
    core = 6.x
    package = "BannerConnect Ads"
    

    It's not a PHP file; the <?php needs to be removed.

11. /*
     * Implementation of hook_install()
     */
    function bannerconnect_adspace_install() {
      // Create tables.
      db_query( "insert into {variable} (name, value) VALUES('%s', '%s')", 'bannerconnect_adspaces', NULL);
      db_query( "insert into {variable} (name, value) VALUES('%s', '%s')", 'bannerconnect_adspace_section_id', NULL);
      
    }
    
    function bannerconnect_adspace_uninstall() {
      // Remove tables.
      db_query( "DELETE FROM {variable} WHERE name = '%s'", 'bannerconnect_adspaces');
      db_query( "DELETE FROM {variable} WHERE name = '%s'", 'bannerconnect_adspace_section_id');
    }
    

    There is no need to set the value of Drupal variables; if the variables are not set, variable_get() will return the second parameter.
    The uninstall queries can be replaced by

      db_query( "DELETE FROM {variable} WHERE name LIKE 'bannerconnect\_%'");
    

    In this way, you don't need to add new queries when you add more Drupal variables.

12. The README.txt is perfectly useless; I guess it is not completed ("Welcome" is the only text I see).

1. Previous point 10 is still valid.
2. In bannerconnect_adspace_block() you keep to set $enabled to 1 (I would rather set it to TRUE, so it would be clearer the purpose of the variable - as the code is, it seems the variable counts the number of items enabled), and then check if that variable is 1 (which is always true, because you set it to 1). The code must be changed. When $op is "view", you are supposed to return the blocks you are listed when $op was "list"; as Drupal will not pass to your hook a value for $delta that you didn't return, that code is useless.
3. Check the coding standards; your files uses a indentation that is not the one the standards suggest.

4.   $form['configuration'] = array(
       '#type' => 'fieldset',
       '#collapsible' => TRUE,
       '#collapsed' => FALSE,
       '#title' => t('Configuration'),
     );
     $form['configuration'] = array(
       '#type' => 'fieldset',
       '#collapsible' => TRUE,
       '#collapsed' => FALSE,
       '#title' => t('Configuration'),
     );
   

   There is duplicated code.

5. strlen($section_id) should be drupal_strlen($section_id).

6.   function bc_adspace_main_settings_validate($form, &$form_state) {
     }
   

   All your functions must have a name starting with bannerconnect, and the same is true also for the Drupal variables used by your code.

7. bc_adspace_main_settings.inc should be renamed bannerconnect_adspace_main_settings.inc.

Before to commit the code in CVS, remember to remove the bc_adspace_main_settings.inc file.

Also, the following code can be optimized:

      switch($delta){
        case 0:
          if ($key_exist) {
            $width = $ads[$delta]['width'];
            $height = $ads[$delta]['height'];
            $css_class_name = 'bc_ad' . $width . 'x'. $height;
            $block['content'] .= '<div class="' . $css_class_name . '" >' . $extra_text . '<script type="text/javascript" SRC="http://ad.bannerconnect.net/st?ad_type=ad&ad_size='. $width . 'x' . $height . '&section=' . $section_id . '"> </script></div>';
          }
          break;
        case 1:
          if ($key_exist) {

Rather than checking for $key_exist on every CASE-branch, it would be better to check the value of that variable before the SWITCH-statement.

Thank you for your contribution! I am going to update your account.

These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.