[jira] [Updated] (LUCENE-1736) DateTools.java general improvements
[ https://issues.apache.org/jira/browse/LUCENE-1736?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] David Smiley updated LUCENE-1736: - Attachment: LUCENE-1736_DateTools_improvements.patch This is an updated patch. * The former DateFormats class was used as a value in a ThreadLocal which isn't a good idea as it hampers class reloading. * Improvements to a switch statement to benefit from fall-through. * Removed a pointless conversion to Calendar in timeToString() * Moved functionality to Resolution enum, and used arrays of Resolutions indexed by format length instead of large if-else or switch blocks for format parse. The ramification is 48 fewer lines of code. 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: LUCENE-1736_DateTools_improvements.patch, 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Updated] (LUCENE-1736) DateTools.java general improvements
[ https://issues.apache.org/jira/browse/LUCENE-1736?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Steven Rowe updated LUCENE-1736: Attachment: LUCENE-1736.patch David, this is your patch with a CHANGES.txt entry and a couple of comments added (for javadocs next to the two imports that are javadocs-only; and formatLen spelled out over the shared format string). Nice improvements. All tests pass. I plan on committing shortly. 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 Assignee: Steven Rowe Priority: Minor Fix For: 4.0 Attachments: LUCENE-1736.patch, LUCENE-1736_DateTools_improvements.patch, 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] Updated: (LUCENE-1736) DateTools.java general improvements
[ 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: java-dev-unsubscr...@lucene.apache.org For additional commands, e-mail: java-dev-h...@lucene.apache.org
[jira] Updated: (LUCENE-1736) DateTools.java general improvements
[ 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: java-dev-unsubscr...@lucene.apache.org For additional commands, e-mail: java-dev-h...@lucene.apache.org
