Code Religion

Code Religion - We don't just like good code. We worship it.

[codereligion-blog]

Time is relative

Introduction

Albert Einstein postulated that time is relative and he has been proven right, but what is time actually? Time is a unit which we use to split our daily live into pieces. Time has multiple different units which translate more or less to astronomic events.

We made it quite easy for ourselves. We named the rotation of the earth around itself “day” which is basically defined by sunrise and sunset and we named the rotation of the earth around the sun “year”. These units are the basic elements of what we call time, but that was not enough.

We needed a finer granularity, so we made up months, weeks, hours, minutes,  seconds and so forth. Most of todays month and week concept was defined by the romans.

For everyone who is really interested in how todays date and time units got established and restructured over time I really recommend to read this “how stuff works” article.

Time is complicated

Leaps

The main problem is that the units we chose do not match the actual astronomic events exactly, which brings in the problem of leap years and even leap seconds. Leaps allow us to re-establish accuracy by adding a day every four years (details) and from time to time a second (details).

Time zones

Despite those basic problems in accuracy we made it a little more complicated by introducing time zones, because a day should still be something that starts with sunrise and ends with sunset. So depending on how far away you are from Greenwich (the meridian) you might add or remove a few hours to get your local time.

Daylight saving

That is something the modern society invented for the sake of having more daylight. That allows us to be more productive, have a “longer day” and it even lets us safe electricity.

Calendar

Historically not all parts of the world evolved in the same way, so we actually now have despite the ISO standardized calendar (ISO8601) a lot of other systems to calculate and define time. For example: Buddhist, Ethiopic and Islamic.

Format and localization

Regional differences do not only come in different calendars, but also in different formats of displaying time, even in the same calendar. For example Germans usually use something like 31.12.2012 while the Americans prefer something like 12/31/2012.

What about java.util.Date?

This class was the first attempt in Java to create a class which can store time information. As it is with all the first drafts, they usually are not perfect and need to be redesigned. Actually the Date class was so fundamentally broken that it only lasted in the Java version 1.0. Most of the public API of this class has been deprecated with version 1.1, this means four of six constructors and 18 of 27 methods. Anyhow this class is still widely used but mostly in combination with the next “draft” which they called Calendar, but let's take a step back and have a look at the Date class.

Weird offsets

The first thing you will have a problem with, when working directly with Date is that it has some really weird offsets. Let's have a look at the constructor; if you would like to initiate a Date object for the last day of the year 2012 (31.12.2012) you would need to call the constructor like this:

new Date(112, 11, 31);

Isn’t that great? So the year is something completely absurd and the month is minus one, because it starts at 0 while the days start at 1. Actually this constructor was not so bad until the year 2000, because if you would take the last day of the year 1985 it would look like:

new Date(85, 11, 31);

So now you can see where this weird year - 1900 formula comes from. I guess they did not think this API would last that long. By the way Java 1.0 was released on the 1th October 1992.

Accuracy

Date stores the time internally as a long which represents the milliseconds since the beginning of the UNIX epoch, which was on 01.01.1970 at 00:00:00:000. It also exposes this accuracy in milliseconds as public API, but there is no constructor with which you can specify milliseconds, despite the one which takes a UNIX epoch timestamp. This means that the internal logic of Date will automatically convert every specified Date to 0 milliseconds.

Internationalization

As it is stated in the class documentation Date is not capable of internationalization. The methods Date.toGMTString() and Date.toLocaleString() are both deprecated and refer to DateFormat.format(Date). Sadly the class DateFormat is abstract and declares in its class documentation that all subclasses are inherently not thread safe. Currently there is only one subclass in the JDK which is called SimpleDateFormat.

Time zones

Date actually supports time zones, but in a very hard to work with manner. It has a method which will return the time zone offset it has from UTC. Wait, how does it know that? The answer is simple, it just takes the default timezone of your system, which you can set  by calling TimeZone.setDefault(TimeZone). This seems to be ok in environments where you have only one working thread or only one time zone to take care of, but as soon as you want to go multithreaded with different time zones you get into trouble.

Actually it is recommended to use SimpleDateFormat to display the time localized to a specific time zone, but watch out for thread-safety.

Calendars

Date does not support different kinds of calendar systems, actually it is hardcoded to the gregorian calendar.

General class design

Despite the already mentioned problems there are some more which concern more or less basic API design principles. What would you expect from a class called Date? Would you expect it to contain time? I would not, but sadly it does. So the name is in best case confusing and in the worst case just plain wrong.

Additionally Date objects are mutable which is again an issue in multithreaded environments. Even in single threaded environment you should make defensive copies on every method call which either takes or returns a Date object which is associated with the internal state of an object.

What about java.util.Calendar?

So is Calendar an improvement to Date? Definitely, but it still has some flaws.

Improvements

it does not have the weird year offset

it supports millisecond precision in constructors

it has time zone support

