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:
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 :)