First - sorry about the time it took for me to produce this answer. Things are hectic in Scandinavia for certain.
:) All right - here we go. 2010/11/18 Kristian Rosenvold <[email protected]> [znip] > As for the patterns discussion nothing specific is being said, so I'm > eagerly anticipating specifics. > > I got half an hour this weekend, so did some dissection of the first class one encounters within the Maven model project. That happens to be the Activation class, and found some things I would suggest refactoring. I'll present them in the form Symptom, Remedy, Example. For the discussion below, exact semantics are not quite important - the code quality and potential for refactoring is. *Current antipatterns and remedies* a) *Symptom:* Lots of empty entity setters/getters (i.e. bloat). If getters and setters do not provide any validation, annotation placeholding or type conversion, they do not really provide any value. (No proper information hiding if the setter/getter simply access the property). Instead, empty setters/getters confuse the reader and yield more maintenance time/cost/pain than if they simply did not exist in the code base. *Remedy:* Replace empty JavaBean property methods with public access to members. *Example (JavaDoc deleted for brevity): * Current Pattern =========== private String jdk; public String getJdk() { return this.jdk; } public void setJdk( String jdk ) { this.jdk = jdk; } Recommended Pattern ================= public String jdk; b) *Symptom:* Lacklustre Separation of Concern and Lack of Object oriented structure. Entities/model holds not only state, but also methods/properties dealing with location within the pom/XML configuration file. Moreover, these methods and properties are copied within all Maven's model entities (as opposed to being placed within an abstract superclass or commonly used, factored-out delegate). *Remedy:* Refactor the model to introduce abstract superclasses or delegate types holding functionality dealing with configuration file tracking and/or integration to the Lifecycle. Place only model state within the entity classes. *Example (JavaDoc deleted for brevity):* **Current Pattern (in most entities) ================================== private java.util.Map<Object, InputLocation> locations; public void setLocation( Object key, InputLocation location ) { if ( location != null ) { if ( this.locations == null ) { this.locations = new java.util.LinkedHashMap<Object, InputLocation>(); } this.locations.put( key, location ); } } public InputLocation getLocation( Object key ) { return ( locations != null ) ? locations.get( key ) : null; } Recommended Pattern =================== public class SomeEntity extends AbstractEntity { // Only domain properties here // No framework utility methods or properties. // They could/should be placed within superclasses. } public abstract class AbstractEntity implements LocationAware { // Deal with hashCode(), equals(), clone() and the like here. // Use reflection, to handle subclass needs. // Also, implement the LocationAware interface here, to // provide required integration with the configuration // framework. } public interface LocationAware { // Define the integration API to the configuration FW here. } c) *Symptom:* Metadata strewn around in model, in the form of properties and methods. This is a matter of style, but I advocate that we separate domain properties (i.e. the properties within domain classes) from metadata which could/should be represented as annotations. *Remedy:* Separate out metadata, such as configuration representation into annotations, to reduce the potential for data/metadata confusion. Examples of such metadata could be the precise format for configuration file storage. Also, we could get rid of unnecessary constrained criteria and properties, that could be replaced with common types and - optionally - metadata. Provide extensible structures, such as typed collections instead of specific properties (this appears to be something of a syndrome in the Activation class, but is likely similar in other places). *Example (JavaDoc deleted for brevity):* **Current Pattern =============== <All of the classes within the .io package [throughout Maven core]> <Lots of Plexus dependencies, such as> import org.codehaus.plexus.util.ReaderFactory; import org.codehaus.plexus.util.xml.Xpp3DomBuilder; import org.codehaus.plexus.util.xml.pull.MXParser; import org.codehaus.plexus.util.xml.pull.XmlPullParser; import org.codehaus.plexus.util.xml.pull.XmlPullParserException; Recommended Pattern =================== <remove .io packages + their content> Remove dependencies on Plexus as per above, et. al. Introduce metadata to represent configuration file structure, such as the standard JAXB annotations. *...@xmltype(namespace = "http://maven.apache.org/model/nazgul";)* *...@xmlaccessortype(XmlAccessType.FIELD)* @SuppressWarnings("all") public class Activation extends AbstractEntity implements Serializable { ... /** * List holding all ActivationCriteria of this Activation block. * ActivationCriterion instances will be checked in the order * they were read from the configuration (i.e. the same order * with which they occur within this List). */ *...@xmlelementwrapper(name = "criteria", nillable = false, required = true)* * @XmlElements({* * @XmlElement(name = "jdkCriterion", type = JdkActivationCriterion.class, required = false),* * @XmlElement(name = "osCriterion", type = OsActivationCriterion.class, required = false),* * @XmlElement(name = "propertyCriterion", type = PropertyActivationCriterion.class, required = false),* * @XmlElement(name = "fileCriterion", type = FileActivationCriterion.class, required = false)* * })* * public List<ActivationCriterion> criteria = new ArrayList<ActivationCriterion>();* ... d) *Symptom:* Identical documentation strewn on property, setter and getter. Along the same lines of reasoning as in (a) above, this is simply confusing. I would recommend against copy/paste-programming. *Remedy:* Fixed when removing unnecessary setters/getters, as per (a) above. *Example (JavaDoc deleted for brevity): *See (a) above. *Concluding example for the Activation class:* This is simply an example of how we could refactor the maven domain, to reduce its footprint, and - hopefully - facilitate understanding. Feel free to compare this incarnation of Activation class with the current one, and comment on the simplicity and clarity. I feel there is quite a lot of places where similar simplifications can be done within the maven model and core. ... and I'll be back with more code and thoughts later on. package org.apache.maven.model; import javax.xml.bind.annotation.XmlAccessType; import javax.xml.bind.annotation.XmlAccessorType; import javax.xml.bind.annotation.XmlType; import java.io.Serializable; /** * The conditions within the build runtime environment which will * trigger the automatic inclusion of the build profile. */ @XmlType(namespace = "http://maven.apache.org/model/nazgul";) @XmlAccessorType(XmlAccessType.FIELD) @SuppressWarnings("all") public class Activation extends AbstractEntity implements Serializable { /** * If set to <code>true</code>, this profile will be active unless another * profile in this pom is activated using the command line -P * option or by one of that profile's activators. */ @XmlAttribute public boolean activeByDefault = false; /** * List holding all ActivationCriteria of this Activation block. * ActivationCriterion instances will be checked in the order * they were read from the configuration (i.e. the same order * with which they occur within this List). */ @XmlElementWrapper(name = "criteria", nillable = false, required = true) @XmlElements({ @XmlElement(name = "jdkCriterion", type = JdkActivationCriterion.class, required = false), @XmlElement(name = "osCriterion", type = OsActivationCriterion.class, required = false), @XmlElement(name = "propertyCriterion", type = PropertyActivationCriterion.class, required = false), @XmlElement(name = "fileCriterion", type = FileActivationCriterion.class, required = false) }) public List<ActivationCriterion> criteria = new ArrayList<ActivationCriterion>(); } -- -- Lennart Jörelid