it has, as the name already implies, calendar support (well kind of ...)

Remaining problems

Sadly the month offset is still there. So January is still equal to 0, but at least there is now a variety of constants to use like Calendar.JANUARY.

Although the Calendar class is supposed to support different calendars it only comes with one which is Gregorian. At least that is what you can see from the Java documentation, but actually there are two more which are hard-coded in the Calendar class. These two are Buddhist and Japanese Imperial. So how can you retrieve an instance of such a calendar when there is no public class to instantiate them?

The answer is: through the static method Calendar.getInstance(Locale). So if you would like to get a Buddhist calendar you must do the following: Calendar.getInstance(new Locale("th", "TH")). The sad thing is that the developers hard-coded it actually to the locale for Thailand, which makes me think they assumed that only Thailand has a Buddhist calendar which is just plain wrong. So this so called calendar API is actually not really providing calendars, nor allows it to create custom calendars easily.

Besides, Calendar is still mutable and does not assert on invalid dates. Time zones are now supported, but you are always forced to have a time zone.

Last but not least what would you actually expect from a class called Calendar? Would you expect it to contain date information? Maybe. Would you expect it to contain time information? Not so much. Would you expect it to contain time zone information? Most certainly not.

This is a class which is not well designed and you can already tell that by reading the name and the class documentation.

What about java.util.TimeZone?

The abstract class TimeZone tries to accomplish to cover the aspects of handling time zones and daylight saving. The interface feels a little unhandy especially when working with Date objects. For example the method TimeZone.getRawOffset() returns signed milliseconds while the method Date.getTimeZoneOffset() returns unsigned minutes.

You can query available time zones by calling TimeZone.getAvailableIDs(). If you try to retrieve a TimeZone that does not match one of those IDs the TimeZone class will automatically fall back to GMT. So for example this will always be true:

TimeZone gmt = TimeZone.getTimeZone("GMT"); TimeZone foo = TimeZone.getTimeZone("foo"); gmt.equals(foo); // will return true

If you find yourself in the position of not finding your desired time zone in the “available IDs” you can specify a custom time zone through SimpleTimeZone class, which has quite a tricky interface. This means if you do not read the entire class documentation, you will not be able to initialize a correct SimpleTimeZone object.

Despite these strange things the TimeZone class also has a quite unintuitive implementation, if not plain false, when it comes to daylight saving. The following tables show the time zone database definition of daylight saving transitions for CST/CDT and compares it to the implementation in the JDK.

Time zone database definition for daylight saving period start CST/CDT

When local standard time is about to reach Sunday, March 11, 2012 at 2:00:00 AM clocks are turned forward 1 hour to Sunday, March 11, 2012 at 3:00:00 AM to local daylight time.

Time DST UTC offset Time Zone 0:00:00 AM No UTC-6h CST 1:59:59 AM No UTC-6h CST 2:00:00 AM→ 3:00:00 AM +1h UTC-5h CDT 3:00:01 AM +1h UTC-5h CDT

So that means when the time zone transitions from CST to CDT one hour is skipped. The day on which the daylight saving period starts has no 2 AM, because the clock automatically jumps from 1:59:59 AM to 3:00:00 AM. In return this means that it should not be possible to initialize a Date or Calendar instance for this concrete moment in time for this specific time zone.

Let's have a look how it is implemented in the JDK.

Behavior implemented by JDK for daylight saving period start CST/CDT

Time DST UTC offset Time Zone 0:00:00 AM No UTC-6h CST 1:59:59 AM No UTC-6h CST 2:00:00 AM → 3:00:00 AM +1h UTC-5h CDT 3:00:01 AM +1h UTC-5h CDT

The table tries to show that if you initialize a Date object with 2 AM for the day on which the daylight saving period starts it will be converted automatically to 3 AM without further notice. I would rather prefer to get an exception when trying to initialize and invalid moment in time, but at least that behavior is consistent with the rest of the public interface of Date and Calendar. One could say that the JDK implementations try to patronize the user by picking some default or next best value, if the given one is not valid.

Let's have a look now at how the transition from daylight saving to non daylight saving is specified and implemented.

Time zone database definition for daylight saving period end CST/CDT

When local standard time is about to reach Sunday, November 4, 2012 at 2:00:00 AM clocks are turned backward 1 hour to Sunday, November 4, 2012 at 1:00:00 AM to local daylight time.

Time DST UTC offset Time Zone 0:00:00 AM +1h UTC-5h CDT 1:00:00 AM +1h UTC-5h CDT 1:59:59 AM +1h UTC-5h CDT 2:00:00 AM→ 1:00:00 AM No UTC-6h CST 1:00:01 AM No UTC-6h CST 2:00:00 AM No UTC-6h CST

The specification states that the last moment in daylight saving is 1:59:59 AM (milliseconds omitted) and that the clock switches from that time directly back to 1 AM. This means that there are actually two 1 AM instants in time for that specific date, one in CDT and one in CST. So how does the JDK handle this case?

