commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "James Carman" <ja...@carmanconsulting.com>
Subject Re: svn commit: r628021 - in /commons/proper/configuration/branches/configuration2_experimental: src/main/java/org/apache/commons/configuration2/plist/ src/test/java/org/apache/commons/configuration2/plist/ xdocs/
Date Fri, 15 Feb 2008 17:18:11 GMT
Perhaps you could refactor your class a bit to make it more
unit-testable.  I've found rare occasions in the past where I had to
expose a member variable to test cases, but for the most part, I could
work around it by redesigning my class a bit.  Is that possible in
this case?

On 2/15/08, sebb <sebbaz@gmail.com> wrote:
> On 15/02/2008, Emmanuel Bourg <ebourg@apache.org> wrote:
>  > sebb a écrit :
>  >
>  >
>  >  > The static DateFormat is not private, so the synchronization is not guaranteed.
>  >
>  >
>  > I'll make it private. I added a comment mentioning that it must be
>  >  synchronized.
>  >
>  >
>  >
>  >  > Likewise the instance DateFormat.
>  >  >
>  >  > Indeed that is worse, since the format is temporarily changed by the
>  >  > private method - even if the field is made private, the method(s) that
>  >  > use it are potentially thread-hostile.
>  >  >
>  >  > Seems to me that the DateFormats should both be private.
>  >
>  >
>  > I left the instance DateFormat package private only to access it from
>  >  the test cases. I'll synchronize its use as well.
>  >
>
>  The instance field ought to have a comment to say why it is package
>  protected,  and to point out that any multi-threaded use must be
>  synchronized on DATE_FORMAT. The testcase may need to be updated
>  accordingly.
>
>  Should probably add an @GuardedBy() annotation as well.
>
>
>  >
>  >
>  >  >> The package name has changed, is it still necessary to change the UID
?
>  >  >>
>  >  >
>  >  > As far as I can tell the package name was not changed in this update,
>  >  > but if the previous version of the class was never deployed then it's
>  >  > probably not necessary.
>  >
>  >
>  > Indeed, it's a newly created experimental branch.
>  >
>  >
>  >
>  >  >> FastDateFormat could be useful to simplify the code on the 1.x branch,
>  >  >>  but for the 2.x code base I think SimpleDateFormat is good enough.
>  >  >
>  >  > I don't follow that reasoning.
>  >
>  >
>  > On the 1.x branch we were parsing a timezone with a SimpleDateFormat,
>  >  since this was not supported by Java 1.3 Oliver implemented an
>  >  alternative date parser. FastDateFormat could probably replace this
>  >  implementation since it supports the timezone and works on Java 1.3.
>  >
>  >  On the 2.x branch Java 5 is the minimum requirement, so the Java 1.3
>  >  compatibility of FastDateFormat is not a compelling reason to adopt it.
>  >
>
>  From the Javadoc:
>
>  "FastDateFormat is a fast and thread-safe version of SimpleDateFormat."
>
>  This is why I suggested using it, not because it is 1.3 compatible.
>
>  >
>  >  Emmanuel Bourg
>  >
>  >
>  >
>  >  ---------------------------------------------------------------------
>  >  To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>  >  For additional commands, e-mail: dev-help@commons.apache.org
>  >
>  >
>
>  ---------------------------------------------------------------------
>  To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>  For additional commands, e-mail: dev-help@commons.apache.org
>
>

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


Mime
View raw message