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 batchMap = new HashMap();

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 batchSelectors = new ArrayList();

		/** The dependentCollections. */
		private final Collection dependentCollections = new ArrayList();

		/** The directSelectors. */
		private final Collection directSelectors = new ArrayList();

		/** The postTo. */
		private String postTo;

		/** The properties. */
		private final Collection properties = new ArrayList();

		/**
		 * Gets the batch selectors iterator.
		 * 
		 * @return the batch selectors iterator
		 */
		public Iterator getBatchSelectorsIterator() {
			return this.batchSelectors.iterator();
		}

		/**
		 * Gets the dependent collections iterator.
		 * 
		 * @return the dependent collections iterator
		 */
		public Iterator 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 batchMap = new HashMap();

	/**
	 * 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 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;
	}
}

[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.