Behavior implemented by JDK for daylight saving period end CST/CDT

Time DST UTC offset Time Zone 0:00:00 AM +1h UTC-5h CDT 0:59:59 AM +1h UTC-5h CDT 1:00:00 AM No UTC-6h CST 2:00:00 AM No UTC-6h CST

So instead of the specified switch from 2 AM to 1 AM and having the 1 AM twice the implementation switches from 0:59:59 (milliseconds omitted) CDT directly to 1:00:00 CST which effectively skips one hour. This means it is not possible to define any instant in CDT on that date for 1 AM, if you do it anyways it will automatically be converted to 1 AM in CST. This can be quite a surprise, because the actual switch will always be one hour earlier as specified by the time zone database.

Joda Time

So can we do better than Date and Calendar? Yes, we can! There is that nice little API called Joda Time, which by the way is not pronounced Yoda but Joda.

Basic concepts

It separates the concerns of date, time, time zones, calendars and mutability/immutability. In the Joda date-time-continuum the basic concept of storing time is called an instant. An instant represents a concrete and valid moment in time with millisecond precision, so if you try to initialize and invalid instant you get an exception. Internally such an instant is stored as the number of milliseconds since the UNIX epoch, which is similar to the JDK approach.

In fact, the Joda Time is fully compatible and interoperable with the JDK classes like Calendar, Date and TimeZone. This means you can initialize Joda Time classes from JDK classes and vice versa.

In order to represent things which are not a concrete instant the Joda Time API uses so called partials. Partials are things that can become an instant by adding the missing parts which are required by an instant. So for example the partial LocalTime, which is a time object without date and time zone information, can be extended with those to become an instant.

So basically you convert every instant to a partial through convenience methods like toLocalTime() and at the same time you can convert every partial to an instant by adding the missing parts.

Convenience

Joda Time is very convenient to use, not only because of those nice conversion APIs, but also because it finally provides a month system which is based on 1 and not on 0. This combined with the parameter validation makes a much better and intuitive API as everything else which comes from the JDK.

Additionally it provides a lot of utility methods and classes which allow you to perform very easy transformation and different representation of data.

It also comes with thread safety through the provided immutable classes and through a thread safe formatter.

Joda Time is widely accepted as the new standard for handling time. This means though not all frameworks and libraries support Joda Time natively, extensions are available or can be written easily.

Calendars

In the Joda API calendars are implemented as “chronologies”. The API comes with the following calendars: Buddhist, Coptic, Ethiopic, Gregorian/Julian, Gregorian, Islamic, ISO and Julian. Additionally it provides some wrapper chronologies to add additional features like lenient, strict, zoned or limited chronologies.

Time zones

Time zone implementation is more convenient than the JDK implementation. Actually Joda Time comes with its own time zone class called DateTimeZone.

Behavior implemented by Joda API for daylight saving period start CST/CDT

Time DST UTC offset Time Zone 0:00:00 AM No UTC-6h CST 1:59:59 AM No UTC-6h CST 2:00:00 AM invalid invalid invalid 3:00:00 AM +1h UTC-5h CDT

So the Joda API will raise an exception if you try to initialize 2 AM on the day of daylight saving start in CST. Programmatically this problem can be solved by initializing the instant as LocalDateTime and check if it is in a daylight saving gap for a specified time zone. If the instant is located in such a gap one can convert that instant to be either the next or previous hour. This gives great flexibility and makes the behavior explicit and customizable to the application’s needs.

Behavior implemented by Joda API for daylight saving period end CST/CDT

Time DST UTC offset Time Zone 0:00:00 AM +1h UTC-5h CDT 0:59:59 AM +1h UTC-5h CDT 1:00:00 AM +1h UTC-5h CDT 2:00:00 AM No UTC-6h CST

So the Joda Time API tries a different approach compared to the JDK. It takes the 1 AM in CDT instead of CST so that the period ends with 2 AM as specified by the time zone database, though there is no 1 AM is CST.

Tips

There a few things which are quite useful to know when working with Joda Time.

If you want to add milliseconds to an object you would naturally use plusMilliseconds(int), but integer is very often the wrong type when working with milliseconds so you should use plus(long) instead.

The default chronology in Joda Time is ISOChronology. It is used when no chronology is explicitly specified, for example when initializing a DateTime object. This can be confusing when initializing Joda classes from GregorianCalendar instances because they will have the GJChronology set. So for example those two objects are not equal:

DateTime dateTimeWithGJ = new DateTime(new GregorianCalendar(2012, 11, 31, 0, 0, 0, 0)); DateTime dateTimeWithISO = new DateTime(2012, 12, 31, 0, 0, 0, 0); dateTimeWithGJ.equals(dateTimeWithISO); // will return false

What about JSR-310?

