I recently came across Martin Fowler‘s post about FunctionLength.
In that post he stated something about intention versus implementation. Although it might seem a very trivial thing, the impact on code readability is huge!

For example, if I want to check if a book is written by more than one author, I could to something like this:

1

2

3

4

Book book=...

if(book.getAuthors().size()>1){

//do something with that book

}

That works perfectly well, but:

It doesn’t tell me the intention of the code: what did the programmer really wanted to check?

It reveals implementation details that I don’t want to know at this abstraction level. I don’t care if the solution is implementing a list size check.

How to refactor this? State your intentions explicitly, by moving implementation detail to the correct abstraction level:

1

2

3

4

5

6

7

8

9

10

...

Book book=...

if(hasMoreThanOneAuthor(book)){

//do something with that book

}

...

privatebooleanhasMoreThanOneAuthor(Book book){

returnbook.getAuthors().size()>1;

}

Or even better (but not really relevant for this blog post):

1

2

3

4

5

...

Book book=...

if(book.hasMoreThanOneAuthor()){

//do something with that book

}

This way, you can stop reading at the abstraction level that contains the expression hasMoreThanOneAuthor, without your mind being cluttered by implementation details. Maybe I will find a better way of checking a book has more than one author without bothering you with it (although in this particular example that seems highly unlikely).

When reviewing code I have now developed the habit to refactor almost every expression that isn’t trivial into its own method. I found that this really improves code readability.

Quite often I see unit tests that seem very interested in a class’s inner workings. This not only misses the point of a unit test, but makes the class harder to refactor, since the corresponding unit test will have to change more often as well.

Suppose you have a BookService class, responsible for retrieving and storing books:

1

2

3

4

5

6

7

8

9

10

11

12

13

14

15

16

publicclassBookService

privateBookRepository bookRepository;

publicBookService(BookRepository bookRepository){

this.bookRepository=bookRepository;

}

publicList<Book>getAllBooks(){

returnbookRepository.getAllBooks();

}

publicvoidcreateBook(Book book){

bookRepository.createBook(book);

}

}

Pretty straightforward, right? Now, suppose we write a unit test to test the getAllBooks method, using Mockito to stub things out. Often I encounter something like this:

Notice the verify call at the end of the method. This makes assumptions about the class’s inner workings. Don’t do this. A unit test should check if a class implements its contract. Instead, verify behaviour only:

Of course you need a collaborator to get this class going, hence the BookRepository mock object and the recorded behaviour using when.

Verfiy behaviour only also applies to methods that mutate the system (a “command”), like the createBook method in the example class. Of course, such methods have a void result type (if you build your system CQRS-wise, of course). Only difference is, the output comes from the bottom this time (pun intended):

1

2

3

4

5

6

7

8

9

@Test

publicvoidinsertBook(){

BookRepository bookRepository=mock(BookRepository.class);

BookService bookService=newBookService(bookRepository);

Book the_dark_tower=newBook("The Dark Tower");

bookService.createBook(the_dark_tower);

verify(bookRepository,times(1)).createBook(the_dark_tower);

}

In some cases, a command also has a result type, for example, the result of the mutation. For example, the createBook method could have returned its Book parameter enriched with an id, for example, as a result of storing the book in the repository. In that case, you could verify output at both sides, although assertion the return value only would still be preferred.

Most of the time, however, encountering such a method is probably a side effect anti-pattern and should be refactored into two separate methods.

I’ve seen quite a lot of Spring projects over the years that use multiple PropertyPlaceholderConfigurer instances in the same bean factory.
Consider a project where maven module A depends on maven module B in the same maven project. The idea is that module A is self-sustaining and loads its own property files, and module B does as well. This philosophy is good, but can lead to a horrible mess when used in combination with default values.

The problem setup

You have a Spring project with two PropertyPlaceholderConfigurer instances, each loading their own property file, and one Spring Bean that has a @Value injection with a default value.

Property placeholder 1 tries to resolve the value of myKey. It doesn’t have a value for that, but “fortunately” the bean specifies a default value, so it uses that.

Property placeholder 2 sees that the bean already has received a value (set by Property placeholder 1), so it ignores it.

Using ignoreUnresovablePlaceholders doesn’t help because the placeholder configurers will still use the default value if encountered on a bean.

