>Replace with MAP

>In Part I, II, and III of the Replace Nested Conditional with Guard Clause series of posts. Several readers pointed out that the refactoring could be done with a MAP instead of a series of if then else statements. Since I recently implemented Syntax Coloring for XSL statements (i.e. they can be colored differently than their embedded xml content they wrap), I decided to tackle this refactoring.

Here is the bit of code as a reference as to where I started, basically following a similar model, but I wasn’t happy with it and how it was to be maintained. This was code I wrote over the last couple of days to implement the syntax coloring for XSL Tools. The key to any type of refactoring though is having some unit tests to help verify that the external behavior is still the same.


private TextAttribute getXSLAttribute(String type) {

if ((type == DOMRegionContext.XML_TAG_OPEN)
|| (type == DOMRegionContext.XML_END_TAG_OPEN)
|| (type == DOMRegionContext.XML_TAG_CLOSE)
|| (type == DOMRegionContext.XML_EMPTY_TAG_CLOSE)) {
return (TextAttribute) getXSLTextAttributes().get(
IStyleConstantsXSL.TAG_BORDER);
}

if (type == DOMRegionContext.XML_TAG_ATTRIBUTE_NAME) {
return (TextAttribute) getXSLTextAttributes().get(
IStyleConstantsXSL.TAG_ATTRIBUTE_NAME);
}

if (type == DOMRegionContext.XML_TAG_NAME) {
return (TextAttribute) getXSLTextAttributes().get(
IStyleConstantsXSL.TAG_NAME);
}

if ((type == DOMRegionContext.XML_TAG_ATTRIBUTE_VALUE)) {
return (TextAttribute) getXSLTextAttributes().get(
IStyleConstantsXSL.TAG_ATTRIBUTE_VALUE);
}

return null;
}

protected HashMap<String,TextAttribute> getXSLTextAttributes() {
if (xslTextAttributes == null) {
xslTextAttributes = new HashMap<String,TextAttribute>();
loadXSLColors();
}
return xslTextAttributes;
}


protected void loadXSLColors() {
addXSLTextAttribute(IStyleConstantsXSL.TAG_NAME);
addXSLTextAttribute(IStyleConstantsXSL.TAG_BORDER);
addXSLTextAttribute(IStyleConstantsXSL.TAG_ATTRIBUTE_NAME);
addXSLTextAttribute(IStyleConstantsXSL.TAG_ATTRIBUTE_VALUE);
}

Like I said in Part III, the problem I have with some if-then-else statements is that they can become very large very quickly. And I do agree that depending on the size of the method and it’s complexity, one should try to limit the number of returns (i.e. One Entry One Exit), but I’m not a stickler to that rule. So with a little bit of work, here is what the refactored code looks like using the MAP interface, specially a HashMap.


private TextAttribute getXSLAttribute(String type) {
TextAttribute attribute = null;
HashMap<String,String> regionMap = getXSLRegions();
HashMap<String,TextAttribute> textAttributes = getXSLTextAttributes();

if (regionMap.containsKey(type)) {
attribute = textAttributes.get(regionMap.get(type));
}
return attribute;
}

protected HashMap<String,String> getXSLRegions() {
if (xslRegionMap == null) {
xslRegionMap = new HashMap<String,String>();
loadXSLRegions();
}
return xslRegionMap;
}

protected void loadXSLRegions() {
xslRegionMap.put(DOMRegionContext.XML_TAG_OPEN, IStyleConstantsXSL.TAG_BORDER);
xslRegionMap.put(DOMRegionContext.XML_END_TAG_OPEN, IStyleConstantsXSL.TAG_BORDER);
xslRegionMap.put(DOMRegionContext.XML_TAG_CLOSE, IStyleConstantsXSL.TAG_BORDER);
xslRegionMap.put(DOMRegionContext.XML_EMPTY_TAG_CLOSE, IStyleConstantsXSL.TAG_BORDER);
xslRegionMap.put(DOMRegionContext.XML_TAG_ATTRIBUTE_NAME, IStyleConstantsXSL.TAG_ATTRIBUTE_NAME);
xslRegionMap.put(DOMRegionContext.XML_TAG_NAME, IStyleConstantsXSL.TAG_NAME);
xslRegionMap.put(DOMRegionContext.XML_TAG_ATTRIBUTE_VALUE, IStyleConstantsXSL.TAG_ATTRIBUTE_VALUE);
}