JSR-310 (java specification request) is the request to finally solve the Date/Calendar debacle in Java by adopting to the ideas and principles provided by Joda Time API. Actually one of the three lead developers is Stephen Colebourne, who is the founder and lead of Joda Time and other Joda APIs. Current e.t.a. of that JSR is Java SE 8.

Overview

Aspect JDK Joda Time convenience got better with Calendar very good performance needs external synchronization in multithreaded environments performs better than Date, Calendar and TimeZone in most use cases, specifically in multithreaded environments precision

seconds with Date

milliseconds with Calendar

milliseconds immutability Date and Calendar are mutable provides immutable and mutable classes calendar

no support for Date

hard coded support for Gregorian, Japanese Imperial and Buddhist with Calendar

all major calendars, easily extendable internationalization

no support with Date

possible with Calendar

good support thread safety no, needs external synchronization through immutable classes and thread safe formatter parameter validation no, silently rolls to the next best value or a default value yes time zone

possible with Date, but very difficult to handle

got a lot better with Calendar

good support and provides data structures without time zones daylight saving very difficult, since it does not stick to the time zone database specification asserts on invalid dates and sticks as close as possible to the time zone database specification (switches back to standard time in the specified hour) leap years yes yes leap seconds no no, but extension is possible

[codereligion-blog]

Hidden feature of Stripes' NumberTypeConverterSupport

A little background

Stripes offers the following basic TypeConverter implementations:

BigDecimalTypeConverter

BigIntegerTypeConverter

BooleanTypeConverter

ByteTypeConverter

CharacterTypeConverter

CreditCardTypeConverter

DateTypeConverter

DoubleTypeConverter

EmailTypeConverter

EnumeratedTypeConverter

FloatTypeConverter

IntegerTypeConverter

LongTypeConverter

ObjectTypeConverter

OneToMnyTypeConverter

PercentageTypeConverter

ShortTypeConverter

StringTypeConverter

They are used to convert/bind the request parameter values to the according java types during the bind and validation event phase. A programmer may also register his own TypeConverter for a specific request parameter by specifying the converter in the @Validate annotation or he may register the TypeConverter globally by using the DefaultTypeConverterFactory.

Where it gets interesting

The following TypeConverters extend the class NumberTypeConverterSupport:

BigDecimalTypeConverter

BigIntegerTypeConverter

ByteTypeConverter

DoubleTypeConverter

FloatTypeConverter

IntegerTypeConverter

LongTypeConverter

PercentageTypeConverter

ShortTypeConverter

The documentation of the NumberTypeConverterSupport states that this class does the following:

Provides the basic support for converting Strings to non-floating point numbers (i.e. shorts, integers, and longs).

Despite the fact that there are also implementations which support floating point data types, one may think what is common about converting numbers and what does this class actually provide?

Basically it does following:

it pre-processes the request parameter string by:

trimming it with String.trim()

removing the current locale’s currency symbol

trimming again

removing parentheses

if there were parentheses prepending a minus sign

it parses the string into a Number according to the current locale’s NumberFormat

So the hidden feature is that Stripes assumes that per default every BigDecimal, BigInteger, Byte, Double, Float, Integer, Long, Short and percentage storing value is a currency/monetary value. By the way, parenthesis around a value are a common way in finance/accounting to express negative values, so that is what it means to replace parenthesis with a minus sign.

Why is that bad?

You may have already noticed that I am not really thinking this is a feature, but why can this “feature” actually be a problem? The problem lies, as always, in the details. First of all I do not expect this behaviour from the above mentioned type converters and second this can have weird effects when trying to validate input data.

Consider the following:

A user enters something like “ $ ( 365 ) ” into a field which represents a number of days as Integer. The logic will silently convert the given value to “-365” without giving any error message that the given input is not a valid integer.

In my humble opinion this is quite unexpected and annoying.

How to solve the issue

The only way to solve this issue is to globally override the above mentioned sub-classes of NumberTypeConverterSupport by using the DefaultTypeConverterFactory in order to remove this wicket default behaviour.

[codereligion-blog]

Code Smells

What is a "Code Smell"?

The term “Code Smell” was first mentioned by Kent Beck and goes back in the late 1990s. Martin Fowler gave it so more thoughts and developed the term further in his book Refactoring: Improving the Design of Existing Code.

A code smell is actually hard to describe, as it is with real smells. It is something about a piece of code that just does not feel right, though you can not always precisely describe what it is.

I like description of Brandon Keepers:

A code smell is a surface indication that usually corresponds to a deeper problem in the system.

So one could say code smells represent known patterns or better “anti-patterns” and heuristics which do indicate the need for refactoring. The important part about this is, that they only indicate, but not determine that there is a definite problem. So a code smell must nearly always be evaluated on a case by case basis.

Differentiation with “Anti-Patterns”

