First of all, great start! I took a peek, and have a few action items.

module:

  • anonymous_suggestion_box_permission - permission machine names should be in lowercase.
  • anonymous_suggestion_box_permission - "Access Anonymous Suggestion Box" - semantically, should probably be "Submit anonymous suggestions"
  • anonymous_suggestion_box_menu - don't use a variable for a Menu item name; it can always be changed using the menu system.
  • anonymous_suggestion_box_mail_send - you should implement hook_mail, see https://api.drupal.org/api/drupal/includes%21mail.inc/function/drupal_ma...
  • anonymous_suggestion_box_form - Consider either theming $form['intro'] or setting a '#prefix' and a '#suffix' with the HTML tags (this gives other module flexibility for altering the form)
  • anonymous_suggestion_box_form_submit - This can be simplified. If email, email. If db, write to db. Don't need explicit if, else in this case.
  • anonymous_suggestion_box_admin_validate - Use drupal_strlen to count characters - https://api.drupal.org/api/drupal/includes%21unicode.inc/function/drupal...
  • anonymous_suggestion_box_view - Add a pager - https://www.drupal.org/node/262854 - or consider using Views instead.

install:

  • Observations and suggestions can only be 255 characters long? Anonymous suggestions can get verbose, consider using a bigger field :-)
  • Delete variables in hook_uninstall.
  • No reason to index location, observation, or suggested. This will not improve performance; if anything, it will create overhead.

Comments

benjaminarthurt’s picture

Assigned: Unassigned » benjaminarthurt

Awesome! Thanks for the review. I'll get started fixing things soon.

  • benjaminarthurt committed 5e72235 on 7.x-1.x
    Issue #2321785 by FluxSauce changed access permission to submit, removed...
benjaminarthurt’s picture

  • anonymous_suggestion_box_permission - permission machine names should be in lowercase.Fixed is now "submit anonymous suggestions"
  • anonymous_suggestion_box_permission - "Access Anonymous Suggestion Box" - semantically, should probably be "Submit anonymous suggestions" Fixed is now "submit anonymous suggestions"
  • anonymous_suggestion_box_menu - don't use a variable for a Menu item name; it can always be changed using the menu system. Fixed, changed to "Anonymous Suggestion Box" and removed corresponding field from Admin form.
  • anonymous_suggestion_box_form_submit - This can be simplified. If email, email. If db, write to db. Don't need explicit if, else in this case. Fixed, I really love overcomplicating things :)
  • anonymous_suggestion_box_admin_validate - Use drupal_strlen to count characters - https://api.drupal.org/api/drupal/includes%21unicode.inc/function/drupal... No longer needed since it was only used to validate the Title

  • benjaminarthurt committed 652db45 on 7.x-1.x
    Issue #2321785 by FluxSauce update form validation to use drupal_strlen...

  • benjaminarthurt committed 1ccf068 on 7.x-1.x
    Issue #2321785 by FluxSauce .install now using  text big for all fields...

benjaminarthurt’s picture

Status: Active » Needs review
  • anonymous_suggestion_box_view - Add a pager - https://www.drupal.org/node/262854 - or consider using Views instead. Added views, supplied default view (page and block), default view is enabled by default, menu path set to replace default page with-in module. Left default display page intact as a fallback if views not enabled (although seriously, why not Views are awesome). I should eventually edit the default module display page so that it has a pager or some sort of searchable something or other.
  • Observations and suggestions can only be 255 characters long? Anonymous suggestions can get verbose, consider using a bigger field :-) fixed - now using text big for all 3 fields.
  • Delete variables in hook_uninstall.fixed...
  • No reason to index location, observation, or suggested. This will not improve performance; if anything, it will create overhead. Fixed.
benjaminarthurt’s picture

Status: Needs review » Closed (fixed)