Room Reservations

Hi,

there is a little bit work required. Please fix errors and warnings listed here: http://ventral.org/pareview/httpgitdrupalorgsandboxbobhumphrey1834546git

Run coder module to check your code before pushing to git
(please check the Drupal coding standards)

Hi Bob,

Thanks for contributing this very interesting module and for all the detailed information.

Several additional comments in terms of code structure:

1 - room_reservation.js
I would recommend you try replacing your JS calls with Drupal.behaviors.xxxx.
For example:

(function ($) {

  Drupal.behaviors.room_reservation = {
    attach: function (context, settings) {
      // select room and start time
      $('#rooms ul li.reservable, #rooms ul li.open').mouseover(function() {
        $(this).css( {
          'background-color' : '#999999'
        });
      }); // end mouseover function
      $('#rooms ul li.reservable, #rooms ul li.open').mouseout(function() {
        $(this).css( {
          'background-color' : '#FFFFFF'
        });
      }); // end mouseout function
      
      [....Next parts of your code...]
    }
  };
})(jQuery);

As a recommendation, I would suggest you take a look at facetapi.admin.js from the Facetapi module.

2 - room_reservation.module
For coding standards, maintenance and the sake of clarity, I would recommend moving all your class objects and declarations into:
includes/room_reservation.[obj].inc
At this point you would have to include these class files in the room_reservation.info file as well.
For an example, I would recommend that you take a look at the Facetapi module, again, which has this includes folder (I think it's called plugins) with many classes declarations.

3 - room_reservation.css
line 1: The @charset is not needed I think (never saw it in other modules).
For example, check Facetapi module, mentioned in point 1.

That's all I could see so far, but I guess we would have to do other checks after the coding standards and these comments have been cleaned up.

Hope that helps. Cheers!

Hi Bob,

Really sorry for the late reply, but I have been kept overly busy with other urgent cases.

Alright, so I took a look again at the code, but I have to say the module file is very large (more than 4000 lines), so it takes quite some time obviously to even just go through half of the code of this single file (not to mention the room_reservations.admin.inc which is pretty big, as well...).

So I guess you're very likely to need quite a few reviews and testing for such an important amount of code, objects and functions.

I assume it would be a good thing to try improving structural aspects, which would end up in a better organization of the code, as I mentioned in #2:

2 - room_reservation.module
For coding standards, maintenance and the sake of clarity, I would recommend moving all your class objects and declarations into:
includes/room_reservation.[obj].inc
...
For an example, I would recommend that you take a look at the Facetapi module, again, which has this includes folder (I think it's called plugins) with many classes declarations.

At that time, I couldn't recall the example of code I was referring to, in my mind, so I just tried to find another module, but that was probably a little inappropriate/awkward.
I think this wasn't necessarily the best example I find, since Facetapi is not exactly the module I was looking for.

I went through the modules I reviewed and followed a little bit and finally found the module I was looking for: Botcha-7.x-2.0.
In particular, I would like to attract your attention at the way the files are organized with the controller and model folders which allow a clearer definition and organization of the code that is needed for defining objects and classes and or controller to do stuffs with objects.

But then it seems that's a 7.x module and Room Reservations is for 6.x (so far), so I'm not sure at all whether something like that would be feasible or recommended with drupal-6.x.
Maybe you could take a look at the Botcha-6.x to see if there is anything that could be adapted.

What I meant is that perhaps a small improvement could be to try moving the class Reservation to its own file and for that perhaps the Botcha could give you some inspiration.

I haven't looked any further than that into module's code and more specifically at the details of how this Botcha style structure could be applied, but I thought you might be interested in looking into that module as well.

That's pretty much all I got from this review so far, but I will keep looking further and surely let you know if I found anything else.

One last note, is that if you ever considered working on a 7.x version, I would certainly recommend you look at working with Entity or Entity API.

I hope all these comments will help improving this impressive module, which must have required a very important amount of work and time.

Feel free to let me know if you would have any other questions, comments or concerns on these items, I would surely be glad to provide more information.

Cheers!

Hi Bob,

You've really done some amazing work there.
It seems my comment and the Botcha module actually gave you a lot of inspiration and you managed taking down the module file from 4000 lines to 800+ .... That's an incredible improvement.
Not to mention the folder structure and distinction with helper functions, etc....

Really nice work.
I checked again the PAReview and it seems that obviously you watched out for that as well (no errors).

So far, I have nothing else to report, but I will certainly keep checking and let you know if I am able to find anything subject to suspicion.

Thanks again very much for all your work, efforts and very kind answers.
Cheers!

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

• Coder 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. http://drupal.org/project/simple_reservation/ - Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Could this be integrated into the existing module? If not please describe the differences to the existing project on your project page.
2. "module_load_include('inc', 'room_reservations', ...": no need to use module_load_include as you are including files from your own module which you already know where they are. Use something like require_once 'mymodule.inc';
3. room_reservations.module: why do you need to populate the global variable on every single page request? Do not execute any stuff in the global scope outside of a function.
4. why do you need to globally include so many files? Are there all hooks in them? Otherwise you should only include files when you actually need them. And you can specify files for inclusion with hook_menu() for example.
5. A global variable? Really? What for? Shouldn't that be just a statically cached function?
6. "db_set_active('mainsite');": what is mainsite? Do I need a second DB in order for your module to work? Please add that to the README.txt
7. _room_reservations_current_months(): why are the months not translated with t()? Please add a comment.
8. _room_reservations_hours(): could be seriously shortened with a loop?
9. _room_reservations_detect_mobile(): that looks really site-specific and should be removed. All the DB switching is really confusing ... what is the mobile site? Where is it explained that I need to configure a second DB for that?
10. "define('SAVE_CONFIRMATION_MSG', ...": all constants defined by your module to be prefixed with your module's name to avoid name collisions.
11. "form_set_error($field, check_plain($message));": why do you use check_plain() here? $message does not contain any user provided text, so it is useless. Please read http://drupal.org/node/28984 again. Also in many other places.
12. "$message = 'Invalid email address.';": all user facing text must run through t() for translation.

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

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

I'm a robot and this is an automated message from Project Applications Scraper.

Added other modules reviewed.