So what is the difference between code smells and anti-patterns? In literature and general linguistic usage the terms are often used synonymously. I think that code smells are actually a subset of anti-patterns, because anti patterns tend to be more abstract and are not only specific to code. For example wikipedia states that there are the following categories of anti-patterns:

organizational

project management

software design

object-oriented design

programming

methodological

configuration management

Where does smell come from?

You probably know that feeling when you finally finished a project. You did not only fit the requirements some manager imposed on you, but you made some real nice piece of code. You feel pride. You feel you did something good, something that other people or even other programmers (yeah there are people too) can enjoy.

… but did you write tests? Did you write sufficient documentation? Did you really made the code “maintainable”?

Chances are that you did not write the best code the world has ever seen. Actually most code is already smelling when it is shipped the first time. This may come from inexperienced programmers, lack of time to refactor or even through requirements which could not be implemented any better due to external dependencies.

Everything starts smelling eventually. Even the best code starts smelling as soon as the newby programmer needs to hack some “urgent feature” into a glorified master piece. This is no need to worry. You need to accept that everything smells, yeah some things do smell more than others but trust me:

It is not all shit what stinks.

Programming is not about making the perfect code, but about keeping the chaos on a good and maintable level.

Examples

Duplicate Code

Some of you may know the term “DRY” which stands for “Don’t Repeat Yourself” which was established by Andy Hunt and Dave Thomas in their book The Pragmatic Programmer. This principle states that:

Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.

So why should we stick to such a principle? In short term there is not much benefit, it even takes longer to be developed. In order to follow that principle you need to think about concepts which introduce abstraction and indirection. You may even end up developing sub-modules and thus introducing more complexity and more dependencies. More dependencies means more communication overhead and more modules to release.

So the natural thing to do is actually not to design code to be DRY, but to structure development cycles so that you will effectively detect “WET”1 code and make it DRY.

Try to find logic that is used in multiple places, especially look out for business logic, error prone and/or overly complicated logic and algorithms. You know what I mean. That pieces of code you definitely want to see only once in the whole application, because a single instance of this logic already makes your brain boil.

A good example for that is working with money/currencies. So you may have different places in your application(s) working with money. Usually you will do conversions and calculations in the backend, the middleware and even the frontend. Try to extract as much of this logic to common components as possible, because then you have one place to test it and make sure that all the common mistakes will at least only be located in one place.

Did he say common mistakes? Yes I did, you may be surprised but floating point data types like float and double are the most used data types when working with money and are highly error prone due to accuracy problems in rounding.

So you may ask now how does duplicate code get produced in the first place? I mean after all when I need to write something that is already existing I will immediately try to refactor the existing code in order to avoid duplicate code creation.

There are a few problems which prevent this ideal world to exist. The first is that a company may have multiple groups of developers working on similar problems in different areas/projects. This is redundancy on a complete different level. Another cause of redundancy may come from refactoring which introduces redundancy which was not visible/existing before. The good thing about this is that you can always continue and make it even better. Refactoring is always to be seen as something incremental.

The last source of duplicate code, which is actually my favorite, is just plain old laziness, aka. “copy-paste-programming”. We have to constantly remind ourselves that whenever we start copy-pasting something it is usually something wrong about what we are currently doing.

So how can we detect duplicate code? The answer to this question is: Learn your domain language and application structure, because that is the only thing (despite some useful tools) which makes you aware of duplication. Review and refactor your code with every iteration.

Do not be afraid to also work on parts of the application which you do not know yet. Pair with an expert in that area and extend your knowledge.

Big Units

“Big Units” is actually not an official term, but more an aggregation of “Long Method” and “Large Class”. I like to handle these two together, because they are pretty much alike.

So what is bad about big chunks of code, be it a method or a whole class?

The bad thing about big units is that they will most likely do more than they should. This means they handle different concerns in one place. This may have been convenient at one point but it will definitely be a pain in the ass when you start unit testing or if you do not do unit testing at least when you have to maintain the code.

There are warning signs you could look out for to avoid big units.

Gut Feeling - if it does not feel right, refactor!

Documentation - if it is hard to describe without conjunctions (and, or), refactor!

Unit Testing - if it is hard to test, refactor!

Maintenance - if it is hard to add features or debug, refactor!

So big units arise from disobeying or simply skipping the previously mentioned warnings. When you find yourself skipping one of these rules you better have a really good excuse.

Deadlines and code freezes for example are always good and valid excuses.

The best software does not earn a dime if it is not released to the customer.

The important thing is to be aware of “the skipping” and to take actions in order to solve the problem later. That can be a simple // TODO inside the code, a post-it on your desk or even a ticket in your ticket/bug-tracking system. Eventually there is always a little time for refactoring, even when it is only in babysteps.

When a code freeze is coming up and I discover code which needs to be refactored, I will postpone the refactoring for the time the QA executes their tests. Usually a code freeze means that a RC (release candidate) branch will be created on which the QA will do the testing. So as I am waiting for QA tickets to pop up I will implement my refactoring on another branch or even the trunk. I will prioritize QA tickets, but in the “free-time” I will do something useful and improve the code or increase the (unit) test coverage.

