[
http://issues.apache.org/jira/browse/ADFFACES-15?page=comments#action_12414203
]
Adam Winer commented on ADFFACES-15:
------------------------------------
Code review on the patch:
General:
- All code must have an Apache license, not an Oracle license; patches
with non-Apache licenses cannot be accepted.
- Code does not follow ADF Faces coding conventions:
-opening braces on new lines
-two-space indentation
-private methods start with underscores
- Import all classes (no java.io.Serializable, java.net.URL, etc. in code)
- Never call ex.printStackTrace(). _LOG.severe() is sufficient.
- Implementation details (like describing the set of hashmaps created
to implement a menu model) should be left out of the Javadoc,
and moved into "//"-style comments inside the class.
- Do not access any Servlet APIs when ExternalContext is sufficient
(as it is in all usages herein)
XmlMenuModel:
- Comments like "our Xdk" aren't appropriate for Apache codebases
- Do not use code from sun.* (sun.misc.Service)
- What is the use of createNotVisible(), putCachedMenuList(),
getCachedMenuList()?
- Don't call Map.get() and put() with Object.hashCode(). Just use the object
directly.
- _rootModelMap is not a threadsafe useage, since it will be shared across all
requests
(and potentially multiple webapps). I'm not sure what its purpose is in
general; there's
likely a simpler way to achieve what you're going after (maybe a
ThreadLocal?)
- For MenuContentHandler, please give "s1", "s2", "s3" more descriptive names.
CombinationMenuModel:
- Is this class useful on its own? If not, maybe it could be merged into
XMLMenuModel,
or at least made package-private?
- Most of the public methods do not appear to need to be public, and could be
private,
package-private, or protected (use the most restrictive that works).
MenuContentHandlerImpl:
- _handlerId can be more simply (and reliably) computed with
System.identityHashCode().
- loadBundle() is only going to get called once, with the Locale in place for
the request
when this file is parsed, so I don't think this will properly be retranslated
on each request.
- BundleMap.get() shouldn't return "MISSING:", it should return "!!!" + key +
"!!!" to follow the
JSF 1.2 spec.
MenuUtils:
- Should be package-private
MenuNode:
- Rename to getIcon(), setIcon(); if the tree bug is still present, please
file an issue.
- joinLabelAndAcessKey(), splitLabelAndAcessKey() are mispelled (AccessKey)
Could you address these issues and post a new patch?
> Added XMLMenuModel
> ------------------
>
> Key: ADFFACES-15
> URL: http://issues.apache.org/jira/browse/ADFFACES-15
> Project: MyFaces ADF-Faces
> Type: New Feature
> Environment: software platform
> Reporter: Gary Kind
> Attachments: trunk.patch
>
> Additions to MyFaces ADF-Faces to allow easy creation of menus (navigation
> and tabbed navigation) for .jspx pages using ADF-Faces UI components. Menus
> are specified in XML Metadata files. In the .jspx page menu items are
> af:commandNavigationItems nodestamped inside of an af:page component. Menu
> Model bean that processes the metadata files, along with the
> navigation-rules/cases are specified in the faces-config.xml file.
--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
http://www.atlassian.com/software/jira
