[
https://issues.apache.org/jira/browse/TOMAHAWK-1235?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12602771#action_12602771
]
Simon Kitching commented on TOMAHAWK-1235:
------------------------------------------
I agree that splitting out the inner classes here makes things a little more
readable.
I'm not so convinced that moving all the static methods out is useful. How is a
bunch of static methods on a utility class easier than a bunch of static
methods within the HtmlCalendarRenderer better?
So my personal feeling is about -0.1 on applying this patch (if all of the more
specific points below are fixed first). Not completely opposed, but not
convinced it is worth doing - and in particular, the risk of introducing new
bugs for no great benefit.
Specific feedback regarding this specific patch:
(1)
There is significant binary incompatibility. Methods that were previously
public on the HtmlCalendarRenderer class are now on util classes instead.
I don't see this as too serious here; it should have been reasonably clear to
users that the HtmlCalendarRenderer class is not one that would provide a
stable API. Of course we *should* try to avoid breaking compatibility, but this
case seems ok to me. Best to send an email to the dev list, though, seeing if
anyone objects to changing the API on this class just for the purposes of
"tidiness".
(2)
There are a lot more public methods and classes than there used to be.
Previously almost all these helper methods were private; this patch makes them
all public. But every time we publish a public method, it is another binary
compatibility issue for the future.
I would like at a *minimum* to see comments on all of the new util classes
stating that this is NOT a public API and nothing in these classes can be
depended on to remain stable between releases.
Alternately, it would be possible to reduce this by leaving these new utils
classes in the parent dir, and making them package scoped. But it appears that
a few util methods are used by the HtmlDateRenderer class which is in another
package. Are they really used by both renderer classes? If so, then maybe a
public utils class could be created to hold just those shared methods?
(3)
In encodeEnd before RendererUtile.checkParamValidity is invoked you've:
(1) inserted a cast of the component param, and
(2) dereferenced the facesContext param.
But checkParamValidity is specifically designed to check those two params for
sanity *before* they are used.
This occurs in other methods too
(4)
Previously private constants have been made public, eg
RESOURCE_NONE
JAVASCRIPT_ENCODED
And a comment (that I added) on RESOURCE_NONE was removed.
(5)
Your patch includes tabs. Please use spaces only.
(6)
It is considered good Java style for variables to be declared where they are
first assigned. But your patch changes some code that does this, and instead
the variables are now mostly declared at the top of the function, and
initialised to null.
Please put this back as it was, eg in encodeEnd:
In current code:
Calendar timeKeeper = Calendar.getInstance(currentLocale);
In proposed patch:
Calendar timeKeeper = null;
....
timeKeeper = Calendar.getInstance(currentLocale);
And in general, initialising variables to null should be avoided. The java
compiler has a nice feature that tracks whether a variable has been initialised
before it is used, and reports an error if not. But if variables are
initialised to null at the top of the function, this nice feature is completely
ineffective.
> Refactoring Calendar component
> ------------------------------
>
> Key: TOMAHAWK-1235
> URL: https://issues.apache.org/jira/browse/TOMAHAWK-1235
> Project: MyFaces Tomahawk
> Issue Type: Improvement
> Components: Calendar
> Affects Versions: 1.1.7-SNAPSHOT
> Reporter: Hazem Saleh
> Fix For: 1.1.7-SNAPSHOT
>
> Attachments: calendar_refactored.patch
>
> Original Estimate: 336h
> Remaining Estimate: 336h
>
> Refactoring Calendar component.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
