[jira] Created: (LUCENE-1736) DateTools.java general improvements

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[jira] Created: (LUCENE-1736) DateTools.java general improvements

JIRA jira@apache.org
DateTools.java general improvements
-----------------------------------

                 Key: LUCENE-1736
                 URL: https://issues.apache.org/jira/browse/LUCENE-1736
             Project: Lucene - Java
          Issue Type: Improvement
    Affects Versions: 2.9
            Reporter: David Smiley
            Priority: Minor
         Attachments: cleanerDateTools.patch

Applying the attached patch shows the improvements to DateTools.java that I think should be done. All logic that does anything at all is moved to instance methods of the inner class Resolution. I argue this is more object-oriented.

1. In cases where Resolution is an argument to the method, I can simply invoke the appropriate call on the Resolution object. Formerly there was a big branch if/else.
2. Instead of "synchronized" being used seemingly everywhere, synchronized is used to sync on the object that is not threadsafe, be it a DateFormat or Calendar instance.
3. Since different DateFormat and Calendar instances are created per-Resolution, there is now less lock contention since threads using different resolutions will not use the same locks.
4. The old implementation of timeToString rounded the time before formatting it. That's unnecessary since the format only includes the resolution desired.
5. round() now uses a switch statement that benefits from fall-through (no break).

Another debatable improvement that could be made is putting the resolution instances into an array indexed by format length. This would mean I could remove the switch in lookupResolutionByLength() and avoid the length constants there. Maybe that would be a bit too over-engineered when the switch is fine.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (LUCENE-1736) DateTools.java general improvements

JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/LUCENE-1736?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

David Smiley updated LUCENE-1736:
---------------------------------

    Attachment: cleanerDateTools.patch

> DateTools.java general improvements
> -----------------------------------
>
>                 Key: LUCENE-1736
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1736
>             Project: Lucene - Java
>          Issue Type: Improvement
>    Affects Versions: 2.9
>            Reporter: David Smiley
>            Priority: Minor
>         Attachments: cleanerDateTools.patch
>
>
> Applying the attached patch shows the improvements to DateTools.java that I think should be done. All logic that does anything at all is moved to instance methods of the inner class Resolution. I argue this is more object-oriented.
> 1. In cases where Resolution is an argument to the method, I can simply invoke the appropriate call on the Resolution object. Formerly there was a big branch if/else.
> 2. Instead of "synchronized" being used seemingly everywhere, synchronized is used to sync on the object that is not threadsafe, be it a DateFormat or Calendar instance.
> 3. Since different DateFormat and Calendar instances are created per-Resolution, there is now less lock contention since threads using different resolutions will not use the same locks.
> 4. The old implementation of timeToString rounded the time before formatting it. That's unnecessary since the format only includes the resolution desired.
> 5. round() now uses a switch statement that benefits from fall-through (no break).
> Another debatable improvement that could be made is putting the resolution instances into an array indexed by format length. This would mean I could remove the switch in lookupResolutionByLength() and avoid the length constants there. Maybe that would be a bit too over-engineered when the switch is fine.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (LUCENE-1736) DateTools.java general improvements

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/LUCENE-1736?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Mark Miller updated LUCENE-1736:
--------------------------------

    Fix Version/s: 3.1

> DateTools.java general improvements
> -----------------------------------
>
>                 Key: LUCENE-1736
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1736
>             Project: Lucene - Java
>          Issue Type: Improvement
>    Affects Versions: 2.9
>            Reporter: David Smiley
>            Priority: Minor
>             Fix For: 3.1
>
>         Attachments: cleanerDateTools.patch
>
>
> Applying the attached patch shows the improvements to DateTools.java that I think should be done. All logic that does anything at all is moved to instance methods of the inner class Resolution. I argue this is more object-oriented.
> 1. In cases where Resolution is an argument to the method, I can simply invoke the appropriate call on the Resolution object. Formerly there was a big branch if/else.
> 2. Instead of "synchronized" being used seemingly everywhere, synchronized is used to sync on the object that is not threadsafe, be it a DateFormat or Calendar instance.
> 3. Since different DateFormat and Calendar instances are created per-Resolution, there is now less lock contention since threads using different resolutions will not use the same locks.
> 4. The old implementation of timeToString rounded the time before formatting it. That's unnecessary since the format only includes the resolution desired.
> 5. round() now uses a switch statement that benefits from fall-through (no break).
> Another debatable improvement that could be made is putting the resolution instances into an array indexed by format length. This would mean I could remove the switch in lookupResolutionByLength() and avoid the length constants there. Maybe that would be a bit too over-engineered when the switch is fine.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]