Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

DateHelper patch

#665

As mentioned in this discussion I've started consolidating and refactoring the helpers.

The patches in the attached diff introduce the following refactorings/enhancements:

  1. Removed unnecessary method DateHelper#leading_zero_on_single_digits and replaced it with sprintf("%02d", number).
  2. DRYed up DateHelper methods select_second, select_minute, select_hour, select_day and select_year by moving creation of tags into build_options method. Not used by select_month because it would unnecessarily bloat the build_options method.
  3. Introduced option :date_separator for DateHelper methods select_date and select_datetime. DRYed up code a little in select_date. Added tests and documentation. (as partially proposed in this ticket

Reasons

1: Since leading_zero_on_single_digits was an internally used method anyway, I thought we might be more concise as well and use the more programmatic sprintf. Might give a little speed boost, too, because it doesn't have to interpolate the string.

2: This removes a lot of duplication from all affected methods at no obvious cost. The only thing one might consider to be negative is that the new method can't be used with select_month for obvious reasons which might lead to some confusion as to why this method is handled differently. Then again, most users don't care too much about the internal implementation.

3: See the linked ticket.

Documentation

I've added documentation for all changes that affect the public API.

Tests

1: I had to modify the tests because they also used the removed method internally. Shouldn't make any difference, though.

2: All existing tests pass.

3: All existing tests pass. Also added two new ones to make sure that all separators work as expected.

Reported by Clemens Kofler · July 21st, 2008 @ 01:32 AM

State: resolved
Milestone: 2.x
Assigned to: josh josh
Importance: none

Activity

  1. josh
    josh
    • State changed from new to open
    • Tag changed from actionpack, doc, enhancement, helper, patch, refactoring, tests to actionpack, patch, refactoring

    Nice writeup. Will this close #606 Inconsistent functionality in date_helper.rb too?

    July 21st, 2008 @ 06:23 PM

  2. josh
    josh

    I think we are going start namespacing the private methods in helpers. So "build_options" becomes "_date_build_options".

    However, I'd really like to start looking into some more presenter patterns for this stuff. Something like this. 

    DateSelector.new(date, options).select_day

    July 21st, 2008 @ 06:55 PM

  3. Repository
    Repository
    • State changed from open to resolved

    (from [ff9f6fcc75526d9fd89be834982dec8624c909c5]) Refactor DateHelper and improve test coverage [#665 state:resolved]

    Signed-off-by: Joshua Peek

    http://github.com/rails/rails/co...

    July 21st, 2008 @ 07:06 PM

  4. Clemens Kofler
    Clemens Kofler

    Josh:

    This doesn't close #606 Inconsistent functionality in date_helper.rb just yet because the object-related methods still don't support the separators.

    I like the approach with the Presenter pattern. It seems that the InstanceTag approach that's currently used by the object-related methods is a similar approach but IMO it tries to do too many jobs at once. It'd be great to have all date helper related functionality in one Presenter class and then have all helper methods, both "standard" and object-related ones, access the same functionality. I'll look into that and keep you posted.

    July 21st, 2008 @ 10:47 PM