Spring AOP : Avoide JDK proxies for struts application

Spring AOP uses both JDK proxies and cglib for creating dynamic proxies. The main purpose of proxy classes is to intercept method invocation on the target object and the execute the code from applicable advice. If the class implements an interface, then spring uses JDK proxies by default, and for all other classes it uses cglib to generate proxies.

Proxy classes generated by JDK proxy provides implementation for all methods in all the interfaces that are implemented by target class. So, generated class will not have methods that are defined by target class but are not part of any interface. All the implementations of the interface can use the same generated class which results in less number of dynamically generated proxy classes.
On the other hand, proxies generated by cglib extends the target class. So the generated proxy has all the methods defined by the class. This results in more number of proxy classes as every class, even different implementation of the same interface, will have their own dynamically generated proxy. However, the generated proxy classes are cached, so new classes are not created on every invocation.

In a web application which uses Struts, its not possible to use JDK proxies for Spring AOP. Consider the following scenario.

I have an action class -

package com.amey.labs.web.action;
//imports

public class HomeAction implements ModelDriven {

private String name;
private PersonService personService;
private HomePage homePage = new HomePage();

public HomeAction() {
}

public HomeAction(PersonService personService) {
this.personService = personService;
}

@Action(value = "/home", results = {@Result(name = SUCCESS, location="home.jsp")})
public String execute() {
homePage.setPerson(personService.getDetails(name));
return SUCCESS;
}

public String getName() {
return name;
}

public void setName(String name) {
this.name = name;
}

public HomePage getModel() {
return homePage;
}
}

I have configured an around advice for this action to log the entry and exit of the execute method.

<aop:config>
<aop:aspect ref="loggerAspect">
<aop:around method="doLogging" pointcut="execution(public * com.amey.labs.web.action..*.execute(..))">
</aop:around>
</aop:aspect>

HomeAction implements ModelDriven interface. So Spring will use JDK proxies to create a proxy class. The created proxy class will implement all the methods from ModelDriven interface. However, it won't have execute() method as it is not defined in ModelDriven interface. If I try to execute this action, I get NoSuchMethodException exception in AnnotationValidationInterceptor. This interceptor gets the method name which has @action annotation and then get the method object with the same name from action's Class object. However, the Class object is a object of proxy class generated by JDK proxy instead of the actual action class. So it does not have execute() method.

To fix this issue, I have to implement the com.opensymphony.xwork2.Action interface (or extend the ActionSupport). Once i do this, the created proxy class implements Action interface and has the execute() method.

Now, when I invoke the action from the UI, it can execute the execute() in the action. However, the generated proxy class doesn't have the methods for setting individual attributes in the action class (name in HomePageAction). So there is no way that the ParameterInterceptor finds out that this name-value passed in the request (either GET or POST) is actually a attribute on action and it just ignores that. CompoundRootAccessor class, which is invoked from ParameterInterceptor generates the warning message that the setter for "name" is not present in Action class or the model class and it is logged if the struts is running in dev mode, otherwise the parameters is considered as junk and it is ignored.

So the action class can not have any attribute which should be set from the UI. I need to move the name attribute from action to the Model class. If attribute is part of model, then it gets populated properly and I can use it in my action. However, attribute can not always be a logical part of the model object.

To force Spring to use cglib proxies, we just have to set proxy-target-class attribute to true.

<aop:config class="true">
// aop aspects
</aop:config>

In the struts actions where we have a property that we want to set from the UI, JDK proxies can not be used. So we have to use proxies generated by cglib. Even in other cases, cglib proxies performs better than JDK proxies. With JDK proxies, proxy class intercepts all the method invocations on the target object. All methods on target object are invoked by reflection, even if they are not advised. With cglib, proxy classes extends the target class and intercepts only advised methods. All other methods are invoked directly. So, using cglib proxies is considerably faster. Obviously, cglib has its own set of problems, but its a better alternative than JDK proxies.

Do you really want moist test ?

