Feed aggregator
CodeSOD: Born Single
Alistair sends us a pretty big blob of code, but it's a blob which touches upon everyone's favorite design pattern: the singleton. It's a lot of Java code, so we're going to take this as chunks. Let's start with the two methods responsible for constructing the object.
The purpose of this code is to parse an XML file, and construct a mapping from a "name" field in the XML to a "batch descriptor".
/** * Instantiates a new batch manager. */ private BatchManager() { try { final XMLReader xmlReader = XMLReaderFactory.createXMLReader(); xmlReader.setContentHandler(this); xmlReader.parse(new InputSource(this.getClass().getClassLoader().getResourceAsStream("templates/" + DOCUMENT))); } catch (final Exception e) { logger.error("Error parsing Batch XML.", e); } } /* * (non-Javadoc) * * @see nz.this.is.absolute.crap.sax.XMLEntity#initChild(java.lang.String, * java.lang.String, java.lang.String, org.xml.sax.Attributes) */ @Override protected ContentHandler initChild(String uri, String localName, String qName, Attributes attributes) throws SAXException { final BatchDescriptor batchDescriptor = new BatchDescriptor(); // put it in the map batchMap.put(attributes.getValue("name"), batchDescriptor); return batchDescriptor; }Here we see a private constructor, which is reasonable for a singleton. It creates a SAX based reader. SAX is event driven- instead of loading the whole document into a DOM, it emits an event as it encounters each new key element in the XML document. It's cumbersome to use, but far more memory efficient, and I'd hardly say this.is.absolute.crap, but whatever.
This code is perfectly reasonable. But do you know what's unreasonable? There's a lot more code, and these are the only things not marked as static. So let's keep going.
// singleton instance so that static batch map can be initialised using // xml /** The Constant singleton. */ @SuppressWarnings("unused") private static final Object singleton = new BatchManager();Wait… why is the singleton object throwing warnings about being unused? And wait a second, what is that comment saying, "so the static batch map can be initalalised"? I saw a batchMap up in the initChild method above, but it can't be…
private static Map<String, BatchDescriptor> batchMap = new HashMap<String, BatchDescriptor>();Oh. Oh no.
/** * Gets the. * * @param batchName * the batch name * * @return the batch descriptor */ public static BatchDescriptor get(String batchName) { return batchMap.get(batchName); } /** * Gets the post to selector name. * * @param batchName * the batch name * * @return the post to selector name */ public static String getPostToSelectorName(String batchName) { final BatchDescriptor batchDescriptor = batchMap.get(batchName); if (batchDescriptor == null) { return null; } return batchDescriptor.getPostTo(); }There are more methods, and I'll share the whole code at the end, but this gives us a taste. Here's what this code is actually doing.
It creates a static Map. static, in this context, means that this instance is shared across all instances of BatchManager.They also create a static instance of BatchManager inside of itself. The constructor of that instance then executes, populating that static Map. Now, when anyone invokes BatchManager.get it will use that static Map to resolve that.
This certainly works, and it offers a certain degree of cleanness in its implementation. A more conventional singleton would have the Map being owned by an instance, and it's just using the singleton convention to ensure there's only a single instance. This version's calling convention is certainly nicer than doing something like BatchManager.getInstance().get(…), but there's just something unholy about this that sticks into me.
I can't say for certain if it's because I just hate Singletons, or if it's this specific abuse of constructors and static members.
This is certainly one of the cases of misusing a singleton- it does not represent something there can be only one of, it's ensuring that an expensive computation is only allowed to be done once. There are better ways to handle that lifecycle. This approach also forces that expensive operation to happen at application startup, instead of being something flexible that can be evaluated lazily. It's not wrong to do this eagerly, but building something that can only do it eagerly is a mistake.
In any case, the full code submission follows:
package nz.this.is.absolute.crap.server.template; import java.io.IOException; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; import java.util.Iterator; import java.util.Map; import java.util.ResourceBundle; import nz.this.is.absolute.crap.KupengaException; import nz.this.is.absolute.crap.SafeComparator; import nz.this.is.absolute.crap.sax.XMLEntity; import nz.this.is.absolute.crap.selector.Selector; import nz.this.is.absolute.crap.selector.SelectorItem; import nz.this.is.absolute.crap.server.BatchValidator; import nz.this.is.absolute.crap.server.Validatable; import nz.this.is.absolute.crap.server.ValidationException; import nz.this.is.absolute.crap.server.business.BusinessObject; import nz.this.is.absolute.crap.server.database.EntityHandler; import nz.this.is.absolute.crap.server.database.SQLEntityHandler; import org.apache.log4j.Logger; import org.xml.sax.Attributes; import org.xml.sax.ContentHandler; import org.xml.sax.InputSource; import org.xml.sax.SAXException; import org.xml.sax.XMLReader; import org.xml.sax.helpers.XMLReaderFactory; /** * The Class BatchManager. */ public class BatchManager extends XMLEntity { private static final Logger logger = Logger.getLogger(BatchManager.class); /** The Constant DOCUMENT. */ private final static String DOCUMENT = "Batches.xml"; /** * The Class BatchDescriptor. */ public class BatchDescriptor extends XMLEntity { /** The batchSelectors. */ private final Collection<String> batchSelectors = new ArrayList<String>(); /** The dependentCollections. */ private final Collection<String> dependentCollections = new ArrayList<String>(); /** The directSelectors. */ private final Collection<String> directSelectors = new ArrayList<String>(); /** The postTo. */ private String postTo; /** The properties. */ private final Collection<String> properties = new ArrayList<String>(); /** * Gets the batch selectors iterator. * * @return the batch selectors iterator */ public Iterator<String> getBatchSelectorsIterator() { return this.batchSelectors.iterator(); } /** * Gets the dependent collections iterator. * * @return the dependent collections iterator */ public Iterator<String> getDependentCollectionsIterator() { return this.dependentCollections.iterator(); } /** * Gets the post to. * * @return the post to */ public String getPostTo() { return this.postTo; } /** * Gets the post to business object. * * @param businessObject * the business object * @param postHandler * the post handler * * @return the post to business object * * @throws ValidationException * the validation exception */ private BusinessObject getPostToBusinessObject( BusinessObject businessObject, EntityHandler postHandler) throws ValidationException { if (this.postTo == null) { return null; } final BusinessObject postToBusinessObject = businessObject .getBusinessObjectFromMap(this.postTo, postHandler); // copy properties for (final String propertyName : this.properties) { String postToPropertyName; if ("postToStatus".equals(propertyName)) { // status field on batch entity refers to the batch entity // itself // so postToStatus is used for updating the status property // of the postToBusinessObject itself postToPropertyName = "status"; } else { postToPropertyName = propertyName; } final SelectorItem destinationItem = postToBusinessObject .find(postToPropertyName); if (destinationItem != null) { final Object oldValue = destinationItem.getValue(); final Object newValue = businessObject.get(propertyName); if (SafeComparator.areDifferent(oldValue, newValue)) { destinationItem.setValue(newValue); } } } // copy direct selectors for (final String selectorName : this.directSelectors) { final SelectorItem destinationItem = postToBusinessObject .find(selectorName); if (destinationItem != null) { // get the old and new values for the selectors Selector oldSelector = (Selector) destinationItem .getValue(); Selector newSelector = (Selector) businessObject .get(selectorName); // strip them down to bare identifiers for comparison if (oldSelector != null) { oldSelector = oldSelector.getAsIdentifier(); } if (newSelector != null) { newSelector = newSelector.getAsIdentifier(); } // if they're different then update if (SafeComparator.areDifferent(oldSelector, newSelector)) { destinationItem.setValue(newSelector); } } } // copy batch selectors for (final String batchSelectorName : this.batchSelectors) { final Selector batchSelector = (Selector) businessObject .get(batchSelectorName); if (batchSelector == null) { throw new ValidationException( "\"PostTo\" selector missing."); } final BusinessObject batchObject = postHandler .find(batchSelector); if (batchObject != null) { // get the postTo selector for the batch object we depend on final BatchDescriptor batchDescriptor = batchMap .get(batchObject.getName()); if (batchDescriptor.postTo != null && postToBusinessObject .containsKey(batchDescriptor.postTo)) { final Selector realSelector = batchObject .getBusinessObjectFromMap( batchDescriptor.postTo, postHandler); postToBusinessObject.put(batchDescriptor.postTo, realSelector); } } } businessObject.put(this.postTo, postToBusinessObject); return postToBusinessObject; } /* * (non-Javadoc) * * @see * nz.this.is.absolute.crap.sax.XMLEntity#initChild(java.lang.String, * java.lang.String, java.lang.String, org.xml.sax.Attributes) */ @Override protected ContentHandler initChild(String uri, String localName, String qName, Attributes attributes) throws SAXException { if ("Properties".equals(qName)) { return new XMLEntity() { @Override protected ContentHandler initChild(String uri, String localName, String qName, Attributes attributes) throws SAXException { BatchDescriptor.this.properties.add(attributes .getValue("name")); return null; } }; } else if ("DirectSelectors".equals(qName)) { return new XMLEntity() { @Override protected ContentHandler initChild(String uri, String localName, String qName, Attributes attributes) throws SAXException { BatchDescriptor.this.directSelectors.add(attributes .getValue("name")); return null; } }; } else if ("BatchSelectors".equals(qName)) { return new XMLEntity() { @Override protected ContentHandler initChild(String uri, String localName, String qName, Attributes attributes) throws SAXException { BatchDescriptor.this.batchSelectors.add(attributes .getValue("name")); return null; } }; } else if ("PostTo".equals(qName)) { return new XMLEntity() { @Override protected ContentHandler initChild(String uri, String localName, String qName, Attributes attributes) throws SAXException { BatchDescriptor.this.postTo = attributes .getValue("name"); return null; } }; } else if ("DependentCollections".equals(qName)) { return new XMLEntity() { @Override protected ContentHandler initChild(String uri, String localName, String qName, Attributes attributes) throws SAXException { BatchDescriptor.this.dependentCollections .add(attributes.getValue("name")); return null; } }; } return null; } } /** The batchMap. */ private static Map<String, BatchDescriptor> batchMap = new HashMap<String, BatchDescriptor>(); /** * Gets the. * * @param batchName * the batch name * * @return the batch descriptor */ public static BatchDescriptor get(String batchName) { return batchMap.get(batchName); } /** * Gets the post to selector name. * * @param batchName * the batch name * * @return the post to selector name */ public static String getPostToSelectorName(String batchName) { final BatchDescriptor batchDescriptor = batchMap.get(batchName); if (batchDescriptor == null) { return null; } return batchDescriptor.getPostTo(); } // singleton instance so that static batch map can be initialised using // xml /** The Constant singleton. */ @SuppressWarnings("unused") private static final Object singleton = new BatchManager(); /** * Post. * * @param businessObject * the business object * * @throws Exception * the exception */ public static void post(BusinessObject businessObject) throws Exception { // validate the batch root object only - it can validate the rest if it // needs to if (businessObject instanceof Validatable) { if (!BatchValidator.validate(businessObject)) { logger.warn(String.format("Validating %s failed", businessObject.getClass().getSimpleName())); throw new ValidationException( "Batch did not validate - it was not posted"); } ((Validatable) businessObject).validator().prepareToPost(); } final SQLEntityHandler postHandler = new SQLEntityHandler(true); final Iterator<BusinessObject> batchIterator = new BatchIterator( businessObject, null, postHandler); // iterate through batch again posting each object try { while (batchIterator.hasNext()) { post(batchIterator.next(), postHandler); } postHandler.commit(); } catch (final Exception e) { logger.error("Exception occurred while posting batches", e); // something went wrong postHandler.rollback(); throw e; } return; } /** * Post. * * @param businessObject * the business object * @param postHandler * the post handler * * @throws KupengaException * the kupenga exception */ private static void post(BusinessObject businessObject, EntityHandler postHandler) throws KupengaException { if (businessObject == null) { return; } if (Boolean.TRUE.equals(businessObject.get("posted"))) { return; } final BatchDescriptor batchDescriptor = batchMap.get(businessObject .getName()); final BusinessObject postToBusinessObject = batchDescriptor .getPostToBusinessObject(businessObject, postHandler); if (postToBusinessObject != null) { postToBusinessObject.save(postHandler); } businessObject.setItemValue("posted", Boolean.TRUE); businessObject.save(postHandler); } /** * Instantiates a new batch manager. */ private BatchManager() { try { final XMLReader xmlReader = XMLReaderFactory.createXMLReader(); xmlReader.setContentHandler(this); xmlReader.parse(new InputSource(this.getClass().getClassLoader().getResourceAsStream("templates/" + DOCUMENT))); } catch (final Exception e) { logger.error("Error parsing Batch XML.", e); } } /* * (non-Javadoc) * * @see nz.this.is.absolute.crap.sax.XMLEntity#initChild(java.lang.String, * java.lang.String, java.lang.String, org.xml.sax.Attributes) */ @Override protected ContentHandler initChild(String uri, String localName, String qName, Attributes attributes) throws SAXException { final BatchDescriptor batchDescriptor = new BatchDescriptor(); // put it in the map batchMap.put(attributes.getValue("name"), batchDescriptor); return batchDescriptor; } } .comment { border: none; } [Advertisement] Keep all your packages and Docker containers in one place, scan for vulnerabilities, and control who can access different feeds. ProGet installs in minutes and has a powerful free version with a lot of great features that you can upgrade when ready.Learn more.California Set To Become First US State To Manage Power Outages With AI
Read more of this story at Slashdot.
Saudi Arabia Asks Consultants To Reassess Feasibility of 'The Line' Megaproject
Read more of this story at Slashdot.
Microsoft Has a New Trick To Improve Laptop Battery Life On Windows
Read more of this story at Slashdot.
US Defense Department Awards Contracts To Google, xAI
Read more of this story at Slashdot.
Meta's Superintelligence Lab Considers Shift To Closed AI Model
Read more of this story at Slashdot.
Five EU States To Test Age Verification App To Protect Children
Read more of this story at Slashdot.
China's Moonshot Launches Free AI Model Kimi K2 That Outperforms GPT-4 In Key Benchmarks
Read more of this story at Slashdot.
Two Guys Hated Using Comcast, So They Built Their Own Fiber ISP
Read more of this story at Slashdot.
Apple Faces Calls To Reboot AI Strategy With Shares Slumping
Read more of this story at Slashdot.
Armagh Observatory Marks 230 Years of Recording Weather
Read more of this story at Slashdot.
Are a Few People Ruining the Internet For the Rest of Us?
Read more of this story at Slashdot.
MoonPay Executives May Have Sent $250,000 To Nigerian Scammer, DoJ Filing Suggests
Read more of this story at Slashdot.
Cognition AI Buys Windsurf as AI Frenzy Escalates
Read more of this story at Slashdot.
Air India Chief Says Preliminary Crash Report Raises Fresh Questions
Read more of this story at Slashdot.
Quality of Scientific Papers Questioned as Academics 'Overwhelmed' By the Millions Published
Read more of this story at Slashdot.
Google Plans To Combine ChromeOS and Android Into Single Platform
Read more of this story at Slashdot.
Zuckerberg Pledges Hundreds of Billions For AI Data Centers in Superintelligence Push
Read more of this story at Slashdot.
BulletVPN Shuts Down, Killing Lifetime Members' Subscriptions
Read more of this story at Slashdot.
Bay Area Restaurants Are Vetting Your Social Media Before You Even Walk In
Read more of this story at Slashdot.