implement FileSystemStyleCache code review comments
---------------------------------------------------
Key: TRINIDAD-1460
URL: https://issues.apache.org/jira/browse/TRINIDAD-1460
Project: MyFaces Trinidad
Issue Type: Improvement
Components: Skinning
Reporter: Jeanne Waldman
Priority: Minor
Code review comments from Blake Sullivan for FileSystemStyleCache:
in private StyleSheetDocument _getStyleSheetDocument(StyleContext context)
this part:
// Otherwise, we create the StyleSheetDocument now
document = createStyleSheetDocument(context);
// If we weren't able to create the StyleSheetDocument,
// use a non-null placeholder
if (document == null)
document = _EMPTY_DOCUMENT;
// Save the document
if (_document == null)
_document = document;
// Re-initialize our Array of namespace prefixes that are in the selectors
// Re-initialize our Map of short style class names
_namespacePrefixes = _getNamespacePrefixes(context, _document);
_shortStyleClassMap = _getShortStyleClassMap(context, _document,
_namespacePrefixes);
Should probably be in a synchronized block
_document, _namespacePrefixes, and _shortStyleClassMap should be volatile.
comment about not needing to use Hashtable and HashMap being OK is
incorrect--_createEntry() which assigns values to _cache and _entryCache is
called without a lock and these Maps are read without a lock. _cache and
_entryCache should probably be ConcurrentHashMaps.
private static class Key
Might as well make the class final as well
Key.hashCode()--hash algorithm is a little weak, since it doesn't spread the
bits out enough. Effective Java's version would be better
Key should calculate its hashCode on initialization because:
1) The comment about not needing to synchronize is wrong
2) The very first thing we do is perform a get on the key, so there is no
performance benefit to not calculating the hashCode aggressively
All fields should be final, making a nice immutable instance, which we should
document as such.
The Entry class should also be documented as thread-safe, though, in this case,
it is only because the List of URIs is implemented as an ArrayList and the List
isn't modified after the Entry class gets the list. We will need to document
this restriction unless we actually copy the list of URIs when creating the
Entry.
private static class DerivationKey
Pretty much the same issues as Key--make the class final, make the fields
final, aggressively calculate the hashCode, fix the hashCode implementation,
document the class as immutable.
private DerivationKey _getDerivationKey(
The whole synchronization thing is bogus--the set of style sheets on the
StyleSheetDocument is immutable, so _copyIterator() should be removed and the
function written as:
// Entries with the same style sheet derivation are compatible.
// Get the style sheet derivation list.
Iterator<StyleSheetNode> styleSheetNodes = document.getStyleSheets(context);
// =-= bts note that we could also make a constant private static final
EMPTY_STYLE_SHEET_NODES =
// new StyleSheetNode[0]; which would be better.
StyleSheetNode[] styleSheets = new StyleSheetNode[0];
// if we have any style sheets, copy them into an array of the correct type
if (styleSheetNodes.hasNext())
{
List<StyleSheetNode> styleSheetNodeList =
CollectionUtils.arrayList(styleSheetNodes);
styleSheets = styleSheetNodeList.toArray(styleSheets);
}
// Create a key out of the style sheet derivation list
return new DerivationKey(context, styleSheets);
_getStyleContextResolvedStyles
replace:
List<StyleNode> v = new ArrayList<StyleNode>();
while (e.hasNext())
v.add(e.next());
return v.toArray(new StyleNode[v.size()]);
with
List<StyleNode> v = CollectionUtils.arrayList(e);
return v.toArray(new StyleNode[v.size()]);
dd
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