protected HashMap<String,TextAttribute> getXSLTextAttributes() {
if (xslTextAttributes == null) {
xslTextAttributes = new HashMap<String,TextAttribute>();
loadXSLColors();
}
return xslTextAttributes;
}

protected void loadXSLColors() {
addXSLTextAttribute(IStyleConstantsXSL.TAG_NAME);
addXSLTextAttribute(IStyleConstantsXSL.TAG_BORDER);
addXSLTextAttribute(IStyleConstantsXSL.TAG_ATTRIBUTE_NAME);
addXSLTextAttribute(IStyleConstantsXSL.TAG_ATTRIBUTE_VALUE);
}

So by refactoring the DOMRegion checks into a HashMap, it makes adding new mappings much simpler. Just add a new key,value pair to the loadXSLRegions method, and off you go. As more mappings need to be done, the getXSLAttribute method remains small and manageable.

A couple of other things popped out at me while do this refactoring…I can isolate this code from the mapping code by creating a Mapper class to handle the mapping. I’ll probably end up doing this, as it is now the Class is doing a bit more than I think it should. It’s handling mapping as well as applying Syntax Coloring.

Thanks to everybody that responded to the original postings, it helped me work through some ways to help improve the code I was writing.

Advertisements
This entry was posted in eclipse, refactoring, xml. Bookmark the permalink.

6 Responses to >Replace with MAP

  1. >A small suggestion about using maps. Use get() method with a null check instead of containsKey() and then get(). You’ll save on an extra hash lookup.

  2. >(updated to clarify my synchronization comment)I recommend preventing the race condition that comes along with initializing the map multiple times. There are a few ways to do this; using a dedicated lock on which you synchronize is the least disruptive to your code as-is, and hardly costs anything (since modern VMs hardly pay for synchronizations that don’t collide much — I can’t guarantee that, but IIRC it’s correct, and will be happy to validate.) You could also make the map’s reference volatile (which isn’t quite as fast; you might still get a couple of multiple synchronizations, but ‘volatile’ is broken in VMs earlier than 1.5, so unless your project requires 1.5, don’t use volatile.In the end, if you don’t want to pay the continued price of a synchronization monitor, you can use the the static initializer on demand holder pattern. Bob Lee discusses this a bit as well.But hey, if you’re going to pay the cost of a map lookup, you might as well just add the synchronization monitor,Your refactoring is more readable now, but if I were writing the code I would not introduce a singleton without preventing race conditions.

  3. ijuma says:

    >I would recommend the static initializer on demand holder if this is indeed a singleton. Synchronization is cheap in recent VMs if there is no contention, but is there a guarantee that there’s no contention here? I would not compare the cost of a Map look-up to the ones introduced by a synchronization monitor (the latter affects scalability while the former does not).Two more comments:- If you don’t want the map to be changed after initialisation, wrap it in an unmodifiable map (Collections.unmodifiableMap).- Why return a HashMap instead of Map from the protected methods? No reason to expose the implementation imho.

  4. David Carver says:

    >An update on this refactoring. I took robert’s advice and implemented the static on demand holder pattern. It seems to work well, and I also took Eugene’s advice and removed the containsKey check, since I was returning null anyways if it couldn’t find the key.@ijuma: I will need to be able to have the map be modified. It will need to listen to Property Change events from the preference pages to get the new values and update the map appropriately.Also, I took the opportunity to separate out the mapping duties into their own classes. This leave the Line Style Provider class with just the responsibility of doing the coloring. It no longer has to also mange the mapping duties.

  5. ijuma says:

    >If the map is modified after construction and it’s accessed by multiple threads, you cannot rely on the static initializer on demand holder pattern for thread-safety…

  6. >I agree with @ijuma. If you use a ConcurrentHashMap you the concurrency issue goes away. Alternatively, create a separate class that has a limited API (e.g. initialize, get, add) and synchronize around it.I never quite got my head around concurrency-safety in Eclipse. Maybe there’s a policy that enforces that all mutations and access occurs in the same thread?

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s