So how do we actually refactor big units when we discover them? The most important and likewise difficult thing to take care of is management of state. The more state variants you have and the more state is shared between sub-components the more difficult it gets to tear them apart. Manage your state by keeping your scope tight.

This means:

keep shared class members at a minimum

define interfaces as lean as possible, this includes input parameters, return values and side effects

reduce method side effects

avoid state over multiple members/variables

do not assign multiple values to one reference inside methods (java’s final modifier is your friend)

If your scope is tight, refactoring feels like playing with a rubik cube. If it is not, it feels like someone super-glued a few pieces together. Not so much fun.

After the scope is tightened the next step is to separate concerns. This means restructuring the code so that you can easily document each method and tell in a few lines what it does without using too much conjunctions.

Now it is time to extract, modularize and compose the logic. Use services, managements, facades and what ever the Gang Of Four has to offer to beautify your code structure and do not forget to document and test all through the way.

One last word of warning about this code smell: Do not go all nuts on slicing up your application, because there is also a code smell which is called “Lazy Unit”, which is the adversary of a big unit. Keep in mind that more abstraction and more indirection can also be seen as “code scattering” and can even make the code more difficult to understand.

Upcoming...

There is quite a bunch of code smells out there and I will try to write about some of them in my next posts. Here is a short preview of what I would still like to talk about:

too many parameters

dead code

primitive obsession & data clumps

feature envy

inappropriate intimacy

indecent exposure

refused bequest

contrived complexity

speculative generality

message chains

divergent change

shotgun surgery

empty catch clauses

arrow anti pattern

comments (not documentation)

conditional complexity

combinatorial explosion

middleman

parallel inheritance hierarchies

inconsistent names

uncommunicative names

excessively long identifier

excessively short identifier

excessive use of literals

How To Smell

The ability to smell can be achieved in many ways. Reading books and blogs about coding, best practices, effectiveness, refactoring, restructuring and alike topics helps you to gain some basics first. Get to know all aspects of the domain and the structure of your application is the next step.

Use documentation and testing to force yourself to reflect what you did. Pair with other developers which are less experienced and share your knowledge with them, also do it the other way around. You can also check out open source projects or other projects owned by your company and have a look how other people code.

Finally develop your own best practices and share it with the team, the department and maybe even the whole company.

There are also many useful tools which help you to “measure” your projects code quality. Some may already come with your IDE like “Compiler Warnings” in Eclipse or “Code Inspections” in Idea. Others can be integrated additionally into the IDE of your choice and your build server system. Examples are:

FindBugs: used for static byte code analysis to find “bugs”

PMD: used for static source code analysis to find “bugs”

Checkstyle: used for static source code analysis to validate “style”

Stan4j: structure analyser

Structure101: structure analyser

to be continued...

References & Further Information

Books

Effective Java by Joshua Bloch

Clean Code: A Handbook of Agile Software Craftsmanship by Robert C. Martin

The Pragmatic Programmer by Andy Hunt and Dave Thomas

Refactoring, Improving The Design Of Existing Code by Martin Fowler

Links

http://c2.com/cgi/wiki?CodeSmell

http://www.codinghorror.com/blog/2006/05/code-smells.html

http://www.soberit.hut.fi/mmantyla/BadCodeSmellsTaxonomy.htm

http://opensoul.org/blog/archives/2012/05/23/why-our-code-smells/

http://en.wikipedia.org/wiki/Code_smell

http://www.soberit.hut.fi/~mmantyla/ESE_2006.pdf

http://sourcemaking.com/

http://wiki.java.net/bin/view/People/SmellsToRefactorings

1 hell yeah, yet another acronym, this one stands for “Write Everything Twice”

[codereligion-blog]

Beware of Stripes' ErrorsTag

A little background

Stripes provides a tag called ErrorsTag which could be used like this:

<stripes:form action="/foo/first.action"> <stripes:errors/> ... </stripes:form>

The default setting is to render all errors which includes global errors and field errors. It can be customized to display only one of each.

<stripes:form action="/foo/fist.action"> <stripes:errors globalErrorsOnly="true"/> <table> <tr> <td>Username:</td> <td> <stripes:text name="username"/> <stripes:errors field="username"/> </td> </tr> </table> </stripes:form>

It is also possible to define some custom header and footer.

<stripes:errors> <stripes:errors-header><div class="errorHeader">Validation Errors</div><ul></stripes:errors-header> <li><stripes:individual-error/></li> <stripes:errors-footer></ul></stripes:errors-footer> </stripes:errors>

The problem

Some of you may yawn now and say... dooh I know that already. So here is the part where it gets interesting.

When we were implementing a feature which was rendering a jsp inside a loop, we were having very strange problems while testing the error messages.

