Mittwoch, 28. Dezember 2011

Principle of Least Astonishment

A fundamental rule of software development is building small assemblies with high cohesion. Software assemblies do one job and they have a clear name describing this job. This means, if you are not able to give assemblies a clear name, you have to split the assemblies to even smaller ones. Breaking this rule will violate the Principle of Least Astonishment and annoy the users of the software assemblies. In bad cases, wrong usage and bugs follow simple code smells which could be solved by clear naming or the separation of the assemblies.

The following method is an example of a code smell violating the Principle of Least Astonishment. This software shows that it is possible to break a source code base with a few lines of smelly code.

Principle of Least Astonishment violation:

public void addInstance(String className) {

    try {

       Class clazz = Class.forName(className);
       objectSet.add(clazz.newInstance());
    }
    catch (RuntimeException ex) {

       throw ex;
    }
    catch (Exception ex) {

      throw new RuntimeException(ex);
    }
}

What is wrong with this code?

  1. There are no asserts in the code to check pre and post conditions. A stable implementation would check the method argument and type of the created instance with “instanceof” (be careful you would on the other hands side lose the flexibility of the implementation and violate the Liskov Substitution Principle).

  2. The compile time type checking is lost. Therefore, errors may come up late at runtime, which hit the code hard and open the door for a security violation.

  3. The more stable implementation of the code would lead to code bloat.

  4. The method name is bad. There is more than adding an instance. The better name is “createAndAddInstance”. Think about the name. The possible better solution is to split the method instead of changing the name.

  5. The implementation assumes that instances which the access modifiers "public" or "package" are created and added to the set.
An alternative implementation covers the strong points and shows the code bloat to remember “new” with all its benefits could be more appropriate solving the main problems of the implementation.

Alternative implementation including an Unit-Test:

import java.util.HashSet;
import java.util.Set;
import org.ccd.reflection.test.DomainObjectException.ErrorCode;
import org.junit.Assert;
import org.junit.Test;

public class InstantiationByReflectionTest {
   
    private Set<DomainObject> objectSet = new HashSet<DomainObject>();
       
    @Test
    public void testInstantiation() {
   
        createAndAddDomainObject("org.ccd.reflection.test.DomainObject");
        createAndAddDomainObject("org.ccd.reflection.test.DomainObject");
       
        Assert.assertTrue(objectSet.size() == 2 ? true : false);   
    }
       
    @Test
    public void testNullPointerArgument() {
   
        try {
       
            createAndAddDomainObject(null);
        }
        catch(DomainObjectException ex) {
           
            Assert.assertTrue(ex.errorCode() == ErrorCode.NullPointerArgument);
        }
    }
   
    @Test
    public void testEmptyArgument() {
   
        try {
       
            createAndAddDomainObject("   ");
        }
        catch(DomainObjectException ex) {
           
            Assert.assertTrue(ex.errorCode() == ErrorCode.EmptyArgument);
        }
    }
   
    @Test
    public void testIllegalType() {
   
        try {
       
            createAndAddDomainObject("java.lang.String");
        }
        catch(DomainObjectException ex) {
           
            Assert.assertTrue(ex.errorCode() == ErrorCode.IllegalType);
        }
    }
   
    @Test
    public void testClassNotFound() {
   
        try {
       
            createAndAddDomainObject("org.ccd.reflection.test.MissingDomainObject");
        }
        catch(DomainObjectException ex) {
           
            Assert.assertTrue(ex.errorCode() == ErrorCode.ClassNotFound);
        }
    }
   
    @Test
    public void testInstantiationFailure() {
   
        try {
       
            createAndAddDomainObject("java.io.Serializable");
        }
        catch(DomainObjectException ex) {
           
            Assert.assertTrue(ex.errorCode() == ErrorCode.InstantiationFailed);
        }
    }
   
    @SuppressWarnings("unused")
    private class MyDomainObject {}

    @Test
    public void testInstantiationPrivate() {
   
        try {
       
            createAndAddDomainObject("org.ccd.reflection.test.MyDomainObject");
       
        }
        catch(DomainObjectException ex) {
           
            Assert.assertTrue(ex.errorCode() == ErrorCode.ClassNotFound);
        }   
    }

    private void createAndAddDomainObject(final String className) {
   
        assertNotNull(className);
        assertNotEmpty(className);
       
        Class<?> cl = null;
        try {
       
            cl = Class.forName(className);
        }
        catch (ClassNotFoundException ex) {
       
            throw new DomainObjectException(ErrorCode.ClassNotFound);
        }
              
        Object domainObject = null;
        try {
           
            domainObject = cl.newInstance();
        }
        catch (InstantiationException ex) {
           
            throw new DomainObjectException(ErrorCode.InstantiationFailed);
        }
        catch (IllegalAccessException ex) {
       
            throw new DomainObjectException(ErrorCode.IllegalAccess);
        }
      
        assertDomainObjectType(domainObject);
       
        objectSet.add((DomainObject) domainObject);
    }
   
    private void assertNotNull(final String className) {

        if(null == className) {
           
            throw new DomainObjectException(ErrorCode.NullPointerArgument);
        }
    }
   
    private void assertNotEmpty(final String className) {

        if(className.trim().isEmpty()) {
           
            throw new DomainObjectException(ErrorCode.EmptyArgument);
        }
    }
   
    private void assertDomainObjectType(final Object domainObject) {
       
        if(!(domainObject instanceof DomainObject)) {
           
            throw new DomainObjectException(ErrorCode.IllegalType);
        }
    }
}

class DomainObject {}

class DomainObjectException extends RuntimeException {
   
    public enum ErrorCode {
       
        ClassNotFound("Class not found!"),
        InstantiationFailed("Instantiation failed!"),
        IllegalAccess("Access not denied!"),
        NullPointerArgument("Parameter cannot be null!"),
        EmptyArgument("Parameter cannot be empty!"),
        IllegalType("Illegal type detected");
       
        private final String errorMessage;
       
        private ErrorCode(final String errorMessage) {
           
            this.errorMessage = errorMessage;
        }

        String errorMessage() {
       
            return errorMessage;
        }
    }
   
    private final ErrorCode errorCode;
   
    public DomainObjectException(final ErrorCode errorCode) {           
       
        this.errorCode = errorCode;
    }

    ErrorCode errorCode() {
   
        return errorCode;
    }
}


Der Rechtshinweis des Java Blog für Clean Code Developer ist bei der Verwendung und Weiterentwicklung des Quellcodes des Blogeintrages zu beachten.