lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Steven Rowe (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (LUCENE-1736) DateTools.java general improvements
Date Mon, 06 Jun 2011 18:38:59 GMT

    [ https://issues.apache.org/jira/browse/LUCENE-1736?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13045018#comment-13045018
] 

Steven Rowe commented on LUCENE-1736:
-------------------------------------

David, I tried to apply your patch, but it failed (using "patch") - maybe partly because the
patch is in git format?  I don't use git, so it's tough for me to review issues with git formatted
patches.

I went and looked at the DateTools.java and compared it to your patch, and things have changed
significantly since you wrote the patch, especially as a result of Uwe's changes from LUCENE-1472.
 Could you take a look at that issue, see if it resolves (enough of) the problems you addressed
in your patch to make this issue now obsolete?  If not, could you regenerate the patch (diff/patch
format please)?

> DateTools.java general improvements
> -----------------------------------
>
>                 Key: LUCENE-1736
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1736
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: core/index
>    Affects Versions: 2.9
>            Reporter: David Smiley
>            Priority: Minor
>             Fix For: 4.0
>
>         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.
For more information on JIRA, see: http://www.atlassian.com/software/jira

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org


Mime
View raw message