Here is an simplified representation of what we did:

<stripes:form action="SomeActionBean.action"> <c:forEach items="${actionBean.items}" var="item" varStatus="loopStatus"> <c:set var="index" value="${loopStatus.index}" /> <%@ include file="item.jsp"%> </c:forEach> </stripes:form>

Inside the item.jsp we were referencing the item and the index variable like this:

<stripes:errors field="items[${index}].label""/> <stripes:label for="item${index}" name="item[${index}].label"/> <stripes:text id="item${index}" name="items[${index}].label" value="${item.label}"/>

In our case the form does not only render already persistent items, it also allows adding new ones. This is achieved by some fancy jquery logic which will render the content of item.jsp into the dom and fill it with some default data when the user clicks an “add-button”.

Everything about this approach seems normal. We had no problems creating items nor displaying already persistent items.

The trouble started when we were doing manual testing for error message. The test could be described as follows:

open the form with one persistent item

add a new item (leave required fields empty)

submit form

check that error message is displayed for the empty required field

The error message was displayed in the right place but to our surprise the empty field was not empty anymore. It contained the data from the already persistent item. The rendered html was actually showing that the field showing the error message had the wrong index.

So we got a rendered html which looked like this.

<label for="label0">Label</label> <input id="label0" name="items[0].label" value="some label" type="text"/> Please provide a label. <label for="label0">Label</label> <input id="label0" name="items[0].label" value="some label" class="error" type="text"/>

While we were actually expecting it to be like this:

<pre name="code" class="xml"> <label for="label0">Label</label> <input id="label0" name="items[0].label" value="some label" type="text"/> Please provide a label. <label for="label1">Label</label> <input id="label1" name="items[1].label" value="" class="error" type="text"/>

The cause

Our first thought was that this must be some server side mixup when processing the data. Sadly it was not. So we started debugging the jsp rendering and hell was that painful. Infact we were not able to identify how the weird html could be produced.

We knew that something was changing the index while we were iterating over the items, but the logic was pretty straight forward and had no side effects to the index variable.

For a few seconds we thought about race conditions, but since jsp rendering is sequential there should not be any concurrency issues.

Than a lightning struck me and I remembered how a former colleague of mine was cursing about the ErrorsTag of Stripes and that it did something very terrible with the page scope. So I went into the ErrorsTag and found this:

