[jira] [Commented] (LANG-980) DurationFormatUtils uses == for comparing objects

["Sebb (JIRA)" <ji...@apache.org>]

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

Sebb commented on LANG-980:
---------------------------

It looks like the only object types ever added as Token values are the 1-char Strings for the duration ids and StringBuilder objects. The Unit test code adds a Number, but the code does not use this feature. Now the StringBuilder objects are only ever used after conversion to a String, so it seems odd to store the StringBuilder rather than the string. I suspect this may be because a 1-char String could be confused with one of the duration ids if it ever got interned.

To avoid repeated use of StringBuffer#toString() whilst still ensuring that duration ids are unique, the Token class could be modified slightly.
The Object value could be replaced with 2 fields: the type (Duration enum) and a string (if relevant). The Duration enum would need to be extended to include STRING - and renamed as it is no longer just a duration.

This would allow further simplification of the code, e.g. in the equals method. The Token constructors could be adjusted to accept onlly String or enum.

> DurationFormatUtils uses == for comparing objects
> -------------------------------------------------
>
>                 Key: LANG-980
>                 URL: https://issues.apache.org/jira/browse/LANG-980
>             Project: Commons Lang
>          Issue Type: Bug
>          Components: lang.time.*
>            Reporter: Sebb
>            Priority: Minor
>
> As reported on the ML, Findbugs complains that == is being used to compare objects in the class DurationFormatUtils.
> These objects are the strings which define the various durations: "y", "M", "d" etc.  These are final static objects (singletons) so the use of == should be OK but it is not good practice.
> One way to avoid the warnings would be to use an Enum for the singleton objects. For example:
> {code}
> enum Duration { YEAR, MONTH, ... }
>     static final ParseObject y = ParseObject.YEAR;
>     static final ParseObject M = ParseObject.MONTH;
> ...
> {code}
> Note: the package protected fields y, M etc are currently needed for the unit tests.
> The above change would then allow the format() method to use a switch statement which would likely be faster than the if chain it has to use now.
> Eliminating the warnings for == which are currently safe would make it obvious if == was used elsewhere in an unsafe way.



--
This message was sent by Atlassian JIRA
(v6.2#6252)