This project is archived and is in readonly mode.

#665 ✓resolved
Clemens Kofler

DateHelper patch

Reported by Clemens Kofler | July 21st, 2008 @ 01:32 AM | in 2.x

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.

Comments and changes to this ticket

  • josh

    josh July 21st, 2008 @ 06:23 PM

    • 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 too?

  • josh

    josh July 21st, 2008 @ 06:55 PM

    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
  • Repository

    Repository July 21st, 2008 @ 07:06 PM

    • 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...

  • Clemens Kofler

    Clemens Kofler July 21st, 2008 @ 10:47 PM

    Josh:

    This doesn't close #606 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.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Attachments

Tags

Referenced by

Pages