/** Sets the context variables for the current error and index */ public void doInitBody() throws JspException { // Apply TEI attributes getPageContext().setAttribute("index", this.index); getPageContext().setAttribute("error", this.currentError); } /** * Manages iteration, running again if there are more errors to display. If there is no * nested FieldError tag, will ensure that the body is evaluated only once. * * @return EVAL_BODY_TAG if there are more errors to display, SKIP_BODY otherwise */ public int doAfterBody() throws JspException { if (this.display && this.nestedErrorTagPresent && this.errorIterator.hasNext()) { this.currentError = this.errorIterator.next(); this.index++; // Reapply TEI attributes getPageContext().setAttribute("index", this.index); getPageContext().setAttribute("error", this.currentError); return EVAL_BODY_BUFFERED; } else { return SKIP_BODY; } }

So what happens here? The Stripes tag actually overrides our helper variable index for their own purposes and does not care to preserve the value afterwards.

The fix

We fixed it by renaming our variable to something like itemIndex but one could also fix the root problem and talk to the Stripes guys if they could find a way to preserve the index and re-assign it after the ErrorsTag has finished his tasks.

So far I could not find that this is a known error, but I will spend some more time investigating and maybe open a bug ticket in their jira.

Conclusion

Until this thing is fixed consider index and error Stripes reserved words and do not use them as page scoped variables when working with ErrorsTag in your jsps.

[codereligion-blog]

Beware of Java Beans Introspector

A Little Background

The package java.beans is part of Java Standard Edition since quite the beginning. Everybody has some understanding of what a JavaBean is. It is a convention which defines that java objects provide the following:

a default constructor

getters and setters to access members

support for serialization

The main reason for this convention and the framework around it was to ease the instantiation and handling of GUI classes in AWT and swing. So most of you which have worked with a Java desktop GUI framework may know classes like PropertyChangeListener and PropertyEditor.

A very convenient part of the JavaBeans API is the introspection. It allows analyzing methods, parameters and properties of a Java class and provides merged information about these in so called descriptors.

The introspection is used in the roots of frameworks which need to analyze classes and instantiate them without actually knowing them. A few prominent examples are Spring and Stripes.

The Problem

A few weeks back I stumbled upon a weird and really hard to reproduce bug. The surface indication was that the frontend framework, which is Stripes, suddenly could not map some POST parameters to Java objects anymore. This bug only occurred on the test servers but never on local environments. After some intense remote debugging sessions I figured out that the cause was located in the java.beans.Introspector class utilized by Stripes to instantiate objects for request binding. For some reason the information returned by the introspector told Stripes that there are no setters for some properties.

The Cause

Recently generics have been added to a few classes used in request binding. Here is an simplified example of such a class:

public class User implements Identifiable<Integer> { private Integer id; public Integer getId() { return id; } public void setId(Integer id) { this.id = id; } }

The belonging interface:

public interface Identifiable<T> { T getId(); }

The setter has been left out on the interface because the services interacting with "Identifiables" should not have the ability to set the id.

In Java generics are only known at compile time and are removed by the compiler through type erasure. This design decision has been made for the sake of backward compatibility, as most of the greatest flaws in software history.

The import thing to know is that the compiler will replace generic types by their bounds or Object (if unbounded), it will also insert type casts where necessary and will generate synthetic bridge methods to preserve polymorphism in extended generic types.

Knowing that, one can understand the output of the following:

for (Method method : User.class.getMethods()) { System.out.println(method.toString()); }

Output with Java Version 1.6.0_29:

public void User.setId(java.lang.Integer) public java.lang.Integer User.getId() public java.lang.Object User.getId() //… the rest has been omitted for readability

The compiler added another method getId() to the User which has the return type Object. Though a programmer is forbidden to overload method return types the compiler can do that.

So the actual class now has two getters which return different types. The newly added getter is called a bridge method and will delegate every call to the Integer getter while casting the return type explicitly to Integer.

The actual root cause of the missing setter in the Stripes logic resides in the logic inside the java.beans.Introspector which could be simplified as:

iterate over all methods and create PropertyDescriptors for each

iterate over all getter PropertyDescriptors

remove redundant getter by merging them so that the last added getter for a property wins

iterate over all setter PropertyDescriptors and merge them into their appropriate getter PropertyDescriptors

Since the last getter method wins, the return type of this method will be made the type of the PropertyDescriptor and the logic may not find an appropriate setter because the types do not match. Tests with different Java versions showed different results in the order of methods and so different results of the following test:

@Test public void test() throws Exception { final Class<User> clazz = User.class; final BeanInfo beanInfo = Introspector.getBeanInfo(clazz); final PropertyDescriptor[] propertyDescriptors = beanInfo.getPropertyDescriptors(); for (final PropertyDescriptor propertyDescriptor : propertyDescriptors) { if (propertyDescriptor.getName().equals("id")) { assertEquals(Integer.class, propertyDescriptor.getPropertyType()); assertNotNull(propertyDescriptor.getReadMethod()); assertNotNull(propertyDescriptor.getWriteMethod()); } } }

Results for Java version 1.5.0_09:

assert: SUCCESS

assert: SUCCESS

assert: SUCCESS

Results for Java version 1.6.0_29:

assert: FAIL

assert: SUCCESS

assert: FAIL

Results for Java version 1.7.0:

assert: SUCCESS

assert: SUCCESS

assert: SUCCESS

Until Java version 1.71 there is no specific logic inside the Introspector which handles bridge methods. So the success of this test solely depends on the right order of methods, which only the compiler has the power of.

How to fix it

Upgrading to Java 1.7

If one has the freedom to do that, great! Despite the bug fixes Java 1.7 brings a lot of nice new features.

Wrap or Avoid Introspector

Usually upgrading to a new major Java versions is a thing which takes a while or may even be forbidden by company policies. It can also just not be possible because the software is a framework and needs to be run against different versions of Java (e.g. Spring).

So the solution is usually to not use the java.beans.Introspector at all or wrap around it to fix the problem. Spring2 and Stripes3 and most certainly other projects do exactly that.

Avoid Asymmetry

When possible make getters and setters in classes and interfaces symmetrically. This will not fix the root cause that one might get a PropertyDescriptor for the wrong type e.g. Object instead of Integer, but at least one would have a setter AND a getter.

This approach actually solved my problem, though Stripes is aware of the issue and tries to work around it, it does not cover all cases.

Conclusion

When using generics and java.beans.Introspector in Java versions below 1.7 be aware of unwanted side effects like missing setters or wrong types. In general beware of type erasure when working with generics especially in combination with reflection.

1 http://bugs.sun.com/view_bug.do?bug_id=6528714

2 org.springframework.beans.GenericTypeAwarePropertyDescriptor

3 net.sourceforge.stripes.util.ReflectUtil

[codereligion-blog]

Final variable may already have been assigned

You may have encountered this javac compiler error when trying to code something like this

public void work() { final byte[] content; try { content = readContent(); } catch (IOException e) { content = getDefaultContent(); } // do stuff with content here }

What happens now is, that the compiler complains about the second assignment of content. If you are a fan of The Final Story (Chapter 2 of Hardcore Java) as big as we are, than getting rid of final is not an option.

Do this instead

private byte[] readContentIfPossible() { try { return readContent(); } catch (IOException e) { return getDefaultContent(); } } public void work() { final byte[] content = readContentIfPossible(); // do stuff with content here }