To make matters even worse, Spring shows undeterministic behaviour here, because it uses unordered hashmaps internally that result in property placeholder 2 sometimes being fired before property placeholder 1. The Spring Bean then actually receives value “value2” which is what we would expect. I’ve seen two log files where starting the Spring container a second time reverses the order in which the property placeholder configurers are loaded.

I guess that propertyplaceholder configurers not knowing about each other is by design, but the above can be a big gotcha.

I’ve posted a sample project on GitHub that reproduces the problem. If you run the unit test multiple times it will inevitably fail at some point.

The solution alternatives

Only use one PropertyPlaceholderConfigurer for your deployable unit (a WAR, for example), at the top level. I would recommend this solution first.

Do not use defaults in your @Value statements. Instead, explicitly define all property values once and use this in combination with ignoreUnresovablePlaceholders on all property placeholder configurers. I consider this a brittle solution at best.

The Spring issue tracker describes the problem here. As of Spring 4.3.5 it is still unresolved.

Over the past years, the Spring Framework has given us many tools to simplify ease of application development.

One of the nicer features is Spring’s PropertyPlaceholderConfigurer, a BeanPostProcessor implementation that allows you to inject values from property files into your managed beans, making application configuration a breeze.

JEE doesn’t have a PropertyPlaceHolderConfigurer right out of the box so we have to come up with a different solution.

What JEE offers are producer methods. A producer method is a method that constructs an object of an arbitary type which can then be used by Context Dependency Injection (CDI) as a candidate for injection.

Producer methods use the @Produces annotation for indicating the kind of object it will produce.
The kind is indicated by the produced type and, optionally, a qualifier annotation.

Say that you want to indicate that some properties should be injected in your client class. We will put an custom annotation @ConfigurationValue on each field to indicate the field value should come from a property.

1

2

3

4

5

6

7

@Inject

@ConfigurationValue(value="jdbc.dbName")

privateStringmyDatabaseName;

@Inject

@ConfigurationValue

privateStringmyProp;

where you want to explicitly specify the property key on the first field to be populated, and just use the field name as a property key on the second field.

The qualifier @ConfigurationValue used by the object that requires configuration would look as follows:

1

2

3

4

5

6

7

8

9

10

11

12

13

14

@Qualifier

@Retention(RUNTIME)

@Target({TYPE,METHOD,FIELD,PARAMETER})

public@interfaceConfigurationValue{

/**

* Property key that will be searched when injecting the value.

*/

Stringvalue()default"";

/**

* Defines if value for the given key must be defined.

*/

booleanrequired()defaulttrue;

}

The meta-annotation @Qualifier here indicates this annotation can be used for JEE’s CDI qualifying mechanism.

Now, we have to write a producer method that responds to fields using the @ConfigurationValue annotation.

An example of a producer method that retrieves its property values from a properties file could be the one below.

1

2

3

4

5

6

7

8

9

10

11

12

13

14

15

16

17

18

19

20

21

22

23

24

25

26

27

28

29

30

31

32

33

34

35

36

publicclassConfigurationValueProducer{

//We could use Spring's PropertyResolver, or the JEE one from the faces package. We use the last one here.

//You could replace your property loading with any way you see fit.

@Inject

PropertyResolver resolver;

@Produces

@ConfigurationValue

publicStringgetStringConfigValue(InjectionPoint ip){

// Trying with explicit key defined on the field

Stringkey=ip.getAnnotated()

.getAnnotation(ConfigurationValue.class).value();

booleanisKeyDefined=!key.trim().isEmpty();

booleanvalueRequired=ip.getAnnotated()

.getAnnotation(ConfigurationValue.class).required();

if(isKeyDefined){

returnresolver.getValue(key);

}

// Falling back to fully-qualified field name resolving.

Stringfqn=ip.getMember().getDeclaringClass().getName()+"."

+ip.getMember().getName();

key=fqn;

Stringvalue=resolver.getValue(fqn);

// No luck... so perhaps just the field name?

if(value==null){

key=ip.getMember().getName();

value=resolver.getValue(key);

}

// No can do - no value found but you've said it's required.

if(value==null&&valueRequired){

thrownewIllegalStateException("No value defined for field: "+fqn

+" but field was marked as required.");

}

returnvalue;

}

Of course, this only works for strings now, but it would be easy to extend in such a way that you would support numerical types as well, through multiple producer methods.