Moist tests are the test cases that do not follow DRY(Don't repeat yourself) principle. According to this philosophy, the setup and any other private method in the test class is a smell. It advocates that having all the test code in test method itself, improves readability and makes it easy to maintain. I recently joined a project team which follows this concept, and it was pretty surprising for me.

The test acts as a documentation for the object it tests. If i have to understand the job of a class, I will go to the test and read test methods. And thanks to the concept of moist test, I end up reading 5 lines of code duplicated in all the tests which creates object of system under test, creates mock objects, set expectations, inject dependency and do other setup stuff. So reader has to skip a chunk of code to get to the crux of the test. This is definitely not improving the readability of test.

The readability and maintainability of the test is more important than deciding whether to be moist or DRY in your tests. I agree that we should not be obsessive about strictly adhering to DRY in the test code, but in the same way, we should not be intentionally 'wet' and duplicate the code everywhere.

Lets look at the reasons people gives to write 'Moist' tests. I have taken most of the reasons from the Jay field's and Shane Harvie's post.

- When a test fails, I want to fix it quickly
How will duplicating the code in all test methods will allow you to fix the code quickly ? Suppose you add one more parameter to the constructor of your object, changing it in at one place in setup method will be much quicker than changing it in every test method.

Obviously, if you want to initialize your object differently, you have to instantiate it in your test method. But that's a special case, may be one of the ten test methods will need a different object and that method will instantiate it. Other methods in the test class will continue using the object that is instantiated in the setup method. If all the test methods needs different objects, then you don't need instance variable at all and all the test will have their own local variable. By doing so, you are breaking DRY principle, but it is necessary to make sure that your test is not forced to execute unnecessary code from setup method.

- I have to search around a large test class for extracted methods, or instance variables that may be modified throughout the class, this costs me time
Why would you have a large test class ? You will have a large test class only if the object your testing has multiple jobs and you have to test all that behavior. If you follow the Single responsibility principle and write classes that has only one job, you will have only one behavior to test (with alternate flows) and you will end up with small test classes.
So, if you have large class, its a smell that your object is doing too many things. You need to fix that first by segregate the responsibility into separate classes. Once you move the responsibilities to the different classes, you have to move the tests to separate test classes as well and you will end up with small test class.If doing so is impossible for some reason, you could extract test cases into separate files each testing a part of object's behavior.

Still even if your test class is large, I do not understand how searching for a method will take much time.(In most of the IDEs, you just have to do Ctrl+B(or other similar shortcut) to go to the method definition). The purpose of extracting methods is not only to group a block of code together, the more important purpose is to create abstraction. By giving descriptive method name to the extracted method and using that method in your test, you are creating an abstraction over a block of code and thereby increasing the readability of the test.

In few test, instance variable that is initialized in setup method will be modified by the test. However, this should not affect other tests. Ideally, you will create the instance of system-under-test (the object which you are testing) in the setup method and then use it in your test. The changes made by other objects to the instance variable will not affect your test as object is getting initialized in setup method for every test.

- Some developers don't know that setup method is getting executed/ they may not notice that setup method exists in test
This is probably the most lame excuse. Even a developer who learned JUnit today will know that the setup method gets executed before every test. And if your developer don't have this basic understanding, you should stop writing any code and educate them first. Their ignorance can't be the reason why you don't want to have setup methods in test class.

- Separating the test code in setup and test method decreases readability
Writing the code in setup method will reduce the readability only if you are not using the setup method properly. i.e. if we have any test specific code in setup method. However, if we have test environment setup code in setup and test and assertion code in test method, then it follows the natural flow of the test and should increase the readability. Though it depends on the reader as well. If the person is used to reading the code with small modular methods,reading the tests with setup methods should come naturally for him.

- Tests should be independent, so why should all of them use common setup (or any other common) method ?
When we say that tests should be independent, it doesn't mean that they should be lexically separate. It means that we should be able to run tests individually and the outcome of one test should not impact other tests. The setup method is executed before every test, so the objects are initialized freshly for every test. If setup method is too big and it contains some code that is not required for all the tests, then you have to extract the test specific code in your test method or write private methods.

- If you have to write large code to setup your test, you have to rethink about your object model instead of putting all the code in setup method
This is definitely true. Big setup code is a smell that your class has too many dependencies. In this case, we should definitely try to minimize it by breaking the class into different classes. But duplicating the chunk of code in all the tests is not the solution.


The ultimate core aim is the readability of test. Readability of the test is more important than applying DRY(this holds almost true for production code as well). We should not apply DRY principle stringently for test code. But it doesn't mean that we should go entirely other way round, write duplicate code and make our test moist purposefully.

IMHO, test method should have the code to invoke the method to test and do the state/behavior verification. It may also have code to set the expectations on the mock objects. The code for initializing the system-under-test (probably by injecting some mock objects) should go in the setup, which is the right place for it. Obviously, if all the test methods needs different setup, then the common setup method doesn't make sense and we have to duplicate some code in test methods. If you notice that changes are required in the common extracted methods and the changes are affecting other test, its time to get rid of extracted method and copy the code in all tests. So, you may end up with a test method which has all the code for setup, test and assertion. Its OK to violate DRY principle in your test code if it is required but this should be an exception. Naturally, we should try to keep our tests DRY and introduce 'moistness' only if required. We shouldn't intentionally make it 'wet' by duplicating the code everywhere.

Using field level access for hibernate entities

Hibernate allows you to configure the access mechanism for attributes in the entity object. It can access the attributes directly or through the accessors. The field or property access type is decided by hibernate based on the location of @Id or @EmbeddedId.

@Entity
public class Item {
@Id
public String id;
public long itemNumber;
}

@Entity
public class FoodItem {
private String id;
private long foodId;

@Id
public String getId(){
return id;
}
public long getFoodId(){
return foodId;
}
}

In the above code, Item class uses field access and FoodItem class uses property access.

Using property access mechanism allows classes to hide the internal data structure from the external world and it enables class to perform validations while setting the value or while accessing it. It is useful when we want to perform some calculation before returning the result or setting the value.

However, mostly we end up directly accessing the variable from the getter and setter methods. In that case, property access is overkill. It makes us write lot of getter - setters which are not used anywhere else in the code. It also exposes setters if when we don't want anyone to set the value of fields like 'id'. We can mark the accessors private or protected and avoid this problem, but we will still have the code.

IMHO, while creating the entity, we should start with field level access. Doing so will reduce the unnecessary code which is used only by hibernate. We should keep the fields private and provide accessor methods only when required. So our entity object will be modeled by the needs of the application and not by the hibernate. In future, if we need to validate or manipulate the values set on the object (which mostly won't happen), we can then switch to property access, but we shouldn't start with it.

Adding static @BeforeClass method in the test class

I was looking at a integration test in project that takes 10 seconds to execute. Its a integration test in a for a class that connects to the FTP server and performs some operations. The test uses a in-memory mock ftp server. The issue is that it starts, initializes the mock file system for every test and then stops the server after each test. The initialization code is written as a part of @Before method and server is stopped in @After method. The fact that the server was getting started and closed for every test was the reason why test was so slow. I want to start and initialize it only once for the all the tests my my class. The way to achieve this is to use @BeforeClass annotation. The method marked with @BeforeClass runs only once for all the test. The corresponding tear down method is @AfterClass. By using @BeforeClass instead of @Before and with some other changes, I was able to reduce the time required to run the test to 3 seconds.

However, using @BeforeClass instead of @Before may make tests dependent on each other and on the order of execution. As all the tests will use the same objects, changes made by one test will be affecting others. So @BeforeClass method should be used to initialize objects which are not modified by the tests. The methods should be used to setup the environment for the test and not for creating the objects that will be directly consumed by the tests.

@BeforeClass (and @AfterClass) method has to be static, which means all the variables that are accessed in this method has to be maked as static. This puts up restrictions on the ways in which @BeforeClass can be used, but this makes it explicit that the @BeforeClass method should be used to set only the environment for the test and objects under test itself.

Improved modularity with Superpackages in Java7

JSR 294 talks about introducing improved modularity support with Superpackages.
Right now modularity is supported at various levels in Java language. Modularity provides encapsulation and information hiding at various levels. Method encapsulates certain program logic and local variables. Class encapsulates instance variables and methods and a package provides a module to group related classes. All these modules helps us to understand and change the system locally, within the module space without worrying about its effect outside the module.

However, the modularity supported by packages is inefficient in some cases. The problem is that packages supports hierarchy, file system also stores classes in the hierarchy, but the accessing member of the package is not hierarchical. There is no special relationship between packages and their children or parents. We create packages like com.domain and com.domain.validators. Clearly, there is some kind of hierarchical relationship between the classes in two packages. You may want to access some classes or methods of com.domain from com.domain.validators package, but you don't want to expose them to the whole world by making them public. The scenario is very well explained by Neal Grafter at http://www.infoq.com/interviews/gafter-closures-language-features-optional-typing. Currently, there is no way to achieve this directly.

We can use default member access so that it becomes public within the same package, but stays inaccessible to the outside world. Classes in the subpackages also can not access it. So we end up putting classes in the same package as we want to access members with the default access. This leads to packages with large number of classes, the classes which should ideally go in the sub packages.

The other option is to make the members public. Doing this may expose unwanted API. We will end up relying on the official documentation of the API and prays that no one comes to know about the class and invoke its 'public' method. The issue is public is too public.

We need a module entity that is bigger than a package but narrower than public. The solution is to introduce one more level of modularity called Superpackages. Superpackages contains classes from one or more packages or superpackages. Its a new construct which is put into super-package.java file and compiled by the Java compiler. The types can be exposed to the outside world by exporting them. Only the exported types are visible outside the superpackage. Public types that are not exported can not be accessed outside of superpackage.
e.g. A superpackage which encapsulates types in com.domain and com.domain.validators is defined as -

superpackage domain {
// member packages
member package com.domain;
member package com.domain.validators;

// exported types
export com.domain.*;
}

Here, only classes from com.domain package is visible outside superpackage, classes from com.domain.validators are not.Note that Java classes doesn't specify the superpackage they belongs to. It is specified in the super-package.java file only.
Superpackages can encapsulate other superpackages as well. Only the public types in exported superpackage will be accessible from outside.

Superpackages is coming up with Java 7 and going to be an effective mechanism for information hiding in Java.

Java 7:Multiple exception catch block

Java 7 proposes a new syntax for catching multiple exceptions in a single catch block (http://www.javac.info/Multicatch.html). It looks like a neat feature to have and the new syntax is definately better than having multiple catch blocks with the same exception handling code.
So instead of -

try{
// Some code that throws ServiceException and WebsiteException
}
catch(ServiceException exception) {
Logger.Log(exception);
throw ApplicationException(exception);
}
catch(WebsiteException exception){
Logger.Log(exception);
throw ApplicationException(exception);
}

we will be able to write -

try{
// Some code that throws ServiceException and WebsiteException
}
catch(WebsiteException | ServiceException exception) {
Logger.Log(exception);
throw ApplicationException(exception);
}

So, we will be able to get rid of duplicate exception handling code and will make our code little simpler. Its a pretty obvious change and ideally it should have been identified and fixed before.

This new feature is useful, but how much ? I think this is trying to solve the problem that should not exist. In most of the cases, you should not be throwing more than one checked exception.It will happen if we are exposing the implementation of method and rethrowing the exceptions from called methods.
Even if we have more than one exceptions, we may want to handle them in different ways. So we can not combine multiple catch blocks.
Apart from that, whole lot people in java devs community, including me, believes that checked exceptions should not be used at all. Checked exceptions forces developer to write the exception handling code even when it is not required. Exception handling code gets mixed up with the business logic. This leads to cluttered and unreadable code. Worst part is that checked exceptions create dependency between a method that throws exception and all other methods that directly or indirectly calls that. It makes caller of the method depends on the implementation details of the method. And after coding in C# (which doesn't have checked exceptions) for 2 years, i doubt whether checked exceptions are required in language at all.
Looks like this is nice to have but not so useful (lame?) features in Java 7.

Following SRP on grounds

Single responsibility principle is probably the simplest OO coding principle. It says "A class should have only one responsibility and thereby, only one reason to change." Isn't it very straightforward ?
But when I look at the code of my application, I see that very few classes in my app follows this principles. Why is that ? The principle is very simple and so it should be simple to implement as well. Its relatively easy to make sure that our class doesn't have any method that doesn't belong to the job of class. I think the difficult part is to know when we are adding additional responsibilities to the class. We do incremental development, start with only what is required and then add up behavior in methods and methods to class as required by the failing test. During this process, we somehow manage to sneak in the logic that is not the part of class's responsibility.
For example, we have mapper classes in our code which maps domain object to data contracts. The job of mapper classes is just to copy the fields from domain objects to contact. The mapper should only do this job of mapping. Consider the following example. In this, Booking object is loaded from the database. This class will map Booking to BookingData object.

public class BookingMapper{

public BookingData Map(Booking booking)
{
BookingData bookingData = new BookingData();
bookingData. Id = booking.Id;
// Other fields
bookingData.BookingFee = CalculateBookingFee (booking.StationCode, booking.TotalCost);

return bookingData;
}

private decimal CalculateBookingFee (StationCode stationCode, decimal totalCost){
// some complex logic to calculate booking fee based on station code and total cost,
// may be, the method will read some values from database.
}
}

The class BookingMapper has only one Map() method. From outside the class, it looks like the class has only one responsibility. However, apart from mapping the data, the class is also calculating the booking fee. It has 2 jobs - to map the data and to calculate the value of booking fee.

The mapper class should do only mapping and we have to move the responsibility of calculating booking fee to other class. If the fee calculation depends only on data present in booking object (like station code and total cost), then the obvious place for CalculateBookingFee method is Booking class itself. We can have a read-only property on Booking class called BookingFee which will calculate the fee.If the calculation of fee involves reading the values from database and/or from some other service, then we don't want our domain object to access services or database. In this case, we can calculate the booking fee (and any other required values) before invoking the mapper and pass it to Map() method. Either way, the fee calculation logic will move out of BookingMapper.