Main | Storing objects graphs - native vs. relational storage »

Code Impressions Episode 1 - Is Spring coming yet?

In civil architecture practitioners study work of the masters: cathedrals, bridges, even pyramids. In software engineering we rarely have things that last so long, as most of the software is built in the rush. I still enjoy studying sources. You would expect well-established frameworks to be of a top quality, and often times this is the case. But sometimes even the best pieces will have their smells.

I've recently came across such a smell where I would least expect it. I mean, smells can be found everywhere and will happen to the best, but it is the matter of a kind in this case that made be blog about it. Just to make things clear: this is not a finger pointing in any way and I am sure that folks behind the piece of code I am about to comment on are otherwise good engineers.

NOTE: Code under investigation taken from git version 8472a2b.

Let's have a look at spring-context:org.springframework.cache.interceptor.CacheAspectSupport lines 184-187:

Class<?> targetClass = AopProxyUtils.ultimateTargetClass(target);javascript:noop()
if (targetClass == null && target != null) {
    targetClass = target.getClass();

This sequence of statements is semantically terrible: if AopProxyUtils.ultimteTargetClass(target) returns an ULTIMATE target class (whatever it might be), what is the "if" block doing there? My guess, somebody simply got lazy. The only possible situation when this call could return null is when the target object is a CGLib proxy and was created from the interface, as then candidate.getClass().getSuperclass() returns null (spring-aop:org.springframework.aop.framework.AopProxyUtils:67):

result = (AopUtils.isCglibProxy(candidate) ? candidate.getClass().getSuperclass() : candidate.getClass());

Should the authors of the CacheAspectSupport consider modifying AopProxyUtils.ultimteTargetClass instead? Possibly. We cannot however forget the regression cost associated with the change and that some other frameworks which rely on spring-aop might have a dependency on the respective utility method. However, as of a considered version, the mentioned method is only used once in production code, in the discussed CacheAspectSupport.execute. 5 other usages in spring-context and spring-aop are in tests. That does qualify as a smell, doesn't it?

Now the true shocking bit: AopProxyUtils DOES NOT HAVE  a single line of test code! Mr Johnson R. and Mr. Hoeller J. shame on you with that one :)

PrintView Printer Friendly Version

EmailEmail Article to Friend

References (3)

References allow you to track sources for this article, as well as articles that were written in response to this article.
  • Response
    Hello, here to post points. Here's a good article written, rich in content. If you want more information, look at the situation here:Hollister Online Shop
  • Response
    Response: pinterest
    All Things Integrated - Journal - Code Impressions Episode 1 - Is Spring coming yet?
  • Response

Reader Comments

There are no comments for this journal entry. To create a new comment, use the form below.

PostPost a New Comment

Enter your information below to add a new comment.

My response is on my own website »
Author Email (optional):
Author URL (optional):
Some HTML allowed: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <code> <em> <i> <strike> <strong>