Refactoring SerialDate

If you go to http://www.jfree.org/jcommon/index.php, you will find the JCommon library.

Deep within that library there is a package named org.jfree.date. Within that package

there is a class named SerialDate. We are going to explore that class.

The author of SerialDate is David Gilbert. David is clearly an experienced and competent

programmer. As we shall see, he shows a significant degree of professionalism and

discipline within his code. For all intents and purposes, this is "good code." And I am

going to rip it to pieces.

268 Chapter 16: Refactoring SerialDate

This is not an activity of malice. Nor do I think that I am so much better than David

that I somehow have a right to pass judgment on his code. Indeed, if you were to find some

of my code, I'm sure you could find plenty of things to complain about.

No, this is not an activity of nastiness or arrogance. What I am about to do is nothing

more and nothing less than a professional review. It is something that we should all be

comfortable doing. And it is something we should welcome when it is done for us. It is

only through critiques like these that we will learn. Doctors do it. Pilots do it. Lawyers do

it. And we programmers need to learn how to do it too.

One more thing about David Gilbert: David is more than just a good programmer.

David had the courage and good will to offer his code to the community at large for free.

He placed it out in the open for all to see and invited public usage and public scrutiny. This

was well done!

SerialDate (Listing B-1, page 349) is a class that represents a date in Java. Why have

a class that represents a date, when Java already has java.util.Date and

java.util.Calendar, and others? The author wrote this class in response to a pain that I

have often felt myself. The comment in his opening Javadoc (line 67) explains it well. We

could quibble about his intention, but I have certainly had to deal with this issue, and I

welcome a class that is about dates instead of times.

First, Make It Work

There are some unit tests in a class named SerialDateTests (Listing B-2, page 366). The

tests all pass. Unfortunately a quick inspection of the tests shows that they don't test everything

[T1]. For example, doing a "Find Usages" search on the method MonthCodeToQuarter

(line 334) indicates that it is not used [F4]. Therefore, the unit tests don't test it.

So I fired up Clover to see what the unit tests covered and what they didn't. Clover

reported that the unit tests executed only 91 of the 185 executable statements in SerialDate

(~50 percent) [T2]. The coverage map looks like a patchwork quilt, with big gobs of unexecuted

code littered all through the class.

It was my goal to completely understand and also refactor this class. I couldn't do that

without much greater test coverage. So I wrote my own suite of completely independent

unit tests (Listing B-4, page 374).

As you look through these tests, you will note that many of them are commented out.

These tests didn't pass. They represent behavior that I think SerialDate should have. So as

I refactor SerialDate, I'll be working to make these tests pass too.

Even with some of the tests commented out, Clover reports that the new unit tests are

executing 170 (92 percent) out of the 185 executable statements. This is pretty good, and I

think we'll be able to get this number higher.

The first few commented-out tests (lines 23-63) were a bit of conceit on my part. The

program was not designed to pass these tests, but the behavior seemed obvious [G2] to me.

First, Make It Work 269

I'm not sure why the testWeekdayCodeToString method was written in the first place, but

because it is there, it seems obvious that it should not be case sensitive. Writing these tests

was trivial [T3]. Making them pass was even easier; I just changed lines 259 and 263 to

use equalsIgnoreCase.

I left the tests at line 32 and line 45 commented out because it's not clear to me that

the "tues" and "thurs" abbreviations ought to be supported.

The tests on line 153 and line 154 don't pass. Clearly, they should [G2]. We can easily

fix this, and the tests on line 163 through line 213, by making the following changes to the

stringToMonthCode function.

The commented test on line 318 exposes a bug in the getFollowingDayOfWeek method

(line 672). December 25th, 2004, was a Saturday. The following Saturday was January 1st,

2005. However, when we run the test, we see that getFollowingDayOfWeek returns December

25th as the Saturday that follows December 25th. Clearly, this is wrong [G3],[T1]. We

see the problem in line 685. It is a typical boundary condition error [T5]. It should read as

follows:

It is interesting to note that this function was the target of an earlier repair. The change

history (line 43) shows that "bugs" were fixed in getPreviousDayOfWeek, getFollowing-

DayOfWeek, and getNearestDayOfWeek [T6].

The testGetNearestDayOfWeek unit test (line 329), which tests the getNearestDayOfWeek

method (line 705), did not start out as long and exhaustive as it currently is. I added a lot

of test cases to it because my initial test cases did not all pass [T6]. You can see the pattern

of failure by looking at which test cases are commented out. That pattern is revealing [T7].

It shows that the algorithm fails if the nearest day is in the future. Clearly there is some

kind of boundary condition error [T5].

The pattern of test coverage reported by Clover is also interesting [T8]. Line 719

never gets executed! This means that the if statement in line 718 is always false. Sure

enough, a look at the code shows that this must be true. The adjust variable is always negative

and so cannot be greater or equal to 4. So this algorithm is just wrong.

457 if ((result < 1) || (result > 12)) {

result = -1;

458 for (int i = 0; i < monthNames.length; i++) {

459 if (s.equalsIgnoreCase(shortMonthNames[i])) {

460 result = i + 1;

461 break;

462 }

463 if (s.equalsIgnoreCase(monthNames[i])) {

464 result = i + 1;

465 break;

466 }

467 }

468 }

685 if (baseDOW >= targetWeekday) {

270 Chapter 16: Refactoring SerialDate

The right algorithm is shown below:

Finally, the tests at line 417 and line 429 can be made to pass simply by throwing an

IllegalArgumentException instead of returning an error string from weekInMonthToString

and relativeToString.

With these changes all the unit tests pass, and I believe SerialDate now works. So now

it's time to make it "right."

Then Make It Right

We are going to walk from the top to the bottom of SerialDate, improving it as we go

along. Although you won't see this in the discussion, I will be running all of the JCommon

unit tests, including my improved unit test for SerialDate, after every change I make. So

rest assured that every change you see here works for all of JCommon.

Starting at line 1, we see a ream of comments with license information, copyrights,

authors, and change history. I acknowledge that there are certain legalities that need to be

addressed, and so the copyrights and licenses must stay. On the other hand, the change history

is a leftover from the 1960s. We have source code control tools that do this for us now.

This history should be deleted [C1].

The import list starting at line 61 could be shortened by using java.text.* and

java.util.*. [J1]

I wince at the HTML formatting in the Javadoc (line 67). Having a source file with

more than one language in it troubles me. This comment has four languages in it: Java,

English, Javadoc, and html [G1]. With that many languages in use, it's hard to keep things

straight. For example, the nice positioning of line 71 and line 72 are lost when the Javadoc

is generated, and yet who wants to see <ul> and <li> in the source code? A better strategy

might be to just surround the whole comment with <pre> so that the formatting that is

apparent in the source code is preserved within the Javadoc.1

Line 86 is the class declaration. Why is this class named SerialDate? What is the significance

of the world "serial"? Is it because the class is derived from Serializable? That

doesn't seem likely.

int delta = targetDOW - base.getDayOfWeek();

int positiveDelta = delta + 7;

int adjust = positiveDelta % 7;

if (adjust > 3)

adjust -= 7;

return SerialDate.addDays(adjust, base);

1. An even better solution would have been for Javadoc to present all comments as preformatted, so that comments appear the

same in both code and document.

Then Make It Right 271

I won't keep you guessing. I know why (or at least I think I know why) the word

"serial" was used. The clue is in the constants SERIAL_LOWER_BOUND and

SERIAL_UPPER_BOUND on line 98 and line 101. An even better clue is in the comment

that begins on line 830. This class is named SerialDate because it is implemented using a

"serial number," which happens to be the number of days since December 30th, 1899.

I have two problems with this. First, the term "serial number" is not really correct.

This may be a quibble, but the representation is more of a relative offset than a serial number.

The term "serial number" has more to do with product identification markers than

dates. So I don't find this name particularly descriptive [N1]. A more descriptive term

might be "ordinal."

The second problem is more significant. The name SerialDate implies an implementation.

This class is an abstract class. There is no need to imply anything at all about the

implementation. Indeed, there is good reason to hide the implementation! So I find this

name to be at the wrong level of abstraction [N2]. In my opinion, the name of this class

should simply be Date.

Unfortunately, there are already too many classes in the Java library named Date, so

this is probably not the best name to choose. Because this class is all about days, instead of

time, I considered naming it Day, but this name is also heavily used in other places. In the

end, I chose DayDate as the best compromise.

From now on in this discussion I will use the term DayDate. I leave it to you to remember

that the listings you are looking at still use SerialDate.

I understand why DayDate inherits from Comparable and Serializable. But why does it

inherit from MonthConstants? The class MonthConstants (Listing B-3, page 372) is just a

bunch of static final constants that define the months. Inheriting from classes with constants

is an old trick that Java programmers used so that they could avoid using expressions

like MonthConstants.January, but it's a bad idea [J2]. MonthConstants should really be

an enum.

public abstract class DayDate implements Comparable,

Serializable {

public static enum Month {

JANUARY(1),

FEBRUARY(2),

MARCH(3),

APRIL(4),

MAY(5),

JUNE(6),

JULY(7),

AUGUST(8),

SEPTEMBER(9),

OCTOBER(10),

NOVEMBER(11),

DECEMBER(12);

Month(int index) {

this.index = index;

}

272 Chapter 16: Refactoring SerialDate

Changing MonthConstants to this enum forces quite a few changes to the DayDate class

and all it's users. It took me an hour to make all the changes. However, any function that

used to take an int for a month, now takes a Month enumerator. This means we can get rid

of the isValidMonthCode method (line 326), and all the month code error checking such as

that in monthCodeToQuarter (line 356) [G5].

Next, we have line 91, serialVersionUID. This variable is used to control the serializer.

If we change it, then any DayDate written with an older version of the software won't be

readable anymore and will result in an InvalidClassException. If you don't declare the

serialVersionUID variable, then the compiler automatically generates one for you, and it

will be different every time you make a change to the module. I know that all the documents

recommend manual control of this variable, but it seems to me that automatic control

of serialization is a lot safer [G4]. After all, I'd much rather debug an

InvalidClassException than the odd behavior that would ensue if I forgot to change the

serialVersionUID. So I'm going to delete the variable—at least for the time being.2

I find the comment on line 93 redundant. Redundant comments are just places to collect

lies and misinformation [C2]. So I'm going to get rid of it and its ilk.

The comments at line 97 and line 100 talk about serial numbers, which I discussed

earlier [C1]. The variables they describe are the earliest and latest possible dates that

DayDate can describe. This can be made a bit clearer [N1].

It's not clear to me why EARLIEST_DATE_ORDINAL is 2 instead of 0. There is a hint in the

comment on line 829 that suggests that this has something to do with the way dates are

represented in Microsoft Excel. There is a much deeper insight provided in a derivative of

DayDate called SpreadsheetDate (Listing B-5, page 382). The comment on line 71 describes

the issue nicely.

The problem I have with this is that the issue seems to be related to the implementation

of SpreadsheetDate and has nothing to do with DayDate. I conclude from this that

public static Month make(int monthIndex) {

for (Month m : Month.values()) {

if (m.index == monthIndex)

return m;

}

throw new IllegalArgumentException("Invalid month index " + monthIndex);

}

public final int index;

}

2. Several of the reviewers of this text have taken exception to this decision. They contend that in an open source framework it is

better to assert manual control over the serial ID so that minor changes to the software don't cause old serialized dates to be

invalid. This is a fair point. However, at least the failure, inconvenient though it might be, has a clear-cut cause. On the other

hand, if the author of the class forgets to update the ID, then the failure mode is undefined and might very well be silent. I

think the real moral of this story is that you should not expect to deserialize across versions.

public static final int EARLIEST_DATE_ORDINAL = 2; // 1/1/1900

public static final int LATEST_DATE_ORDINAL = 2958465; // 12/31/9999

Then Make It Right 273

EARLIEST_DATE_ORDINAL and LATEST_DATE_ORDINAL do not really belong in DayDate and

should be moved to SpreadsheetDate [G6].

Indeed, a search of the code shows that these variables are used only within

SpreadsheetDate. Nothing in DayDate, nor in any other class in the JCommon framework, uses

them. Therefore, I'll move them down into SpreadsheetDate.

The next variables, MINIMUM_YEAR_SUPPORTED, and MAXIMUM_YEAR_SUPPORTED (line 104

and line 107), provide something of a dilemma. It seems clear that if DayDate is an abstract

class that provides no foreshadowing of implementation, then it should not inform us

about a minimum or maximum year. Again, I am tempted to move these variables down

into SpreadsheetDate [G6]. However, a quick search of the users of these variables shows

that one other class uses them: RelativeDayOfWeekRule (Listing B-6, page 390). We see that

usage at line 177 and line 178 in the getDate function, where they are used to check that

the argument to getDate is a valid year. The dilemma is that a user of an abstract class

needs information about its implementation.

What we need to do is provide this information without polluting DayDate itself.

Usually, we would get implementation information from an instance of a derivative.

However, the getDate function is not passed an instance of a DayDate. It does, however,

return such an instance, which means that somewhere it must be creating it. Line 187

through line 205 provide the hint. The DayDate instance is being created by one of the

three functions, getPreviousDayOfWeek, getNearestDayOfWeek, or getFollowingDayOfWeek.

Looking back at the DayDate listing, we see that these functions (lines 638–724) all return

a date created by addDays (line 571), which calls createInstance (line 808), which creates

a SpreadsheetDate! [G7].

It's generally a bad idea for base classes to know about their derivatives. To fix this, we

should use the ABSTRACT FACTORY3 pattern and create a DayDateFactory. This factory will

create the instances of DayDate that we need and can also answer questions about the

implementation, such as the maximum and minimum dates.

3. [GOF].

public abstract class DayDateFactory {

private static DayDateFactory factory = new SpreadsheetDateFactory();

public static void setInstance(DayDateFactory factory) {

DayDateFactory.factory = factory;

}

protected abstract DayDate _makeDate(int ordinal);

protected abstract DayDate _makeDate(int day, DayDate.Month month, int year);

protected abstract DayDate _makeDate(int day, int month, int year);

protected abstract DayDate _makeDate(java.util.Date date);

protected abstract int _getMinimumYear();

protected abstract int _getMaximumYear();

public static DayDate makeDate(int ordinal) {

return factory._makeDate(ordinal);

}

274 Chapter 16: Refactoring SerialDate

This factory class replaces the createInstance methods with makeDate methods, which

improves the names quite a bit [N1]. It defaults to a SpreadsheetDateFactory but can be

changed at any time to use a different factory. The static methods that delegate to abstract

methods use a combination of the SINGLETON,4 DECORATOR,5 and ABSTRACT FACTORY

patterns that I have found to be useful.

The SpreadsheetDateFactory looks like this.

public static DayDate makeDate(int day, DayDate.Month month, int year) {

return factory._makeDate(day, month, year);

}

public static DayDate makeDate(int day, int month, int year) {

return factory._makeDate(day, month, year);

}

public static DayDate makeDate(java.util.Date date) {

return factory._makeDate(date);

}

public static int getMinimumYear() {

return factory._getMinimumYear();

}

public static int getMaximumYear() {

return factory._getMaximumYear();

}

}

4. Ibid.

5. Ibid.

public class SpreadsheetDateFactory extends DayDateFactory {

public DayDate _makeDate(int ordinal) {

return new SpreadsheetDate(ordinal);

}

public DayDate _makeDate(int day, DayDate.Month month, int year) {

return new SpreadsheetDate(day, month, year);

}

public DayDate _makeDate(int day, int month, int year) {

return new SpreadsheetDate(day, month, year);

}

public DayDate _makeDate(Date date) {

final GregorianCalendar calendar = new GregorianCalendar();

calendar.setTime(date);

return new SpreadsheetDate(

calendar.get(Calendar.DATE),

DayDate.Month.make(calendar.get(Calendar.MONTH) + 1),

calendar.get(Calendar.YEAR));

}

Then Make It Right 275

As you can see, I have already moved the MINIMUM_YEAR_SUPPORTED and

MAXIMUM_YEAR_SUPPORTED variables into SpreadsheetDate, where they belong [G6].

The next issue in DayDate are the day constants beginning at line 109. These should

really be another enum [J3]. We've seen this pattern before, so I won't repeat it here. You'll

see it in the final listings.

Next, we see a series of tables starting with LAST_DAY_OF_MONTH at line 140. My first

issue with these tables is that the comments that describe them are redundant [C3]. Their

names are sufficient. So I'm going to delete the comments.

There seems to be no good reason that this table isn't private [G8], because there is a

static function lastDayOfMonth that provides the same data.

The next table, AGGREGATE_DAYS_TO_END_OF_MONTH, is a bit more mysterious because it is

not used anywhere in the JCommon framework [G9]. So I deleted it.

The same goes for LEAP_YEAR_AGGREGATE_DAYS_TO_END_OF_MONTH.

The next table, AGGREGATE_DAYS_TO_END_OF_PRECEDING_MONTH, is used only in SpreadsheetDate

(line 434 and line 473). This begs the question of whether it should be moved

to SpreadsheetDate. The argument for not moving it is that the table is not specific to any

particular implementation [G6]. On the other hand, no implementation other than

SpreadsheetDate actually exists, and so the table should be moved close to where it is

used [G10].

What settles the argument for me is that to be consistent [G11], we should make the

table private and expose it through a function like julianDateOfLastDayOfMonth. Nobody

seems to need a function like that. Moreover, the table can be moved back to DayDate easily

if any new implementation of DayDate needs it. So I moved it.

The same goes for the table, LEAP_YEAR_AGGREGATE_DAYS_TO_END_OF_MONTH.

Next, we see three sets of constants that can be turned into enums (lines 162–205).

The first of the three selects a week within a month. I changed it into an enum named

WeekInMonth.

protected int _getMinimumYear() {

return SpreadsheetDate.MINIMUM_YEAR_SUPPORTED;

}

protected int _getMaximumYear() {

return SpreadsheetDate.MAXIMUM_YEAR_SUPPORTED;

}

}

public enum WeekInMonth {

FIRST(1), SECOND(2), THIRD(3), FOURTH(4), LAST(0);

public final int index;

WeekInMonth(int index) {

this.index = index;

}

}

276 Chapter 16: Refactoring SerialDate

The second set of constants (lines 177–187) is a bit more obscure. The INCLUDE_NONE,

INCLUDE_FIRST, INCLUDE_SECOND, and INCLUDE_BOTH constants are used to describe whether

the defining end-point dates of a range should be included in that range. Mathematically,

this is described using the terms "open interval," "half-open interval," and "closed interval."

I think it is clearer using the mathematical nomenclature [N3], so I changed it to an

enum named DateInterval with CLOSED, CLOSED_LEFT, CLOSED_RIGHT, and OPEN enumerators.

The third set of constants (lines 18–205) describe whether a search for a particular

day of the week should result in the last, next, or nearest instance. Deciding what to call

this is difficult at best. In the end, I settled for WeekdayRange with LAST, NEXT, and NEAREST

enumerators.

You might not agree with the names I've chosen. They make sense to me, but they

may not make sense to you. The point is that they are now in a form that makes them easy

to change [J3]. They aren't passed as integers anymore; they are passed as symbols. I can

use the "change name" function of my IDE to change the names, or the types, without

worrying that I missed some -1 or 2 somewhere in the code or that some int argument declaration

is left poorly described.

The description field at line 208 does not seem to be used by anyone. I deleted it along

with its accessor and mutator [G9].

I also deleted the degenerate default constructor at line 213 [G12]. The compiler will

generate it for us.

We can skip over the isValidWeekdayCode method (lines 216–238) because we deleted

it when we created the Day enumeration.

This brings us to the stringToWeekdayCode method (lines 242–270). Javadocs that

don't add much to the method signature are just clutter [C3],[G12]. The only value this

Javadoc adds is the description of the -1 return value. However, because we changed to the

Day enumeration, the comment is actually wrong [C2]. The method now throws an

IllegalArgumentException. So I deleted the Javadoc.

I also deleted all the final keywords in arguments and variable declarations. As far as

I could tell, they added no real value but did add to the clutter [G12]. Eliminating final

flies in the face of some conventional wisdom. For example, Robert Simmons6 strongly

recommends us to ". . . spread final all over your code." Clearly I disagree. I think that

there are a few good uses for final, such as the occasional final constant, but otherwise

the keyword adds little value and creates a lot of clutter. Perhaps I feel this way because the

kinds of errors that final might catch are already caught by the unit tests I write.

I didn't care for the duplicate if statements [G5] inside the for loop (line 259 and

line 263), so I connected them into a single if statement using the || operator. I also used

the Day enumeration to direct the for loop and made a few other cosmetic changes.

It occurred to me that this method does not really belong in DayDate. It's really the

parse function of Day. So I moved it into the Day enumeration. However, that made the Day

6. [Simmons04], p. 73.

Then Make It Right 277

enumeration pretty large. Because the concept of Day does not depend on DayDate, I moved

the Day enumeration outside of the DayDate class into its own source file [G13].

I also moved the next function, weekdayCodeToString (lines 272–286) into the Day

enumeration and called it toString.

There are two getMonths functions (lines 288–316). The first calls the second. The

second is never called by anyone but the first. Therefore, I collapsed the two into one and

vastly simplified them [G9],[G12],[F4]. Finally, I changed the name to be a bit more selfdescriptive

[N1].

public enum Day {

MONDAY(Calendar.MONDAY),

TUESDAY(Calendar.TUESDAY),

WEDNESDAY(Calendar.WEDNESDAY),s

THURSDAY(Calendar.THURSDAY),

FRIDAY(Calendar.FRIDAY),

SATURDAY(Calendar.SATURDAY),

SUNDAY(Calendar.SUNDAY);

public final int index;

private static DateFormatSymbols dateSymbols = new DateFormatSymbols();

Day(int day) {

index = day;

}

public static Day make(int index) throws IllegalArgumentException {

for (Day d : Day.values())

if (d.index == index)

return d;

throw new IllegalArgumentException(

String.format("Illegal day index: %d.", index));

}

public static Day parse(String s) throws IllegalArgumentException {

String[] shortWeekdayNames =

dateSymbols.getShortWeekdays();

String[] weekDayNames =

dateSymbols.getWeekdays();

s = s.trim();

for (Day day : Day.values()) {

if (s.equalsIgnoreCase(shortWeekdayNames[day.index]) ||

s.equalsIgnoreCase(weekDayNames[day.index])) {

return day;

}

}

throw new IllegalArgumentException(

String.format("%s is not a valid weekday string", s));

}

public String toString() {

return dateSymbols.getWeekdays()[index];

}

}

278 Chapter 16: Refactoring SerialDate

The isValidMonthCode function (lines 326–346) was made irrelevant by the Month

enum, so I deleted it [G9].

The monthCodeToQuarter function (lines 356–375) smells of FEATURE ENVY7 [G14]

and probably belongs in the Month enum as a method named quarter. So I replaced it.

This made the Month enum big enough to be in its own class. So I moved it out of

DayDate to be consistent with the Day enum [G11],[G13].

The next two methods are named monthCodeToString (lines 377–426). Again, we see

the pattern of one method calling its twin with a flag. It is usually a bad idea to pass a flag

as an argument to a function, especially when that flag simply selects the format of the output

[G15]. I renamed, simplified, and restructured these functions and moved them into the

Month enum [N1],[N3],[C3],[G14].

The next method is stringToMonthCode (lines 428–472). I renamed it, moved it into the

Month enum, and simplified it [N1],[N3],[C3],[G14],[G12].

public static String[] getMonthNames() {

return dateFormatSymbols.getMonths();

}

7. [Refactoring].

public int quarter() {

return 1 + (index-1)/3;

}

public String toString() {

return dateFormatSymbols.getMonths()[index - 1];

}

public String toShortString() {

return dateFormatSymbols.getShortMonths()[index - 1];

}

public static Month parse(String s) {

s = s.trim();

for (Month m : Month.values())

if (m.matches(s))

return m;

try {

return make(Integer.parseInt(s));

}

catch (NumberFormatException e) {}

throw new IllegalArgumentException("Invalid month " + s);

}

Then Make It Right 279

The isLeapYear method (lines 495–517) can be made a bit more expressive [G16].

The next function, leapYearCount (lines 519–536) doesn't really belong in DayDate.

Nobody calls it except for two methods in SpreadsheetDate. So I pushed it down [G6].

The lastDayOfMonth function (lines 538–560) makes use of the LAST_DAY_OF_MONTH

array. This array really belongs in the Month enum [G17], so I moved it there. I also simplified

the function and made it a bit more expressive [G16].

Now things start to get a bit more interesting. The next function is addDays (lines 562–

576). First of all, because this function operates on the variables of DayDate, it should not

be static [G18]. So I changed it to an instance method. Second, it calls the function

toSerial. This function should be renamed toOrdinal [N1]. Finally, the method can be

simplified.

The same goes for addMonths (lines 578–602). It should be an instance method [G18].

The algorithm is a bit complicated, so I used EXPLAINING TEMPORARY VARIABLES8 [G19]

to make it more transparent. I also renamed the method getYYY to getYear [N1].

private boolean matches(String s) {

return s.equalsIgnoreCase(toString()) ||

s.equalsIgnoreCase(toShortString());

}

public static boolean isLeapYear(int year) {

boolean fourth = year % 4 == 0;

boolean hundredth = year % 100 == 0;

boolean fourHundredth = year % 400 == 0;

return fourth && (!hundredth || fourHundredth);

}

public static int lastDayOfMonth(Month month, int year) {

if (month == Month.FEBRUARY && isLeapYear(year))

return month.lastDay() + 1;

else

return month.lastDay();

}

public DayDate addDays(int days) {

return DayDateFactory.makeDate(toOrdinal() + days);

}

8. [Beck97].

public DayDate addMonths(int months) {

int thisMonthAsOrdinal = 12 * getYear() + getMonth().index - 1;

int resultMonthAsOrdinal = thisMonthAsOrdinal + months;

int resultYear = resultMonthAsOrdinal / 12;

Month resultMonth = Month.make(resultMonthAsOrdinal % 12 + 1);

280 Chapter 16: Refactoring SerialDate

The addYears function (lines 604–626) provides no surprises over the others.

There is a little itch at the back of my mind that is bothering me about changing

these methods from static to instance. Does the expression date.addDays(5) make it

clear that the date object does not change and that a new instance of DayDate is returned?

Or does it erroneously imply that we are adding five days to the date object? You might

not think that is a big problem, but a bit of code that looks like the following can be very

deceiving [G20].

DayDate date = DateFactory.makeDate(5, Month.DECEMBER, 1952);

date.addDays(7); // bump date by one week.

Someone reading this code would very likely just accept that addDays is changing the

date object. So we need a name that breaks this ambiguity [N4]. So I changed the names to

plusDays and plusMonths. It seems to me that the intent of the method is captured nicely by

DayDate date = oldDate.plusDays(5);

whereas the following doesn't read fluidly enough for a reader to simply accept that the

date object is changed:

date.plusDays(5);

The algorithms continue to get more interesting. getPreviousDayOfWeek (lines 628–

660) works but is a bit complicated. After some thought about what was really going on

[G21], I was able to simplify it and use EXPLAINING TEMPORARY VARIABLES [G19] to

make it clearer. I also changed it from a static method to an instance method [G18], and

got rid of the duplicate instance method [G5] (lines 997–1008).

The exact same analysis and result occurred for getFollowingDayOfWeek (lines 662–693).

int lastDayOfResultMonth = lastDayOfMonth(resultMonth, resultYear);

int resultDay = Math.min(getDayOfMonth(), lastDayOfResultMonth);

return DayDateFactory.makeDate(resultDay, resultMonth, resultYear);

}

public DayDate plusYears(int years) {

int resultYear = getYear() + years;

int lastDayOfMonthInResultYear = lastDayOfMonth(getMonth(), resultYear);

int resultDay = Math.min(getDayOfMonth(), lastDayOfMonthInResultYear);

return DayDateFactory.makeDate(resultDay, getMonth(), resultYear);

}

public DayDate getPreviousDayOfWeek(Day targetDayOfWeek) {

int offsetToTarget = targetDayOfWeek.index - getDayOfWeek().index;

if (offsetToTarget >= 0)

offsetToTarget -= 7;

return plusDays(offsetToTarget);

}

public DayDate getFollowingDayOfWeek(Day targetDayOfWeek) {

int offsetToTarget = targetDayOfWeek.index - getDayOfWeek().index;

if (offsetToTarget <= 0)

Then Make It Right 281

The next function is getNearestDayOfWeek (lines 695–726), which we corrected back

on page 270. But the changes I made back then aren't consistent with the current pattern in

the last two functions [G11]. So I made it consistent and used some EXPLAINING TEMPORARY

VARIABLES [G19] to clarify the algorithm.

The getEndOfCurrentMonth method (lines 728–740) is a little strange because it is an

instance method that envies [G14] its own class by taking a DayDate argument. I made it a

true instance method and clarified a few names.

Refactoring weekInMonthToString (lines 742–761) turned out to be very interesting

indeed. Using the refactoring tools of my IDE, I first moved the method to the WeekInMonth

enum that I created back on page 275. Then I renamed the method to toString. Next, I

changed it from a static method to an instance method. All the tests still passed. (Can you

guess where I am going?)

Next, I deleted the method entirely! Five asserts failed (lines 411–415, Listing B-4,

page 374). I changed these lines to use the names of the enumerators (FIRST,

SECOND, . . .). All the tests passed. Can you see why? Can you also see why each of these

steps was necessary? The refactoring tool made sure that all previous callers of

weekInMonthToString now called toString on the weekInMonth enumerator because all enumerators

implement toString to simply return their names. . . .

Unfortunately, I was a bit too clever. As elegant as that wonderful chain of refactorings

was, I finally realized that the only users of this function were the tests I had just modified,

so I deleted the tests.

Fool me once, shame on you. Fool me twice, shame on me! So after determining that

nobody other than the tests called relativeToString (lines 765–781), I simply deleted the

function and its tests.

offsetToTarget += 7;

return plusDays(offsetToTarget);

}

public DayDate getNearestDayOfWeek(final Day targetDay) {

int offsetToThisWeeksTarget = targetDay.index - getDayOfWeek().index;

int offsetToFutureTarget = (offsetToThisWeeksTarget + 7) % 7;

int offsetToPreviousTarget = offsetToFutureTarget - 7;

if (offsetToFutureTarget > 3)

return plusDays(offsetToPreviousTarget);

else

return plusDays(offsetToFutureTarget);

}

public DayDate getEndOfMonth() {

Month month = getMonth();

int year = getYear();

int lastDay = lastDayOfMonth(month, year);

return DayDateFactory.makeDate(lastDay, month, year);

}

282 Chapter 16: Refactoring SerialDate

We have finally made it to the abstract methods of this abstract class. And the first one

is as appropriate as they come: toSerial (lines 838–844). Back on page 279 I had changed

the name to toOrdinal. Having looked at it in this context, I decided the name should be

changed to getOrdinalDay.

The next abstract method is toDate (lines 838–844). It converts a DayDate to a

java.util.Date. Why is this method abstract? If we look at its implementation in

SpreadsheetDate (lines 198–207, Listing B-5, page 382), we see that it doesn't depend on

anything in the implementation of that class [G6]. So I pushed it up.

The getYYYY, getMonth, and getDayOfMonth methods are nicely abstract. However, the

getDayOfWeek method is another one that should be pulled up from SpreadSheetDate

because it doesn't depend on anything that can't be found in DayDate [G6]. Or does it?

If you look carefully (line 247, Listing B-5, page 382), you'll see that the algorithm

implicitly depends on the origin of the ordinal day (in other words, the day of the week of

day 0). So even though this function has no physical dependencies that couldn't be moved

to DayDate, it does have a logical dependency.

Logical dependencies like this bother me [G22]. If something logical depends on

the implementation, then something physical should too. Also, it seems to me that the

algorithm itself could be generic with a much smaller portion of it dependent on the

implementation [G6].

So I created an abstract method in DayDate named getDayOfWeekForOrdinalZero and

implemented it in SpreadsheetDate to return Day.SATURDAY. Then I moved the getDayOfWeek

method up to DayDate and changed it to call getOrdinalDay and getDayOfWeekForOrdinal-

Zero.

As a side note, look carefully at the comment on line 895 through line 899. Was this

repetition really necessary? As usual, I deleted this comment along with all the others.

The next method is compare (lines 902–913). Again, this method is inappropriately

abstract [G6], so I pulled the implementation up into DayDate. Also, the name does not

communicate enough [N1]. This method actually returns the difference in days since the

argument. So I changed the name to daysSince. Also, I noted that there weren't any tests

for this method, so I wrote them.

The next six functions (lines 915–980) are all abstract methods that should be implemented

in DayDate. So I pulled them all up from SpreadsheetDate.

The last function, isInRange (lines 982–995) also needs to be pulled up and refactored.

The switch statement is a bit ugly [G23] and can be replaced by moving the cases

into the DateInterval enum.

public Day getDayOfWeek() {

Day startingDay = getDayOfWeekForOrdinalZero();

int startingOffset = startingDay.index - Day.SUNDAY.index;

return Day.make((getOrdinalDay() + startingOffset) % 7 + 1);

}

Then Make It Right 283

That brings us to the end of DayDate. So now we'll make one more pass over the whole

class to see how well it flows.

First, the opening comment is long out of date, so I shortened and improved it [C2].

Next, I moved all the remaining enums out into their own files [G12].

Next, I moved the static variable (dateFormatSymbols) and three static methods

(getMonthNames, isLeapYear, lastDayOfMonth) into a new class named DateUtil [G6].

I moved the abstract methods up to the top where they belong [G24].

I changed Month.make to Month.fromInt [N1] and did the same for all the other enums.

I also created a toInt() accessor for all the enums and made the index field private.

There was some interesting duplication [G5] in plusYears and plusMonths that I was

able to eliminate by extracting a new method named correctLastDayOfMonth, making the

all three methods much clearer.

I got rid of the magic number 1 [G25], replacing it with Month.JANUARY.toInt() or

Day.SUNDAY.toInt(), as appropriate. I spent a little time with SpreadsheetDate, cleaning up

the algorithms a bit. The end result is contained in Listing B-7, page 394, through

Listing B-16, page 405.

public enum DateInterval {

OPEN {

public boolean isIn(int d, int left, int right) {

return d > left && d < right;

}

},

CLOSED_LEFT {

public boolean isIn(int d, int left, int right) {

return d >= left && d < right;

}

},

CLOSED_RIGHT {

public boolean isIn(int d, int left, int right) {

return d > left && d <= right;

}

},

CLOSED {

public boolean isIn(int d, int left, int right) {

return d >= left && d <= right;

}

};

public abstract boolean isIn(int d, int left, int right);

}

public boolean isInRange(DayDate d1, DayDate d2, DateInterval interval) {

int left = Math.min(d1.getOrdinalDay(), d2.getOrdinalDay());

int right = Math.max(d1.getOrdinalDay(), d2.getOrdinalDay());

return interval.isIn(getOrdinalDay(), left, right);

}

284 Chapter 16: Refactoring SerialDate

Interestingly the code coverage in DayDate has decreased to 84.9 percent! This is not

because less functionality is being tested; rather it is because the class has shrunk so much

that the few uncovered lines have a greater weight. DayDate now has 45 out of 53 executable

statements covered by tests. The uncovered lines are so trivial that they weren't worth

testing.

Conclusion

So once again we've followed the Boy Scout Rule. We've checked the code in a bit cleaner

than when we checked it out. It took a little time, but it was worth it. Test coverage was

increased, some bugs were fixed, the code was clarified and shrunk. The next person to

look at this code will hopefully find it easier to deal with than we did. That person will also

probably be able to clean it up a bit more than we did.

Bibliography

[GOF]: Design Patterns: Elements of Reusable Object Oriented Software, Gamma et al.,

Addison-Wesley, 1996.

[Simmons04]: Hardcore Java, Robert Simmons, Jr., O'Reilly, 2004.

[Refactoring]: Refactoring: Improving the Design of Existing Code, Martin Fowler et al.,

Addison-Wesley, 1999.

[Beck97]: Smalltalk Best Practice Patterns, Kent Beck, Prentice Hall, 1997.

285

17

Smells and Heuristics

In his wonderful book Refactoring,1 Martin Fowler identified many different "Code

Smells." The list that follows includes many of Martin's smells and adds many more of my

own. It also includes other pearls and heuristics that I use to practice my trade.

1. [Refactoring].

286 Chapter 17: Smells and Heuristics

I compiled this list by walking through several different programs and refactoring

them. As I made each change, I asked myself why I made that change and then wrote the

reason down here. The result is a rather long list of things that smell bad to me when I read

code.

This list is meant to be read from top to bottom and also to be used as a reference.

There is a cross-reference for each heuristic that shows you where it is referenced in the

rest of the text in "Appendix C" on page 409.

Comments

C1: Inappropriate Information

It is inappropriate for a comment to hold information better held in a different kind of system

such as your source code control system, your issue tracking system, or any other

record-keeping system. Change histories, for example, just clutter up source files with

volumes of historical and uninteresting text. In general, meta-data such as authors, lastmodified-

date, SPR number, and so on should not appear in comments. Comments should

be reserved for technical notes about the code and design.

C2: Obsolete Comment

A comment that has gotten old, irrelevant, and incorrect is obsolete. Comments get old

quickly. It is best not to write a comment that will become obsolete. If you find an obsolete

comment, it is best to update it or get rid of it as quickly as possible. Obsolete comments

tend to migrate away from the code they once described. They become floating islands of

irrelevance and misdirection in the code.

C3: Redundant Comment

A comment is redundant if it describes something that adequately describes itself. For

example:

i++; // increment i

Another example is a Javadoc that says nothing more than (or even less than) the function

signature:

/**

* @param sellRequest

* @return

* @throws ManagedComponentException

*/

public SellResponse beginSellItem(SellRequest sellRequest)

throws ManagedComponentException

Comments should say things that the code cannot say for itself.

Environment 287

C4: Poorly Written Comment

A comment worth writing is worth writing well. If you are going to write a comment,

take the time to make sure it is the best comment you can write. Choose your words

carefully. Use correct grammar and punctuation. Don't ramble. Don't state the obvious.

Be brief.

C5: Commented-Out Code

It makes me crazy to see stretches of code that are commented out. Who knows how old it

is? Who knows whether or not it's meaningful? Yet no one will delete it because everyone

assumes someone else needs it or has plans for it.

That code sits there and rots, getting less and less relevant with every passing day. It

calls functions that no longer exist. It uses variables whose names have changed. It follows

conventions that are long obsolete. It pollutes the modules that contain it and distracts the

people who try to read it. Commented-out code is an abomination.

When you see commented-out code, delete it! Don't worry, the source code control

system still remembers it. If anyone really needs it, he or she can go back and check out a

previous version. Don't suffer commented-out code to survive.

Environment

E1: Build Requires More Than One Step

Building a project should be a single trivial operation. You should not have to check many

little pieces out from source code control. You should not need a sequence of arcane commands

or context dependent scripts in order to build the individual elements. You should

not have to search near and far for all the various little extra JARs, XML files, and other

artifacts that the system requires. You should be able to check out the system with one simple

command and then issue one other simple command to build it.

svn get mySystem

cd mySystem

ant all

E2: Tests Require More Than One Step

You should be able to run all the unit tests with just one command. In the best case you

can run all the tests by clicking on one button in your IDE. In the worst case you should

be able to issue a single simple command in a shell. Being able to run all the tests is so

fundamental and so important that it should be quick, easy, and obvious to do.

288 Chapter 17: Smells and Heuristics

Functions

F1: Too Many Arguments

Functions should have a small number of arguments. No argument is best, followed by

one, two, and three. More than three is very questionable and should be avoided with prejudice.

(See "Function Arguments" on page 40.)

F2: Output Arguments

Output arguments are counterintuitive. Readers expect arguments to be inputs, not outputs.

If your function must change the state of something, have it change the state of the

object it is called on. (See "Output Arguments" on page 45.)

F3: Flag Arguments

Boolean arguments loudly declare that the function does more than one thing. They are

confusing and should be eliminated. (See "Flag Arguments" on page 41.)

F4: Dead Function

Methods that are never called should be discarded. Keeping dead code around is wasteful.

Don't be afraid to delete the function. Remember, your source code control system still

remembers it.

General

G1: Multiple Languages in One Source File

Today's modern programming environments make it possible to put many different languages

into a single source file. For example, a Java source file might contain snippets of XML,

HTML, YAML, JavaDoc, English, JavaScript, and so on. For another example, in addition to

HTML a JSP file might contain Java, a tag library syntax, English comments, Javadocs,

XML, JavaScript, and so forth. This is confusing at best and carelessly sloppy at worst.

The ideal is for a source file to contain one, and only one, language. Realistically, we

will probably have to use more than one. But we should take pains to minimize both the

number and extent of extra languages in our source files.

G2: Obvious Behavior Is Unimplemented

Following "The Principle of Least Surprise,"2 any function or class should implement the

behaviors that another programmer could reasonably expect. For example, consider a

function that translates the name of a day to an enum that represents the day.

2. Or "The Principle of Least Astonishment": http://en.wikipedia.org/wiki/

Principle_of_least_astonishment

General 289

Day day = DayDate.StringToDay(String dayName);

We would expect the string "Monday" to be translated to Day.MONDAY. We would also expect

the common abbreviations to be translated, and we would expect the function to ignore

case.

When an obvious behavior is not implemented, readers and users of the code can no

longer depend on their intuition about function names. They lose their trust in the original

author and must fall back on reading the details of the code.

G3: Incorrect Behavior at the Boundaries

It seems obvious to say that code should behave correctly. The problem is that we seldom

realize just how complicated correct behavior is. Developers often write functions that

they think will work, and then trust their intuition rather than going to the effort to prove

that their code works in all the corner and boundary cases.

There is no replacement for due diligence. Every boundary condition, every corner

case, every quirk and exception represents something that can confound an elegant and

intuitive algorithm. Don't rely on your intuition. Look for every boundary condition and

write a test for it.

G4: Overridden Safeties

Chernobyl melted down because the plant manager overrode each of the safety mechanisms

one by one. The safeties were making it inconvenient to run an experiment. The

result was that the experiment did not get run, and the world saw it's first major civilian

nuclear catastrophe.

It is risky to override safeties. Exerting manual control over serialVersionUID may be

necessary, but it is always risky. Turning off certain compiler warnings (or all warnings!)

may help you get the build to succeed, but at the risk of endless debugging sessions. Turning

off failing tests and telling yourself you'll get them to pass later is as bad as pretending

your credit cards are free money.

G5: Duplication

This is one of the most important rules in this book, and you should take it very seriously.

Virtually every author who writes about software design mentions this rule. Dave Thomas

and Andy Hunt called it the DRY3 principle (Don't Repeat Yourself). Kent Beck made it

one of the core principles of Extreme Programming and called it: "Once, and only once."

Ron Jeffries ranks this rule second, just below getting all the tests to pass.

Every time you see duplication in the code, it represents a missed opportunity for

abstraction. That duplication could probably become a subroutine or perhaps another

class outright. By folding the duplication into such an abstraction, you increase the vocabulary

of the language of your design. Other programmers can use the abstract facilities

3. [PRAG].

290 Chapter 17: Smells and Heuristics

you create. Coding becomes faster and less error prone because you have raised the

abstraction level.

The most obvious form of duplication is when you have clumps of identical code that

look like some programmers went wild with the mouse, pasting the same code over and

over again. These should be replaced with simple methods.

A more subtle form is the switch/case or if/else chain that appears again and again in

various modules, always testing for the same set of conditions. These should be replaced

with polymorphism.

Still more subtle are the modules that have similar algorithms, but that don't share

similar lines of code. This is still duplication and should be addressed by using the TEMPLATE

METHOD,4 or STRATEGY5 pattern.

Indeed, most of the design patterns that have appeared in the last fifteen years are simply

well-known ways to eliminate duplication. So too the Codd Normal Forms are a strategy

for eliminating duplication in database schemae. OO itself is a strategy for organizing

modules and eliminating duplication. Not surprisingly, so is structured programming.

I think the point has been made. Find and eliminate duplication wherever you can.

G6: Code at Wrong Level of Abstraction

It is important to create abstractions that separate higher level general concepts from lower

level detailed concepts. Sometimes we do this by creating abstract classes to hold the

higher level concepts and derivatives to hold the lower level concepts. When we do this,

we need to make sure that the separation is complete. We want all the lower level concepts

to be in the derivatives and all the higher level concepts to be in the base class.

For example, constants, variables, or utility functions that pertain only to the detailed

implementation should not be present in the base class. The base class should know nothing

about them.

This rule also pertains to source files, components, and modules. Good software

design requires that we separate concepts at different levels and place them in different

containers. Sometimes these containers are base classes or derivatives and sometimes they

are source files, modules, or components. Whatever the case may be, the separation needs

to be complete. We don't want lower and higher level concepts mixed together.

Consider the following code:

public interface Stack {

Object pop() throws EmptyException;

void push(Object o) throws FullException;

double percentFull();

4. [GOF].

5. [GOF].

General 291

class EmptyException extends Exception {}

class FullException extends Exception {}

}

The percentFull function is at the wrong level of abstraction. Although there are

many implementations of Stack where the concept of fullness is reasonable, there are other

implementations that simply could not know how full they are. So the function would be

better placed in a derivative interface such as BoundedStack.

Perhaps you are thinking that the implementation could just return zero if the stack

were boundless. The problem with that is that no stack is truly boundless. You cannot

really prevent an OutOfMemoryException by checking for

stack.percentFull() < 50.0.

Implementing the function to return 0 would be telling a lie.

The point is that you cannot lie or fake your way out of a misplaced abstraction. Isolating

abstractions is one of the hardest things that software developers do, and there is no

quick fix when you get it wrong.

G7: Base Classes Depending on Their Derivatives

The most common reason for partitioning concepts into base and derivative classes is so

that the higher level base class concepts can be independent of the lower level derivative

class concepts. Therefore, when we see base classes mentioning the names of their derivatives,

we suspect a problem. In general, base classes should know nothing about their

derivatives.

There are exceptions to this rule, of course. Sometimes the number of derivatives is

strictly fixed, and the base class has code that selects between the derivatives. We see this a

lot in finite state machine implementations. However, in that case the derivatives and base

class are strongly coupled and always deploy together in the same jar file. In the general

case we want to be able to deploy derivatives and bases in different jar files.

Deploying derivatives and bases in different jar files and making sure the base jar files

know nothing about the contents of the derivative jar files allow us to deploy our systems

in discrete and independent components. When such components are modified, they can

be redeployed without having to redeploy the base components. This means that the

impact of a change is greatly lessened, and maintaining systems in the field is made much

simpler.

G8: Too Much Information

Well-defined modules have very small interfaces that allow you to do a lot with a little.

Poorly defined modules have wide and deep interfaces that force you to use many different

gestures to get simple things done. A well-defined interface does not offer very many functions

to depend upon, so coupling is low. A poorly defined interface provides lots of functions

that you must call, so coupling is high.

292 Chapter 17: Smells and Heuristics

Good software developers learn to limit what they expose at the interfaces of their

classes and modules. The fewer methods a class has, the better. The fewer variables a function

knows about, the better. The fewer instance variables a class has, the better.

Hide your data. Hide your utility functions. Hide your constants and your temporaries.

Don't create classes with lots of methods or lots of instance variables. Don't create lots of

protected variables and functions for your subclasses. Concentrate on keeping interfaces

very tight and very small. Help keep coupling low by limiting information.

G9: Dead Code

Dead code is code that isn't executed. You find it in the body of an if statement that checks

for a condition that can't happen. You find it in the catch block of a try that never throws.

You find it in little utility methods that are never called or switch/case conditions that

never occur.

The problem with dead code is that after awhile it starts to smell. The older it is, the

stronger and sourer the odor becomes. This is because dead code is not completely

updated when designs change. It still compiles, but it does not follow newer conventions or

rules. It was written at a time when the system was different. When you find dead code, do

the right thing. Give it a decent burial. Delete it from the system.

G10: Vertical Separation

Variables and function should be defined close to where they are used. Local variables

should be declared just above their first usage and should have a small vertical scope. We

don't want local variables declared hundreds of lines distant from their usages.

Private functions should be defined just below their first usage. Private functions

belong to the scope of the whole class, but we'd still like to limit the vertical distance

between the invocations and definitions. Finding a private function should just be a matter

of scanning downward from the first usage.

G11: Inconsistency

If you do something a certain way, do all similar things in the same way. This goes back

to the principle of least surprise. Be careful with the conventions you choose, and once

chosen, be careful to continue to follow them.

If within a particular function you use a variable named response to hold an

HttpServletResponse, then use the same variable name consistently in the other functions

that use HttpServletResponse objects. If you name a method processVerificationRequest,

then use a similar name, such as processDeletionRequest, for the methods that process

other kinds of requests.

Simple consistency like this, when reliably applied, can make code much easier to

read and modify.

General 293

G12: Clutter

Of what use is a default constructor with no implementation? All it serves to do is clutter

up the code with meaningless artifacts. Variables that aren't used, functions that are never

called, comments that add no information, and so forth. All these things are clutter and

should be removed. Keep your source files clean, well organized, and free of clutter.

G13: Artificial Coupling

Things that don't depend upon each other should not be artificially coupled. For example,

general enums should not be contained within more specific classes because this forces the

whole application to know about these more specific classes. The same goes for general

purpose static functions being declared in specific classes.

In general an artificial coupling is a coupling between two modules that serves no

direct purpose. It is a result of putting a variable, constant, or function in a temporarily

convenient, though inappropriate, location. This is lazy and careless.

Take the time to figure out where functions, constants, and variables ought to be

declared. Don't just toss them in the most convenient place at hand and then leave them

there.

G14: Feature Envy

This is one of Martin Fowler's code smells.6 The methods of a class should be interested in

the variables and functions of the class they belong to, and not the variables and functions

of other classes. When a method uses accessors and mutators of some other object to

manipulate the data within that object, then it envies the scope of the class of that other

object. It wishes that it were inside that other class so that it could have direct access to the

variables it is manipulating. For example:

public class HourlyPayCalculator {

public Money calculateWeeklyPay(HourlyEmployee e) {

int tenthRate = e.getTenthRate().getPennies();

int tenthsWorked = e.getTenthsWorked();

int straightTime = Math.min(400, tenthsWorked);

int overTime = Math.max(0, tenthsWorked - straightTime);

int straightPay = straightTime * tenthRate;

int overtimePay = (int)Math.round(overTime*tenthRate*1.5);

return new Money(straightPay + overtimePay);

}

}

The calculateWeeklyPay method reaches into the HourlyEmployee object to get the data on

which it operates. The calculateWeeklyPay method envies the scope of HourlyEmployee. It

"wishes" that it could be inside HourlyEmployee.

6. [Refactoring].

294 Chapter 17: Smells and Heuristics

All else being equal, we want to eliminate Feature Envy because it exposes the internals

of one class to another. Sometimes, however, Feature Envy is a necessary evil. Consider the

following:

public class HourlyEmployeeReport {

private HourlyEmployee employee ;

public HourlyEmployeeReport(HourlyEmployee e) {

this.employee = e;

}

String reportHours() {

return String.format(

"Name: %s\tHours:%d.%1d

",

employee.getName(),

employee.getTenthsWorked()/10,

employee.getTenthsWorked()%10);

}

}

Clearly, the reportHours method envies the HourlyEmployee class. On the other hand, we

don't want HourlyEmployee to have to know about the format of the report. Moving that format

string into the HourlyEmployee class would violate several principles of object oriented

design.7 It would couple HourlyEmployee to the format of the report, exposing it to changes

in that format.

G15: Selector Arguments

There is hardly anything more abominable than a dangling false argument at the end of a

function call. What does it mean? What would it change if it were true? Not only is the

purpose of a selector argument difficult to remember, each selector argument combines

many functions into one. Selector arguments are just a lazy way to avoid splitting a large

function into several smaller functions. Consider:

public int calculateWeeklyPay(boolean overtime) {

int tenthRate = getTenthRate();

int tenthsWorked = getTenthsWorked();

int straightTime = Math.min(400, tenthsWorked);

int overTime = Math.max(0, tenthsWorked - straightTime);

int straightPay = straightTime * tenthRate;

double overtimeRate = overtime ? 1.5 : 1.0 * tenthRate;

int overtimePay = (int)Math.round(overTime*overtimeRate);

return straightPay + overtimePay;

}

You call this function with a true if overtime is paid as time and a half, and with a

false if overtime is paid as straight time. It's bad enough that you must remember what

calculateWeeklyPay(false) means whenever you happen to stumble across it. But the

7. Specifically, the Single Responsibility Principle, the Open Closed Principle, and the Common Closure Principle. See [PPP].

General 295

real shame of a function like this is that the author missed the opportunity to write the

following:

public int straightPay() {

return getTenthsWorked() * getTenthRate();

}

public int overTimePay() {

int overTimeTenths = Math.max(0, getTenthsWorked() - 400);

int overTimePay = overTimeBonus(overTimeTenths);

return straightPay() + overTimePay;

}

private int overTimeBonus(int overTimeTenths) {

double bonus = 0.5 * getTenthRate() * overTimeTenths;

return (int) Math.round(bonus);

}

Of course, selectors need not be boolean. They can be enums, integers, or any other

type of argument that is used to select the behavior of the function. In general it is better to

have many functions than to pass some code into a function to select the behavior.

G16: Obscured Intent

We want code to be as expressive as possible. Run-on expressions, Hungarian notation,

and magic numbers all obscure the author's intent. For example, here is the overTimePay

function as it might have appeared:

public int m_otCalc() {

return iThsWkd * iThsRte +

(int) Math.round(0.5 * iThsRte *

Math.max(0, iThsWkd - 400)

);

}

Small and dense as this might appear, it's also virtually impenetrable. It is worth taking

the time to make the intent of our code visible to our readers.

G17: Misplaced Responsibility

One of the most important decisions a software developer can make is where to put code.

For example, where should the PI constant go? Should it be in the Math class? Perhaps it

belongs in the Trigonometry class? Or maybe in the Circle class?

The principle of least surprise comes into play here. Code should be placed where a

reader would naturally expect it to be. The PI constant should go where the trig functions

are declared. The OVERTIME_RATE constant should be declared in the HourlyPay-

Calculator class.

Sometimes we get "clever" about where to put certain functionality. We'll put it in a

function that's convenient for us, but not necessarily intuitive to the reader. For example,

perhaps we need to print a report with the total of hours that an employee worked. We

296 Chapter 17: Smells and Heuristics

could sum up those hours in the code that prints the report, or we could try to keep a running

total in the code that accepts time cards.

One way to make this decision is to look at the names of the functions. Let's say that

our report module has a function named getTotalHours. Let's also say that the module that

accepts time cards has a saveTimeCard function. Which of these two functions, by it's name,

implies that it calculates the total? The answer should be obvious.

Clearly, there are sometimes performance reasons why the total should be calculated

as time cards are accepted rather than when the report is printed. That's fine, but the names

of the functions ought to reflect this. For example, there should be a computeRunning-

TotalOfHours function in the timecard module.

G18: Inappropriate Static

Math.max(double a, double b) is a good static method. It does not operate on a single

instance; indeed, it would be silly to have to say new Math().max(a,b) or even a.max(b).

All the data that max uses comes from its two arguments, and not from any "owning"

object. More to the point, there is almost no chance that we'd want Math.max to be

polymorphic.

Sometimes, however, we write static functions that should not be static. For example,

consider:

HourlyPayCalculator.calculatePay(employee, overtimeRate).

Again, this seems like a reasonable static function. It doesn't operate on any particular

object and gets all it's data from it's arguments. However, there is a reasonable chance that

we'll want this function to be polymorphic. We may wish to implement several different

algorithms for calculating hourly pay, for example, OvertimeHourlyPayCalculator and

StraightTimeHourlyPayCalculator. So in this case the function should not be static. It

should be a nonstatic member function of Employee.

In general you should prefer nonstatic methods to static methods. When in doubt,

make the function nonstatic. If you really want a function to be static, make sure that there

is no chance that you'll want it to behave polymorphically.

G19: Use Explanatory Variables

Kent Beck wrote about this in his great book Smalltalk Best Practice Patterns8 and again

more recently in his equally great book Implementation Patterns.9 One of the more powerful

ways to make a program readable is to break the calculations up into intermediate values

that are held in variables with meaningful names.

8. [Beck97], p. 108.

9. [Beck07].

General 297

Consider this example from FitNesse:

Matcher match = headerPattern.matcher(line);

if(match.find())

{

String key = match.group(1);

String value = match.group(2);

headers.put(key.toLowerCase(), value);

}

The simple use of explanatory variables makes it clear that the first matched group is

the key, and the second matched group is the value.

It is hard to overdo this. More explanatory variables are generally better than fewer. It

is remarkable how an opaque module can suddenly become transparent simply by breaking

the calculations up into well-named intermediate values.

G20: Function Names Should Say What They Do

Look at this code:

Date newDate = date.add(5);

Would you expect this to add five days to the date? Or is it weeks, or hours? Is the date

instance changed or does the function just return a new Date without changing the old one?

You can't tell from the call what the function does.

If the function adds five days to the date and changes the date, then it should be called

addDaysTo or increaseByDays. If, on the other hand, the function returns a new date that is

five days later but does not change the date instance, it should be called daysLater or

daysSince.

If you have to look at the implementation (or documentation) of the function to know

what it does, then you should work to find a better name or rearrange the functionality so

that it can be placed in functions with better names.

G21: Understand the Algorithm

Lots of very funny code is written because people don't take the time to understand the

algorithm. They get something to work by plugging in enough if statements and flags,

without really stopping to consider what is really going on.

Programming is often an exploration. You think you know the right algorithm for

something, but then you wind up fiddling with it, prodding and poking at it, until you get it

to "work." How do you know it "works"? Because it passes the test cases you can think of.

There is nothing wrong with this approach. Indeed, often it is the only way to get a

function to do what you think it should. However, it is not sufficient to leave the quotation

marks around the word "work."

298 Chapter 17: Smells and Heuristics

Before you consider yourself to be done with a function, make sure you understand

how it works. It is not good enough that it passes all the tests. You must know10 that the

solution is correct.

Often the best way to gain this knowledge and understanding is to refactor the function

into something that is so clean and expressive that it is obvious how it works.

G22: Make Logical Dependencies Physical

If one module depends upon another, that dependency should be physical, not just logical.

The dependent module should not make assumptions (in other words, logical dependencies)

about the module it depends upon. Rather it should explicitly ask that module for all

the information it depends upon.

For example, imagine that you are writing a function that prints a plain text report of

hours worked by employees. One class named HourlyReporter gathers all the data into a

convenient form and then passes it to HourlyReportFormatter to print it. (See Listing 17-1.)

10. There is a difference between knowing how the code works and knowing whether the algorithm will do the job required of it.

Being unsure that an algorithm is appropriate is often a fact of life. Being unsure what your code does is just laziness.

Listing 17-1

HourlyReporter.java

public class HourlyReporter {

private HourlyReportFormatter formatter;

private List<LineItem> page;

private final int PAGE_SIZE = 55;

public HourlyReporter(HourlyReportFormatter formatter) {

this.formatter = formatter;

page = new ArrayList<LineItem>();

}

public void generateReport(List<HourlyEmployee> employees) {

for (HourlyEmployee e : employees) {

addLineItemToPage(e);

if (page.size() == PAGE_SIZE)

printAndClearItemList();

}

if (page.size() > 0)

printAndClearItemList();

}

private void printAndClearItemList() {

formatter.format(page);

page.clear();

}

private void addLineItemToPage(HourlyEmployee e) {

LineItem item = new LineItem();

item.name = e.getName();

item.hours = e.getTenthsWorked() / 10;

General 299

This code has a logical dependency that has not been physicalized. Can you spot it? It

is the constant PAGE_SIZE. Why should the HourlyReporter know the size of the page? Page

size should be the responsibility of the HourlyReportFormatter.

The fact that PAGE_SIZE is declared in HourlyReporter represents a misplaced

responsibility [G17] that causes HourlyReporter to assume that it knows what the page size

ought to be. Such an assumption is a logical dependency. HourlyReporter depends on the

fact that HourlyReportFormatter can deal with page sizes of 55. If some implementation of

HourlyReportFormatter could not deal with such sizes, then there would be an error.

We can physicalize this dependency by creating a new method in HourlyReport-

Formatter named getMaxPageSize(). HourlyReporter will then call that function rather than

using the PAGE_SIZE constant.

G23: Prefer Polymorphism to If/Else or Switch/Case

This might seem a strange suggestion given the topic of Chapter 6. After all, in that chapter I

make the point that switch statements are probably appropriate in the parts of the system

where adding new functions is more likely than adding new types.

First, most people use switch statements because it's the obvious brute force solution,

not because it's the right solution for the situation. So this heuristic is here to remind us to

consider polymorphism before using a switch.

Second, the cases where functions are more volatile than types are relatively rare. So

every switch statement should be suspect.

I use the following "ONE SWITCH" rule: There may be no more than one switch statement

for a given type of selection. The cases in that switch statement must create polymorphic

objects that take the place of other such switch statements in the rest of the system.

G24: Follow Standard Conventions

Every team should follow a coding standard based on common industry norms. This coding

standard should specify things like where to declare instance variables; how to name

classes, methods, and variables; where to put braces; and so on. The team should not need

a document to describe these conventions because their code provides the examples.

item.tenths = e.getTenthsWorked() % 10;

page.add(item);

}

public class LineItem {

public String name;

public int hours;

public int tenths;

}

}

Listing 17-1 (continued)

HourlyReporter.java

300 Chapter 17: Smells and Heuristics

Everyone on the team should follow these conventions. This means that each team

member must be mature enough to realize that it doesn't matter a whit where you put your

braces so long as you all agree on where to put them.

If you would like to know what conventions I follow, you'll see them in the refactored

code in Listing B-7 on page 394, through Listing B-14.

G25: Replace Magic Numbers with Named Constants

This is probably one of the oldest rules in software development. I remember reading it in the

late sixties in introductory COBOL, FORTRAN, and PL/1 manuals. In general it is a bad

idea to have raw numbers in your code. You should hide them behind well-named constants.

For example, the number 86,400 should be hidden behind the constant

SECONDS_PER_DAY. If you are printing 55 lines per page, then the constant 55 should be hidden

behind the constant LINES_PER_PAGE.

Some constants are so easy to recognize that they don't always need a named constant

to hide behind so long as they are used in conjunction with very self-explanatory code. For

example:

double milesWalked = feetWalked/5280.0;

int dailyPay = hourlyRate * 8;

double circumference = radius * Math.PI * 2;

Do we really need the constants FEET_PER_MILE, WORK_HOURS_PER_DAY, and TWO in the

above examples? Clearly, the last case is absurd. There are some formulae in which constants

are simply better written as raw numbers. You might quibble about the

WORK_HOURS_PER_DAY case because the laws or conventions might change. On the other

hand, that formula reads so nicely with the 8 in it that I would be reluctant to add 17 extra

characters to the readers' burden. And in the FEET_PER_MILE case, the number 5280 is so

very well known and so unique a constant that readers would recognize it even if it stood

alone on a page with no context surrounding it.

Constants like 3.141592653589793 are also very well known and easily recognizable.

However, the chance for error is too great to leave them raw. Every time someone sees

3.1415927535890793, they know that it is p, and so they fail to scrutinize it. (Did you

catch the single-digit error?) We also don't want people using 3.14, 3.14159, 3.142, and so

forth. Therefore, it is a good thing that Math.PI has already been defined for us.

The term "Magic Number" does not apply only to numbers. It applies to any token

that has a value that is not self-describing. For example:

assertEquals(7777, Employee.find("John Doe").employeeNumber());

There are two magic numbers in this assertion. The first is obviously 7777, though

what it might mean is not obvious. The second magic number is "John Doe," and again the

intent is not clear.

It turns out that "John Doe" is the name of employee #7777 in a well-known test database

created by our team. Everyone in the team knows that when you connect to this

General 301

database, it will have several employees already cooked into it with well-known values

and attributes. It also turns out that "John Doe" represents the sole hourly employee in

that test database. So this test should really read:

assertEquals(

HOURLY_EMPLOYEE_ID,

Employee.find(HOURLY_EMPLOYEE_NAME).employeeNumber());

G26: Be Precise

Expecting the first match to be the only match to a query is probably naive. Using floating

point numbers to represent currency is almost criminal. Avoiding locks and/or transaction

management because you don't think concurrent update is likely is lazy at best. Declaring

a variable to be an ArrayList when a List will due is overly constraining. Making all variables

protected by default is not constraining enough.

When you make a decision in your code, make sure you make it precisely. Know why

you have made it and how you will deal with any exceptions. Don't be lazy about the precision

of your decisions. If you decide to call a function that might return null, make sure

you check for null. If you query for what you think is the only record in the database,

make sure your code checks to be sure there aren't others. If you need to deal with currency,

use integers11 and deal with rounding appropriately. If there is the possibility of

concurrent update, make sure you implement some kind of locking mechanism.

Ambiguities and imprecision in code are either a result of disagreements or laziness.

In either case they should be eliminated.

G27: Structure over Convention

Enforce design decisions with structure over convention. Naming conventions are good,

but they are inferior to structures that force compliance. For example, switch/cases with

nicely named enumerations are inferior to base classes with abstract methods. No one is

forced to implement the switch/case statement the same way each time; but the base

classes do enforce that concrete classes have all abstract methods implemented.

G28: Encapsulate Conditionals

Boolean logic is hard enough to understand without having to see it in the context of an if

or while statement. Extract functions that explain the intent of the conditional.

For example:

if (shouldBeDeleted(timer))

is preferable to

if (timer.hasExpired() && !timer.isRecurrent())

11. Or better yet, a Money class that uses integers.

302 Chapter 17: Smells and Heuristics

G29: Avoid Negative Conditionals

Negatives are just a bit harder to understand than positives. So, when possible, conditionals

should be expressed as positives. For example:

if (buffer.shouldCompact())

is preferable to

if (!buffer.shouldNotCompact())

G30: Functions Should Do One Thing

It is often tempting to create functions that have multiple sections that perform a series of

operations. Functions of this kind do more than one thing, and should be converted into

many smaller functions, each of which does one thing.

For example:

public void pay() {

for (Employee e : employees) {

if (e.isPayday()) {

Money pay = e.calculatePay();

e.deliverPay(pay);

}

}

}

This bit of code does three things. It loops over all the employees, checks to see whether

each employee ought to be paid, and then pays the employee. This code would be better

written as:

public void pay() {

for (Employee e : employees)

payIfNecessary(e);

}

private void payIfNecessary(Employee e) {

if (e.isPayday())

calculateAndDeliverPay(e);

}

private void calculateAndDeliverPay(Employee e) {

Money pay = e.calculatePay();

e.deliverPay(pay);

}

Each of these functions does one thing. (See "Do One Thing" on page 35.)

G31: Hidden Temporal Couplings

Temporal couplings are often necessary, but you should not hide the coupling. Structure

the arguments of your functions such that the order in which they should be called is obvious.

Consider the following:

General 303

public class MoogDiver {

Gradient gradient;

List<Spline> splines;

public void dive(String reason) {

saturateGradient();

reticulateSplines();

diveForMoog(reason);

}

...

}

The order of the three functions is important. You must saturate the gradient before you

can reticulate the splines, and only then can you dive for the moog. Unfortunately, the code

does not enforce this temporal coupling. Another programmer could call reticulate-

Splines before saturateGradient was called, leading to an UnsaturatedGradientException.

A better solution is:

public class MoogDiver {

Gradient gradient;

List<Spline> splines;

public void dive(String reason) {

Gradient gradient = saturateGradient();

List<Spline> splines = reticulateSplines(gradient);

diveForMoog(splines, reason);

}

...

}

This exposes the temporal coupling by creating a bucket brigade. Each function produces a

result that the next function needs, so there is no reasonable way to call them out of order.

You might complain that this increases the complexity of the functions, and you'd be

right. But that extra syntactic complexity exposes the true temporal complexity of the situation.

Note that I left the instance variables in place. I presume that they are needed by private

methods in the class. Even so, I want the arguments in place to make the temporal

coupling explicit.

G32: Don't Be Arbitrary

Have a reason for the way you structure your code, and make sure that reason is communicated

by the structure of the code. If a structure appears arbitrary, others will feel empowered

to change it. If a structure appears consistently throughout the system, others will use it

and preserve the convention. For example, I was recently merging changes to FitNesse and

discovered that one of our committers had done this:

public class AliasLinkWidget extends ParentWidget

{

public static class VariableExpandingWidgetRoot {

...

...

}

304 Chapter 17: Smells and Heuristics

The problem with this was that VariableExpandingWidgetRoot had no need to be

inside the scope of AliasLinkWidget. Moreover, other unrelated classes made use of

AliasLinkWidget.VariableExpandingWidgetRoot. These classes had no need to know

about AliasLinkWidget.

Perhaps the programmer had plopped the VariableExpandingWidgetRoot into

AliasWidget as a matter of convenience, or perhaps he thought it really needed to be

scoped inside AliasWidget. Whatever the reason, the result wound up being arbitrary. Public

classes that are not utilities of some other class should not be scoped inside another

class. The convention is to make them public at the top level of their package.

G33: Encapsulate Boundary Conditions

Boundary conditions are hard to keep track of. Put the processing for them in one place.

Don't let them leak all over the code. We don't want swarms of +1s and -1s scattered hither

and yon. Consider this simple example from FIT:

if(level + 1 < tags.length)

{

parts = new Parse(body, tags, level + 1, offset + endTag);

body = null;

}

Notice that level+1 appears twice. This is a boundary condition that should be encapsulated

within a variable named something like nextLevel.

int nextLevel = level + 1;

if(nextLevel < tags.length)

{

parts = new Parse(body, tags, nextLevel, offset + endTag);

body = null;

}

G34: Functions Should Descend Only One Level of Abstraction

The statements within a function should all be written at the same level of abstraction,

which should be one level below the operation described by the name of the function. This

may be the hardest of these heuristics to interpret and follow. Though the idea is plain

enough, humans are just far too good at seamlessly mixing levels of abstraction. Consider,

for example, the following code taken from FitNesse:

public String render() throws Exception

{

StringBuffer html = new StringBuffer("<hr");

if(size > 0)

html.append(" size=\"").append(size + 1).append("\"");

html.append(">");

return html.toString();

}

General 305

A moment's study and you can see what's going on. This function constructs the HTML

tag that draws a horizontal rule across the page. The height of that rule is specified in the

size variable.

Now look again. This method is mixing at least two levels of abstraction. The first is

the notion that a horizontal rule has a size. The second is the syntax of the HR tag itself.

This code comes from the HruleWidget module in FitNesse. This module detects a row of

four or more dashes and converts it into the appropriate HR tag. The more dashes, the

larger the size.

I refactored this bit of code as follows. Note that I changed the name of the size field

to reflect its true purpose. It held the number of extra dashes.

public String render() throws Exception

{

HtmlTag hr = new HtmlTag("hr");

if (extraDashes > 0)

hr.addAttribute("size", hrSize(extraDashes));

return hr.html();

}

private String hrSize(int height)

{

int hrSize = height + 1;

return String.format("%d", hrSize);

}

This change separates the two levels of abstraction nicely. The render function simply constructs

an HR tag, without having to know anything about the HTML syntax of that tag.

The HtmlTag module takes care of all the nasty syntax issues.

Indeed, by making this change I caught a subtle error. The original code did not put

the closing slash on the HR tag, as the XHTML standard would have it. (In other words, it

emitted <hr> instead of <hr/>.) The HtmlTag module had been changed to conform to

XHTML long ago.

Separating levels of abstraction is one of the most important functions of refactoring,

and it's one of the hardest to do well. As an example, look at the code below. This

was my first attempt at separating the abstraction levels in the HruleWidget.render

method.

public String render() throws Exception

{

HtmlTag hr = new HtmlTag("hr");

if (size > 0) {

hr.addAttribute("size", ""+(size+1));

}

return hr.html();

}

My goal, at this point, was to create the necessary separation and get the tests to pass.

I accomplished that goal easily, but the result was a function that still had mixed levels

of abstraction. In this case the mixed levels were the construction of the HR tag and the

306 Chapter 17: Smells and Heuristics

interpretation and formatting of the size variable. This points out that when you break a

function along lines of abstraction, you often uncover new lines of abstraction that were

obscured by the previous structure.

G35: Keep Configurable Data at High Levels

If you have a constant such as a default or configuration value that is known and expected

at a high level of abstraction, do not bury it in a low-level function. Expose it as an argument

to that low-level function called from the high-level function. Consider the following

code from FitNesse:

public static void main(String[] args) throws Exception

{

Arguments arguments = parseCommandLine(args);

...

}

public class Arguments

{

public static final String DEFAULT_PATH = ".";

public static final String DEFAULT_ROOT = "FitNesseRoot";

public static final int DEFAULT_PORT = 80;

public static final int DEFAULT_VERSION_DAYS = 14;

...

}

The command-line arguments are parsed in the very first executable line of FitNesse. The

default values of those arguments are specified at the top of the Argument class. You don't

have to go looking in low levels of the system for statements like this one:

if (arguments.port == 0) // use 80 by default

The configuration constants reside at a very high level and are easy to change. They get

passed down to the rest of the application. The lower levels of the application do not own

the values of these constants.

G36: Avoid Transitive Navigation

In general we don't want a single module to know much about its collaborators. More specifically,

if A collaborates with B, and B collaborates with C, we don't want modules that use

A to know about C. (For example, we don't want a.getB().getC().doSomething();.)

This is sometimes called the Law of Demeter. The Pragmatic Programmers call it

"Writing Shy Code."12 In either case it comes down to making sure that modules know

only about their immediate collaborators and do not know the navigation map of the whole

system.

If many modules used some form of the statement a.getB().getC(), then it would be

difficult to change the design and architecture to interpose a Q between B and C. You'd

12. [PRAG], p. 138.

Java 307

have to find every instance of a.getB().getC() and convert it to a.getB().getQ().getC().

This is how architectures become rigid. Too many modules know too much about the

architecture.

Rather we want our immediate collaborators to offer all the services we need. We

should not have to roam through the object graph of the system, hunting for the method we

want to call. Rather we should simply be able to say:

myCollaborator.doSomething().

Java

J1: Avoid Long Import Lists by Using Wildcards

If you use two or more classes from a package, then import the whole package with

import package.*;

Long lists of imports are daunting to the reader. We don't want to clutter up the tops of our

modules with 80 lines of imports. Rather we want the imports to be a concise statement

about which packages we collaborate with.

Specific imports are hard dependencies, whereas wildcard imports are not. If you specifically

import a class, then that class must exist. But if you import a package with a wildcard,

no particular classes need to exist. The import statement simply adds the package to

the search path when hunting for names. So no true dependency is created by such

imports, and they therefore serve to keep our modules less coupled.

There are times when the long list of specific imports can be useful. For example, if

you are dealing with legacy code and you want to find out what classes you need to build

mocks and stubs for, you can walk down the list of specific imports to find out the true

qualified names of all those classes and then put the appropriate stubs in place. However,

this use for specific imports is very rare. Furthermore, most modern IDEs will allow you

to convert the wildcarded imports to a list of specific imports with a single command. So

even in the legacy case it's better to import wildcards.

Wildcard imports can sometimes cause name conflicts and ambiguities. Two classes

with the same name, but in different packages, will need to be specifically imported, or at

least specifically qualified when used. This can be a nuisance but is rare enough that using

wildcard imports is still generally better than specific imports.

J2: Don't Inherit Constants

I have seen this several times and it always makes me grimace. A programmer puts some

constants in an interface and then gains access to those constants by inheriting that interface.

Take a look at the following code:

public class HourlyEmployee extends Employee {

private int tenthsWorked;

private double hourlyRate;

308 Chapter 17: Smells and Heuristics

public Money calculatePay() {

int straightTime = Math.min(tenthsWorked, TENTHS_PER_WEEK);

int overTime = tenthsWorked - straightTime;

return new Money(

hourlyRate * (tenthsWorked + OVERTIME_RATE * overTime)

);

}

...

}

Where did the constants TENTHS_PER_WEEK and OVERTIME_RATE come from? They might have

come from class Employee; so let's take a look at that:

public abstract class Employee implements PayrollConstants {

public abstract boolean isPayday();

public abstract Money calculatePay();

public abstract void deliverPay(Money pay);

}

Nope, not there. But then where? Look closely at class Employee. It implements

PayrollConstants.

public interface PayrollConstants {

public static final int TENTHS_PER_WEEK = 400;

public static final double OVERTIME_RATE = 1.5;

}

This is a hideous practice! The constants are hidden at the top of the inheritance hierarchy.

Ick! Don't use inheritance as a way to cheat the scoping rules of the language. Use a static

import instead.

import static PayrollConstants.*;

public class HourlyEmployee extends Employee {

private int tenthsWorked;

private double hourlyRate;

public Money calculatePay() {

int straightTime = Math.min(tenthsWorked, TENTHS_PER_WEEK);

int overTime = tenthsWorked - straightTime;

return new Money(

hourlyRate * (tenthsWorked + OVERTIME_RATE * overTime)

);

}

...

}

J3: Constants versus Enums

Now that enums have been added to the language (Java 5), use them! Don't keep using the

old trick of public static final ints. The meaning of ints can get lost. The meaning of

enums cannot, because they belong to an enumeration that is named.

What's more, study the syntax for enums carefully. They can have methods and fields.

This makes them very powerful tools that allow much more expression and flexibility than

ints. Consider this variation on the payroll code:

Names 309

public class HourlyEmployee extends Employee {

private int tenthsWorked;

HourlyPayGrade grade;

public Money calculatePay() {

int straightTime = Math.min(tenthsWorked, TENTHS_PER_WEEK);

int overTime = tenthsWorked - straightTime;

return new Money(

grade.rate() * (tenthsWorked + OVERTIME_RATE * overTime)

);

}

...

}

public enum HourlyPayGrade {

APPRENTICE {

public double rate() {

return 1.0;

}

},

LEUTENANT_JOURNEYMAN {

public double rate() {

return 1.2;

}

},

JOURNEYMAN {

public double rate() {

return 1.5;

}

},

MASTER {

public double rate() {

return 2.0;

}

};

public abstract double rate();

}

Names

N1: Choose Descriptive Names

Don't be too quick to choose a name. Make sure the name is descriptive. Remember that

meanings tend to drift as software evolves, so frequently reevaluate the appropriateness of

the names you choose.

This is not just a "feel-good" recommendation. Names in software are 90 percent of

what make software readable. You need to take the time to choose them wisely and keep

them relevant. Names are too important to treat carelessly.

Consider the code below. What does it do? If I show you the code with well-chosen

names, it will make perfect sense to you, but like this it's just a hodge-podge of symbols

and magic numbers.

310 Chapter 17: Smells and Heuristics

public int x() {

int q = 0;

int z = 0;

for (int kk = 0; kk < 10; kk++) {

if (l[z] == 10)

{

q += 10 + (l[z + 1] + l[z + 2]);

z += 1;

}

else if (l[z] + l[z + 1] == 10)

{

q += 10 + l[z + 2];

z += 2;

} else {

q += l[z] + l[z + 1];

z += 2;

}

}

return q;

}

Here is the code the way it should be written. This snippet is actually less complete

than the one above. Yet you can infer immediately what it is trying to do, and you could

very likely write the missing functions based on that inferred meaning. The magic numbers

are no longer magic, and the structure of the algorithm is compellingly descriptive.

public int score() {

int score = 0;

int frame = 0;

for (int frameNumber = 0; frameNumber < 10; frameNumber++) {

if (isStrike(frame)) {

score += 10 + nextTwoBallsForStrike(frame);

frame += 1;

} else if (isSpare(frame)) {

score += 10 + nextBallForSpare(frame);

frame += 2;

} else {

score += twoBallsInFrame(frame);

frame += 2;

}

}

return score;

}

The power of carefully chosen names is that they overload the structure of the code

with description. That overloading sets the readers' expectations about what the other

functions in the module do. You can infer the implementation of isStrike() by looking at

the code above. When you read the isStrike method, it will be "pretty much what you

expected."13

private boolean isStrike(int frame) {

return rolls[frame] == 10;

}

13. See Ward Cunningham's quote on page 11.

Names 311

N2: Choose Names at the Appropriate Level of Abstraction

Don't pick names that communicate implementation; choose names the reflect the level of

abstraction of the class or function you are working in. This is hard to do. Again, people

are just too good at mixing levels of abstractions. Each time you make a pass over your

code, you will likely find some variable that is named at too low a level. You should take

the opportunity to change those names when you find them. Making code readable

requires a dedication to continuous improvement. Consider the Modem interface below:

public interface Modem {

boolean dial(String phoneNumber);

boolean disconnect();

boolean send(char c);

char recv();

String getConnectedPhoneNumber();

}

At first this looks fine. The functions all seem appropriate. Indeed, for many applications

they are. But now consider an application in which some modems aren't connected by

dialling. Rather they are connected permanently by hard wiring them together (think of the

cable modems that provide Internet access to most homes nowadays). Perhaps some are

connected by sending a port number to a switch over a USB connection. Clearly the notion

of phone numbers is at the wrong level of abstraction. A better naming strategy for this

scenario might be:

public interface Modem {

boolean connect(String connectionLocator);

boolean disconnect();

boolean send(char c);

char recv();

String getConnectedLocator();

}

Now the names don't make any commitments about phone numbers. They can still be used

for phone numbers, or they could be used for any other kind of connection strategy.

N3: Use Standard Nomenclature Where Possible

Names are easier to understand if they are based on existing convention or usage. For example,

if you are using the DECORATOR pattern, you should use the word Decorator in the names

of the decorating classes. For example, AutoHangupModemDecorator might be the name of a

class that decorates a Modem with the ability to automatically hang up at the end of a session.

Patterns are just one kind of standard. In Java, for example, functions that convert

objects to string representations are often named toString. It is better to follow conventions

like these than to invent your own.

Teams will often invent their own standard system of names for a particular project.

Eric Evans refers to this as a ubiquitous language for the project.14 Your code should use

14. [DDD].

312 Chapter 17: Smells and Heuristics

the terms from this language extensively. In short, the more you can use names that are

overloaded with special meanings that are relevant to your project, the easier it will be for

readers to know what your code is talking about.

N4: Unambiguous Names

Choose names that make the workings of a function or variable unambiguous. Consider

this example from FitNesse:

private String doRename() throws Exception

{

if(refactorReferences)

renameReferences();

renamePage();

pathToRename.removeNameFromEnd();

pathToRename.addNameToEnd(newName);

return PathParser.render(pathToRename);

}

The name of this function does not say what the function does except in broad and vague

terms. This is emphasized by the fact that there is a function named renamePage inside the

function named doRename! What do the names tell you about the difference between the

two functions? Nothing.

A better name for that function is renamePageAndOptionallyAllReferences. This may

seem long, and it is, but it's only called from one place in the module, so it's explanatory

value outweighs the length.

N5: Use Long Names for Long Scopes

The length of a name should be related to the length of the scope. You can use very short

variable names for tiny scopes, but for big scopes you should use longer names.

Variable names like i and j are just fine if their scope is five lines long. Consider this

snippet from the old standard "Bowling Game":

private void rollMany(int n, int pins)

{

for (int i=0; i<n; i++)

g.roll(pins);

}

This is perfectly clear and would be obfuscated if the variable i were replaced with something

annoying like rollCount. On the other hand, variables and functions with short names

lose their meaning over long distances. So the longer the scope of the name, the longer and

more precise the name should be.

N6: Avoid Encodings

Names should not be encoded with type or scope information. Prefixes such as m_ or f

are useless in today's environments. Also project and/or subsystem encodings such as

Tests 313

vis_ (for visual imaging system) are distracting and redundant. Again, today's environments

provide all that information without having to mangle the names. Keep your

names free of Hungarian pollution.

N7: Names Should Describe Side-Effects

Names should describe everything that a function, variable, or class is or does. Don't hide

side effects with a name. Don't use a simple verb to describe a function that does more

than just that simple action. For example, consider this code from TestNG:

public ObjectOutputStream getOos() throws IOException {

if (m_oos == null) {

m_oos = new ObjectOutputStream(m_socket.getOutputStream());

}

return m_oos;

}

This function does a bit more than get an "oos"; it creates the "oos" if it hasn't been created

already. Thus, a better name might be createOrReturnOos.

Tests

T1: Insufficient Tests

How many tests should be in a test suite? Unfortunately, the metric many programmers use

is "That seems like enough." A test suite should test everything that could possibly break.

The tests are insufficient so long as there are conditions that have not been explored by the

tests or calculations that have not been validated.

T2: Use a Coverage Tool!

Coverage tools reports gaps in your testing strategy. They make it easy to find modules,

classes, and functions that are insufficiently tested. Most IDEs give you a visual indication,

marking lines that are covered in green and those that are uncovered in red. This makes it

quick and easy to find if or catch statements whose bodies haven't been checked.

T3: Don't Skip Trivial Tests

They are easy to write and their documentary value is higher than the cost to produce

them.

T4: An Ignored Test Is a Question about an Ambiguity

Sometimes we are uncertain about a behavioral detail because the requirements are

unclear. We can express our question about the requirements as a test that is commented

out, or as a test that annotated with @Ignore. Which you choose depends upon whether the

ambiguity is about something that would compile or not.

314 Chapter 17: Smells and Heuristics

T5: Test Boundary Conditions

Take special care to test boundary conditions. We often get the middle of an algorithm

right but misjudge the boundaries.

T6: Exhaustively Test Near Bugs

Bugs tend to congregate. When you find a bug in a function, it is wise to do an exhaustive

test of that function. You'll probably find that the bug was not alone.

T7: Patterns of Failure Are Revealing

Sometimes you can diagnose a problem by finding patterns in the way the test cases fail.

This is another argument for making the test cases as complete as possible. Complete test

cases, ordered in a reasonable way, expose patterns.

As a simple example, suppose you noticed that all tests with an input larger than five

characters failed? Or what if any test that passed a negative number into the second argument

of a function failed? Sometimes just seeing the pattern of red and green on the test

report is enough to spark the "Aha!" that leads to the solution. Look back at page 267 to

see an interesting example of this in the SerialDate example.

T8: Test Coverage Patterns Can Be Revealing

Looking at the code that is or is not executed by the passing tests gives clues to why the

failing tests fail.

T9: Tests Should Be Fast

A slow test is a test that won't get run. When things get tight, it's the slow tests that will be

dropped from the suite. So do what you must to keep your tests fast.

Conclusion

This list of heuristics and smells could hardly be said to be complete. Indeed, I'm not sure

that such a list can ever be complete. But perhaps completeness should not be the goal,

because what this list does do is imply a value system.

Indeed, that value system has been the goal, and the topic, of this book. Clean code is

not written by following a set of rules. You don't become a software craftsman by learning

a list of heuristics. Professionalism and craftsmanship come from values that drive

disciplines.

Bibliography 315

Bibliography

[Refactoring]: Refactoring: Improving the Design of Existing Code, Martin Fowler et al.,

Addison-Wesley, 1999.

[PRAG]: The Pragmatic Programmer, Andrew Hunt, Dave Thomas, Addison-Wesley,

2000.

[GOF]: Design Patterns: Elements of Reusable Object Oriented Software, Gamma et al.,

Addison-Wesley, 1996.

[Beck97]: Smalltalk Best Practice Patterns, Kent Beck, Prentice Hall, 1997.

[Beck07]: Implementation Patterns, Kent Beck, Addison-Wesley, 2008.

[PPP]: Agile Software Development: Principles, Patterns, and Practices, Robert C. Martin,

Prentice Hall, 2002.

[DDD]: Domain Driven Design, Eric Evans, Addison-Wesley, 2003.

This page intentionally left blank

317

Appendix A

Concurrency II

by Brett L. Schuchert

This appendix supports and amplifies the Concurrency chapter on page 177. It is written

as a series of independent topics and you can generally read them in any order. There is

some duplication between sections to allow for such reading.

Client/Server Example

Imagine a simple client/server application. A server sits and waits listening on a socket for

a client to connect. A client connects and sends a request.

The Server

Here is a simplified version of a server application. Full source for this example is available

starting on page 343, Client/Server Nonthreaded.

ServerSocket serverSocket = new ServerSocket(8009);

while (keepProcessing) {

try {

Socket socket = serverSocket.accept();

process(socket);

} catch (Exception e) {

handle(e);

}

}

318 Appendix A: Concurrency II

This simple application waits for a connection, processes an incoming message, and then

again waits for the next client request to come in. Here's client code that connects to this

server:

private void connectSendReceive(int i) {

try {

Socket socket = new Socket("localhost", PORT);

MessageUtils.sendMessage(socket, Integer.toString(i));

MessageUtils.getMessage(socket);

socket.close();

} catch (Exception e) {

e.printStackTrace();

}

}

How well does this client/server pair perform? How can we formally describe that performance?

Here's a test that asserts that the performance is "acceptable":

@Test(timeout = 10000)

public void shouldRunInUnder10Seconds() throws Exception {

Thread[] threads = createThreads();

startAllThreadsw(threads);

waitForAllThreadsToFinish(threads);

}

The setup is left out to keep the example simple (see "ClientTest.java" on page 344). This

test asserts that it should complete within 10,000 milliseconds.

This is a classic example of validating the throughput of a system. This system should

complete a series of client requests in ten seconds. So long as the server can process each

individual client request in time, the test will pass.

What happens if the test fails? Short of developing some kind of event polling loop,

there is not much to do within a single thread that will make this code any faster. Will

using multiple threads solve the problem? It might, but we need to know where the time is

being spent. There are two possibilities:

• I/O—using a socket, connecting to a database, waiting for virtual memory swapping,

and so on.

• Processor—numerical calculations, regular expression processing, garbage collection,

and so on.

Systems typically have some of each, but for a given operation one tends to dominate. If

the code is processor bound, more processing hardware can improve throughput, making

our test pass. But there are only so many CPU cycles available, so adding threads to a

processor-bound problem will not make it go faster.

On the other hand, if the process is I/O bound, then concurrency can increase efficiency.

When one part of the system is waiting for I/O, another part can use that wait time

to process something else, making more effective use of the available CPU.

Client/Server Example 319

Adding Threading

Assume for the moment that the performance test fails. How can we improve the throughput

so that the performance test passes? If the process method of the server is I/O bound,

then here is one way to make the server use threads (just change the processMessage):

void process(final Socket socket) {

if (socket == null)

return;

Runnable clientHandler = new Runnable() {

public void run() {

try {

String message = MessageUtils.getMessage(socket);

MessageUtils.sendMessage(socket, "Processed: " + message);

closeIgnoringException(socket);

} catch (Exception e) {

e.printStackTrace();

}

}

};

Thread clientConnection = new Thread(clientHandler);

clientConnection.start();

}

Assume that this change causes the test to pass;1 the code is complete, correct?

Server Observations

The updated server completes the test successfully in just over one second. Unfortunately,

this solution is a bit naive and introduces some new problems.

How many threads might our server create? The code sets no limit, so the we could

feasibly hit the limit imposed by the Java Virtual Machine (JVM). For many simple systems

this may suffice. But what if the system is meant to support many users on the public

net? If too many users connect at the same time, the system might grind to a halt.

But set the behavioral problem aside for the moment. The solution shown has problems

of cleanliness and structure. How many responsibilities does the server code have?

• Socket connection management

• Client processing

• Threading policy

• Server shutdown policy

Unfortunately, all these responsibilities live in the process function. In addition, the

code crosses many different levels of abstraction. So, small as the process function is, it

needs to be repartitioned.

1. You can verify that for yourself by trying out the before and after code. Review the nonthreaded code starting on page 343.

Review the threaded code starting on page 346.

320 Appendix A: Concurrency II

The server has several reasons to change; therefore it violates the Single Responsibility

Principle. To keep concurrent systems clean, thread management should be kept to a few,

well-controlled places. What's more, any code that manages threads should do nothing

other than thread management. Why? If for no other reason than that tracking down concurrency

issues is hard enough without having to unwind other nonconcurrency issues at

the same time.

If we create a separate class for each of the responsibilities listed above, including the

thread management responsibility, then when we change the thread management strategy,

the change will impact less overall code and will not pollute the other responsibilities. This

also makes it much easier to test all the other responsibilities without having to worry

about threading. Here is an updated version that does just that:

public void run() {

while (keepProcessing) {

try {

ClientConnection clientConnection = connectionManager.awaitClient();

ClientRequestProcessor requestProcessor

= new ClientRequestProcessor(clientConnection);

clientScheduler.schedule(requestProcessor);

} catch (Exception e) {

e.printStackTrace();

}

}

connectionManager.shutdown();

}

This now focuses all things thread-related into one place, clientScheduler. If there are

concurrency problems, there is just one place to look:

public interface ClientScheduler {

void schedule(ClientRequestProcessor requestProcessor);

}

The current policy is easy to implement:

public class ThreadPerRequestScheduler implements ClientScheduler {

public void schedule(final ClientRequestProcessor requestProcessor) {

Runnable runnable = new Runnable() {

public void run() {

requestProcessor.process();

}

};

Thread thread = new Thread(runnable);

thread.start();

}

}

Having isolated all the thread management into a single place, it is much easier to change

the way we control threads. For example, moving to the Java 5 Executor framework

involves writing a new class and plugging it in (Listing A-1).

Possible Paths of Execution 321

Conclusion

Introducing concurrency in this particular example demonstrates a way to improve the

throughput of a system and one way of validating that throughput through a testing framework.

Focusing all concurrency code into a small number of classes is an example of

applying the Single Responsibility Principle. In the case of concurrent programming, this

becomes especially important because of its complexity.

Possible Paths of Execution

Review the method incrementValue, a one-line Java method with no looping or branching:

public class IdGenerator {

int lastIdUsed;

public int incrementValue() {

return ++lastIdUsed;

}

}

Ignore integer overflow and assume that only one thread has access to a single instance

of IdGenerator. In this case there is a single path of execution and a single guaranteed

result:

• The value returned is equal to the value of lastIdUsed, both of which are one greater

than just before calling the method.

Listing A-1

ExecutorClientScheduler.java

import java.util.concurrent.Executor;

import java.util.concurrent.Executors;

public class ExecutorClientScheduler implements ClientScheduler {

Executor executor;

public ExecutorClientScheduler(int availableThreads) {

executor = Executors.newFixedThreadPool(availableThreads);

}

public void schedule(final ClientRequestProcessor requestProcessor) {

Runnable runnable = new Runnable() {

public void run() {

requestProcessor.process();

}

};

executor.execute(runnable);

}

}

322 Appendix A: Concurrency II

What happens if we use two threads and leave the method unchanged? What are the

possible outcomes if each thread calls incrementValue once? How many possible paths of

execution are there? First, the outcomes (assume lastIdUsed starts with a value of 93):

• Thread 1 gets the value of 94, thread 2 gets the value of 95, and lastIdUsed is now 95.

• Thread 1 gets the value of 95, thread 2 gets the value of 94, and lastIdUsed is now 95.

• Thread 1 gets the value of 94, thread 2 gets the value of 94, and lastIdUsed is now 94.

The final result, while surprising, is possible. To see how these different results are possible,

we need to understand the number of possible paths of execution and how the Java

Virtual Machine executes them.

Number of Paths

To calculate the number of possible execution paths, we'll start with the generated bytecode.

The one line of java (return ++lastIdUsed;) becomes eight byte-code instructions. It

is possible for the two threads to interleave the execution of these eight instructions the

way a card dealer interleaves cards as he shuffles a deck.2 Even with only eight cards in

each hand, there are a remarkable number of shuffled outcomes.

For this simple case of N instructions in a sequence, no looping or conditionals, and T

threads, the total number of possible execution paths is equal to

2. This is a bit of a simplification. However, for the purpose of this discussion, we can use this simplifying model.

Calculating the Possible Orderings

This comes from an email from Uncle Bob to Brett:

With N steps and T threads there are T *N total steps. Prior to each step

there is a context switch that chooses between the T threads. Each path can

thus be represented as a string of digits denoting the context switches.

Given steps A and B and threads 1 and 2, the six possible paths are 1122,

1212, 1221, 2112, 2121, and 2211. Or, in terms of steps it is A1B1A2B2,

A1A2B1B2, A1A2B2B1, A2A1B1B2, A2A1B2B1, and A2B2A1B1. For

three threads the sequence is 112233, 112323, 113223, 113232, 112233,

121233, 121323, 121332, 123132, 123123, . . . .

One characteristic of these strings is that there must always be N

instances of each T. So the string 111111 is invalid because it has six

instances of 1 and zero instances of 2 and 3.

(NT)!

N!T

--------------

Possible Paths of Execution 323

For our simple case of one line of Java code, which equates to eight lines of byte-code

and two threads, the total number of possible paths of execution is 12,870. If the type of

lastIdUsed is a long, then every read/write becomes two operations instead of one, and the

number of possible orderings becomes 2,704,156.

What happens if we make one change to this method?

public synchronized void incrementValue() {

++lastIdUsed;

}

The number of possible execution pathways becomes two for two threads and N! in the

general case.

Digging Deeper

What about the surprising result that two threads could both call the method once (before

we added synchronized) and get the same numeric result? How is that possible? First

things first.

What is an atomic operation? We can define an atomic operation as any operation that

is uninterruptable. For example, in the following code, line 5, where 0 is assigned to

lastid, is atomic because according to the Java Memory model, assignment to a 32-bit

value is uninterruptable.

So we want the permutations of N 1's, N 2's, . . . and N T's. This is

really just the permutations of N * T things taken N * T at a time, which is

(N * T )!, but with all the duplicates removed. So the trick is to count the

duplicates and subtract that from (N * T )!.

Given two steps and two threads, how many duplicates are there? Each

four-digit string has two 1s and two 2s. Each of those pairs could be

swapped without changing the sense of the string. You could swap the 1s or

the 2s both, or neither. So there are four isomorphs for each string, which

means that there are three duplicates. So three out of four of the options are

duplicates; alternatively one of four of the permutations are NOT duplicates.

4! * .25 = 6. So this reasoning seems to work.

How many duplicates are there? In the case where N = 2 and T = 2, I

could swap the 1s, the 2s, or both. In the case where N = 2 and T = 3, I

could swap the 1s, the 2s, the 3s, 1s and 2s, 1s and 3s, or 2s and 3s. Swapping

is just the permutations of N. Let's say there are P permutations of N.

The number of different ways to arrange those permutations are P**T.

So the number of possible isomorphs is N!**T. And so the number of

paths is (T*N)!/(N!**T). Again, in our T = 2, N = 2 case we get 6 (24/4).

For N = 2 and T = 3 we get 720/8 = 90.

For N = 3 and T = 3 we get 9!/6^3 = 1680.

Calculating the Possible Orderings (continued)

324 Appendix A: Concurrency II

01: public class Example {

02: int lastId;

03:

04: public void resetId() {

05: value = 0;

06: }

07:

08: public int getNextId() {

09: ++value;

10: }

11:}

What happens if we change type of lastId from int to long? Is line 5 still atomic?

Not according to the JVM specification. It could be atomic on a particular processor,

but according to the JVM specification, assignment to any 64-bit value requires two

32-bit assignments. This means that between the first 32-bit assignment and the second

32-bit assignment, some other thread could sneak in and change one of the values.

What about the pre-increment operator, ++, on line 9? The pre-increment operator can

be interrupted, so it is not atomic. To understand, let's review the byte-code of both of these

methods in detail.

Before we go any further, here are three definitions that will be important:

• Frame—Every method invocation requires a frame. The frame includes the return

address, any parameters passed into the method and the local variables defined in the

method. This is a standard technique used to define a call stack, which is used by

modern languages to allow for basic function/method invocation and to allow for

recursive invocation.

• Local variable—Any variables defined in the scope of the method. All nonstatic methods

have at least one variable, this, which represents the current object, the object

that received the most recent message (in the current thread), which caused the

method invocation.

• Operand stack—Many of the instructions in the Java Virtual Machine take parameters.

The operand stack is where those parameters are put. The stack is a standard

last-in, first-out (LIFO) data structure.

Here is the byte-code generated for resetId():

Mnemonic Description Operand

Stack After

ALOAD 0 Load the 0th variable onto the operand stack.

What is the 0th variable? It is this., the current

object. When the method was called, the

receiver of the message, an instance of Example,

was pushed into the local variable array of the

frame created for method invocation. This is

always the first variable put in every instance

method.

this

Possible Paths of Execution 325

These three instructions are guaranteed to be atomic because, although the thread

executing them could be interrupted after any one of them, the information for the

PUTFIELD instruction (the constant value 0 on the top of the stack and the reference to

this one below the top, along with the field value) cannot be touched by another thread.

So when the assignment occurs, we are guaranteed that the value 0 will be stored in the

field value. The operation is atomic. The operands all deal with information local to the

method, so there is no interference between multiple threads.

So if these three instructions are executed by ten threads, there are 4.38679733629e+24

possible orderings. However, there is only one possible outcome, so the different orderings

are irrelevant. It just so happens that the same outcome is guaranteed for longs in this case

as well. Why? All ten threads are assigning a constant value. Even if they interleave with

each other, the end result is the same.

With the ++ operation in the getNextId method, there are going to be problems.

Assume that lastId holds 42 at the beginning of this method. Here is the byte-code for this

new method:

ICONST_0 Put the constant value 0 onto the operand stack. this, 0

PUTFIELD lastId Store the top value on the stack (which is 0) into

the field value of the object referred to by the

object reference one away from the top of the

stack, this.

<empty>

Mnemonic Description Operand

Stack After

ALOAD 0 Load this onto the operand stack this

DUP Copy the top of the stack. We now have two

copies of this on the operand stack.

this, this

GETFIELD lastId Retrieve the value of the field lastId from the

object pointed to on the top of the stack (this) and

store that value back on to the stack.

this, 42

ICONST_1 Push the integer constant 1 on the stack. this, 42, 1

IADD Integer add the top two values on the operand

stack and store the result back on to the operand

stack.

this, 43

DUP_X1 Duplicate the value 43 and put it before this. 43, this, 43

PUTFIELD value Store the top value on the operand stack, 43, into

the field value of the current object, represented by

the next-to-top value on the operand stack, this.

43

IRETURN return the top (and only) value on the stack. <empty>

Mnemonic Description Operand

Stack After

326 Appendix A: Concurrency II

Imagine the case where the first thread completes the first three instructions, up to and

including GETFIELD, and then it is interrupted. A second thread takes over and performs

the entire method, incrementing lastId by one; it gets 43 back. Then the first thread picks up

where it left off; 42 is still on the operand stack because that was the value of lastId when it

executed GETFIELD. It adds one to get 43 again and stores the result. The value 43 is

returned to the first thread as well. The result is that one of the increments is lost because the

first thread stepped on the second thread after the second thread interrupted the first thread.

Making the getNexId() method synchronized fixes this problem.

Conclusion

An intimate understanding of byte-code is not necessary to understand how threads can

step on each other. If you can understand this one example, it should demonstrate the possibility

of multiple threads stepping on each other, which is enough knowledge.

That being said, what this trivial example demonstrates is a need to understand the

memory model enough to know what is and is not safe. It is a common misconception that

the ++ (pre- or post-increment) operator is atomic, and it clearly is not. This means you

need to know:

• Where there are shared objects/values

• The code that can cause concurrent read/update issues

• How to guard such concurrent issues from happening

Knowing Your Library

Executor Framework

As demonstrated in the ExecutorClientScheduler.java on page 321, the Executor framework

introduced in Java 5 allows for sophisticated execution using thread pools. This is a

class in the java.util.concurrent package.

If you are creating threads and are not using a thread pool or are using a hand-written

one, you should consider using the Executor. It will make your code cleaner, easier to follow,

and smaller.

The Executor framework will pool threads, resize automatically, and recreate threads

if necessary. It also supports futures, a common concurrent programming construct. The

Executor framework works with classes that implement Runnable and also works with

classes that implement the Callable interface. A Callable looks like a Runnable, but it can

return a result, which is a common need in multithreaded solutions.

A future is handy when code needs to execute multiple, independent operations and

wait for both to finish:

public String processRequest(String message) throws Exception {

Callable<String> makeExternalCall = new Callable<String>() {

Knowing Your Library 327

public String call() throws Exception {

String result = "";

// make external request

return result;

}

};

Future<String> result = executorService.submit(makeExternalCall);

String partialResult = doSomeLocalProcessing();

return result.get() + partialResult;

}

In this example, the method starts executing the makeExternalCall object. The method continues

other processing. The final line calls result.get(), which blocks until the future

completes.

Nonblocking Solutions

The Java 5 VM takes advantage of modern processor design, which supports reliable,

nonblocking updates. Consider, for example, a class that uses synchronization (and therefore

blocking) to provide a thread-safe update of a value:

public class ObjectWithValue {

private int value;

public void synchronized incrementValue() { ++value; }

public int getValue() { return value; }

}

Java 5 has a series of new classes for situations like this: AtomicBoolean,

AtomicInteger, and AtomicReference are three examples; there are several more. We can

rewrite the above code to use a nonblocking approach as follows:

public class ObjectWithValue {

private AtomicInteger value = new AtomicInteger(0);

public void incrementValue() {

value.incrementAndGet();

}

public int getValue() {

return value.get();

}

}

Even though this uses an object instead of a primitive and sends messages like

incrementAndGet() instead of ++, the performance of this class will nearly always beat the

previous version. In some cases it will only be slightly faster, but the cases where it will be

slower are virtually nonexistent.

How is this possible? Modern processors have an operation typically called Compare

and Swap (CAS). This operation is analogous to optimistic locking in databases, whereas

the synchronized version is analogous to pessimistic locking.

328 Appendix A: Concurrency II

The synchronized keyword always acquires a lock, even when a second thread is not

trying to update the same value. Even though the performance of intrinsic locks has

improved from version to version, they are still costly.

The nonblocking version starts with the assumption that multiple threads generally do

not modify the same value often enough that a problem will arise. Instead, it efficiently

detects whether such a situation has occurred and retries until the update happens successfully.

This detection is almost always less costly than acquiring a lock, even in moderate to

high contention situations.

How does the Virtual Machine accomplish this? The CAS operation is atomic. Logically,

the CAS operation looks something like the following:

int variableBeingSet;

void simulateNonBlockingSet(int newValue) {

int currentValue;

do {

currentValue = variableBeingSet

} while(currentValue != compareAndSwap(currentValue, newValue));

}

int synchronized compareAndSwap(int currentValue, int newValue) {

if(variableBeingSet == currentValue) {

variableBeingSet = newValue;

return currentValue;

}

return variableBeingSet;

}

When a method attempts to update a shared variable, the CAS operation verifies that

the variable getting set still has the last known value. If so, then the variable is changed. If

not, then the variable is not set because another thread managed to get in the way. The

method making the attempt (using the CAS operation) sees that the change was not made

and retries.

Nonthread-Safe Classes

There are some classes that are inherently not thread safe. Here are a few examples:

• SimpleDateFormat

• Database Connections

• Containers in java.util

• Servlets

Note that some collection classes have individual methods that are thread-safe. However,

any operation that involves calling more than one method is not. For example, if you do

not want to replace something in a HashTable because it is already there, you might write

the following code:

if(!hashTable.containsKey(someKey)) {

hashTable.put(someKey, new SomeValue());

}

Dependencies Between Methods Can Break Concurrent Code 329

Each individual method is thread-safe. However, another thread might add a value in

between the containsKey and put calls. There are several options to fix this problem.

• Lock the HashTable first, and make sure all other users of the HashTable do the same—

client-based locking:

synchronized(map) {

if(!map.conainsKey(key))

map.put(key,value);

}

• Wrap the HashTable in its own object and use a different API—server-based locking

using an ADAPTER:

public class WrappedHashtable<K, V> {

private Map<K, V> map = new Hashtable<K, V>();

public synchronized void putIfAbsent(K key, V value) {

if (map.containsKey(key))

map.put(key, value);

}

}

• Use the thread-safe collections:

ConcurrentHashMap<Integer, String> map = new ConcurrentHashMap<Integer,

String>();

map.putIfAbsent(key, value);

The collections in java.util.concurrent have operations like putIfAbsent() to accommodate

such operations.

Dependencies Between Methods

Can Break Concurrent Code

Here is a trivial example of a way to introduce dependencies between methods:

public class IntegerIterator implements Iterator<Integer>

private Integer nextValue = 0;

public synchronized boolean hasNext() {

return nextValue < 100000;

}

public synchronized Integer next() {

if (nextValue == 100000)

throw new IteratorPastEndException();

return nextValue++;

}

public synchronized Integer getNextValue() {

return nextValue;

}

}

Here is some code to use this IntegerIterator:

IntegerIterator iterator = new IntegerIterator();

while(iterator.hasNext()) {

330 Appendix A: Concurrency II

int nextValue = iterator.next();

// do something with nextValue

}

If one thread executes this code, there will be no problem. But what happens if two threads

attempt to share a single instance of IngeterIterator with the intent that each thread will

process the values it gets, but that each element of the list is processed only once? Most of

the time, nothing bad happens; the threads happily share the list, processing the elements

they are given by the iterator and stopping when the iterator is complete. However, there is

a small chance that, at the end of the iteration, the two threads will interfere with each

other and cause one thread to go beyond the end of the iterator and throw an exception.

Here's the problem: Thread 1 asks the question hasNext(), which returns true. Thread

1 gets preempted and then Thread 2 asks the same question, which is still true. Thread 2

then calls next(), which returns a value as expected but has a side effect of making

hasNext() return false. Thread 1 starts up again, thinking hasNext() is still true, and then

calls next(). Even though the individual methods are synchronized, the client uses two

methods.

This is a real problem and an example of the kinds of problems that crop up in concurrent

code. In this particular situation this problem is especially subtle because the only

time where this causes a fault is when it happens during the final iteration of the iterator.

If the threads happen to break just right, then one of the threads could go beyond the end

of the iterator. This is the kind of bug that happens long after a system has been in production,

and it is hard to track down.

You have three options:

• Tolerate the failure.

• Solve the problem by changing the client: client-based locking

• Solve the problem by changing the server, which additionally changes the client:

server-based locking

Tolerate the Failure

Sometimes you can set things up such that the failure causes no harm. For example, the

above client could catch the exception and clean up. Frankly, this is a bit sloppy. It's rather

like cleaning up memory leaks by rebooting at midnight.

Client-Based Locking

To make IntegerIterator work correctly with multiple threads, change this client (and

every other client) as follows:

IntegerIterator iterator = new IntegerIterator();

while (true) {

int nextValue;

Dependencies Between Methods Can Break Concurrent Code 331

synchronized (iterator) {

if (!iterator.hasNext())

break;

nextValue = iterator.next();

}

doSometingWith(nextValue);

}

Each client introduces a lock via the synchronized keyword. This duplication violates the

DRY principle, but it might be necessary if the code uses non-thread-safe third-party tools.

This strategy is risky because all programmers who use the server must remember to

lock it before using it and unlock it when done. Many (many!) years ago I worked on a

system that employed client-based locking on a shared resource. The resource was used in

hundreds of different places throughout the code. One poor programmer forgot to lock the

resource in one of those places.

The system was a multi-terminal time-sharing system running accounting software

for Local 705 of the trucker's union. The computer was in a raised-floor, environmentcontrolled

room 50 miles north of the Local 705 headquarters. At the headquarters they

had dozens of data entry clerks typing union dues postings into the terminals. The terminals

were connected to the computer using dedicated phone lines and 600bps half-duplex

modems. (This was a very, very long time ago.)

About once per day, one of the terminals would "lock up." There was no rhyme or reason

to it. The lock up showed no preference for particular terminals or particular times. It

was as though there were someone rolling dice choosing the time and terminal to lock up.

Sometimes more than one terminal would lock up. Sometimes days would go by without

any lock-ups.

At first the only solution was a reboot. But reboots were tough to coordinate. We had

to call the headquarters and get everyone to finish what they were doing on all the terminals.

Then we could shut down and restart. If someone was doing something important

that took an hour or two, the locked up terminal simply had to stay locked up.

After a few weeks of debugging we found that the cause was a ring-buffer counter that

had gotten out of sync with its pointer. This buffer controlled output to the terminal. The

pointer value indicated that the buffer was empty, but the counter said it was full. Because

it was empty, there was nothing to display; but because it was also full, nothing could be

added to the buffer to be displayed on the screen.

So we knew why the terminals were locking, but we didn't know why the ring buffer

was getting out of sync. So we added a hack to work around the problem. It was possible to

read the front panel switches on the computer. (This was a very, very, very long time ago.)

We wrote a little trap function that detected when one of these switches was thrown and

then looked for a ring buffer that was both empty and full. If one was found, it reset that

buffer to empty. Voila! The locked-up terminal(s) started displaying again.

So now we didn't have to reboot the system when a terminal locked up. The Local

would simply call us and tell us we had a lock-up, and then we just walked into the computer

room and flicked a switch.

332 Appendix A: Concurrency II

Of course sometimes they worked on the weekends, and we didn't. So we added a

function to the scheduler that checked all the ring buffers once per minute and reset any

that were both empty and full. This caused the displays to unclog before the Local could

even get on the phone.

It was several more weeks of poring over page after page of monolithic assembly language

code before we found the culprit. We had done the math and calculated that the frequency

of the lock-ups was consistent with a single unprotected use of the ring buffer. So

all we had to do was find that one faulty usage. Unfortunately, this was so very long ago

that we didn't have search tools or cross references or any other kind of automated help.

We simply had to pore over listings.

I learned an important lesson that cold Chicago winter of 1971. Client-based locking

really blows.

Server-Based Locking

The duplication can be removed by making the following changes to IntegerIterator:

public class IntegerIteratorServerLocked {

private Integer nextValue = 0;

public synchronized Integer getNextOrNull() {

if (nextValue < 100000)

return nextValue++;

else

return null;

}

}

And the client code changes as well:

while (true) {

Integer nextValue = iterator.getNextOrNull();

if (next == null)

break;

// do something with nextValue

}

In this case we actually change the API of our class to be multithread aware.3 The client

needs to perform a null check instead of checking hasNext().

In general you should prefer server-based locking for these reasons:

• It reduces repeated code—Client-based locking forces each client to lock the server

properly. By putting the locking code into the server, clients are free to use the object

and not worry about writing additional locking code.

3. In fact, the Iterator interface is inherently not thread-safe. It was never designed to be used by multiple threads, so this

should come as no surprise.

Increasing Throughput 333

• It allows for better performance—You can swap out a thread-safe server for a nonthread

safe one in the case of single-threaded deployment, thereby avoiding all

overhead.

• It reduces the possibility of error—All it takes is for one programmer to forget to lock

properly.

• It enforces a single policy—The policy is in one place, the server, rather than many

places, each client.

• It reduces the scope of the shared variables—The client is not aware of them or how

they are locked. All of that is hidden in the server. When things break, the number of

places to look is smaller.

What if you do not own the server code?

• Use an ADAPTER to change the API and add locking

public class ThreadSafeIntegerIterator {

private IntegerIterator iterator = new IntegerIterator();

public synchronized Integer getNextOrNull() {

if(iterator.hasNext())

return iterator.next();

return null;

}

}

• OR better yet, use the thread-safe collections with extended interfaces

Increasing Throughput

Let's assume that we want to go out on the net and read the contents of a set of pages from

a list of URLs. As each page is read, we will parse it to accumulate some statistics. Once

all the pages are read, we will print a summary report.

The following class returns the contents of one page, given a URL.

public class PageReader {

//...

public String getPageFor(String url) {

HttpMethod method = new GetMethod(url);

try {

httpClient.executeMethod(method);

String response = method.getResponseBodyAsString();

return response;

} catch (Exception e) {

handle(e);

} finally {

method.releaseConnection();

}

}

}

334 Appendix A: Concurrency II

The next class is the iterator that provides the contents of the pages based on an iterator of

URLs:

public class PageIterator {

private PageReader reader;

private URLIterator urls;

public PageIterator(PageReader reader, URLIterator urls) {

this.urls = urls;

this.reader = reader;

}

public synchronized String getNextPageOrNull() {

if (urls.hasNext())

getPageFor(urls.next());

else

return null;

}

public String getPageFor(String url) {

return reader.getPageFor(url);

}

}

An instance of the PageIterator can be shared between many different threads, each

one using it's own instance of the PageReader to read and parse the pages it gets from the

iterator.

Notice that we've kept the synchronized block very small. It contains just the critical

section deep inside the PageIterator. It is always better to synchronize as little as possible

as opposed to synchronizing as much as possible.

Single-Thread Calculation of Throughput

Now lets do some simple calculations. For the purpose of argument, assume the following:

• I/O time to retrieve a page (average): 1 second

• Processing time to parse page (average): .5 seconds

• I/O requires 0 percent of the CPU while processing requires 100 percent.

For N pages being processed by a single thread, the total execution time is 1.5 seconds

* N. Figure A-1 shows a snapshot of 13 pages or about 19.5 seconds.

Figure A-1

Single thread

Deadlock 335

Multithread Calculation of Throughput

If it is possible to retrieve pages in any order and process the pages independently, then it

is possible to use multiple threads to increase throughput. What happens if we use three

threads? How many pages can we acquire in the same time?

As you can see in Figure A-2, the multithreaded solution allows the process-bound

parsing of the pages to overlap with the I/O-bound reading of the pages. In an idealized

world this means that the processor is fully utilized. Each one-second page read is overlapped

with two parses. Thus, we can process two pages per second, which is three times

the throughput of the single-threaded solution.

Deadlock

Imagine a Web application with two shared resource pools of some finite size:

• A pool of database connections for local work in process storage

• A pool of MQ connections to a master repository

Assume there are two operations in this application, create and update:

• Create—Acquire connection to master repository and database. Talk to service master

repository and then store work in local work in process database.

Figure A-2

Three concurrent threads

336 Appendix A: Concurrency II

• Update—Acquire connection to database and then master repository. Read from work

in process database and then send to the master repository

What happens when there are more users than the pool sizes? Consider each pool has

a size of ten.

• Ten users attempt to use create, so all ten database connections are acquired, and each

thread is interrupted after acquiring a database connection but before acquiring a connection

to the master repository.

• Ten users attempt to use update, so all ten master repository connections are acquired,

and each thread is interrupted after acquiring the master repository but before acquiring

a database connection.

• Now the ten "create" threads must wait to acquire a master repository connection, but

the ten "update" threads must wait to acquire a database connection.

• Deadlock. The system never recovers.

This might sound like an unlikely situation, but who wants a system that freezes solid

every other week? Who wants to debug a system with symptoms that are so difficult to

reproduce? This is the kind of problem that happens in the field, then takes weeks to solve.

A typical "solution" is to introduce debugging statements to find out what is happening.

Of course, the debug statements change the code enough so that the deadlock happens

in a different situation and takes months to again occur.4

To really solve the problem of deadlock, we need to understand what causes it. There

are four conditions required for deadlock to occur:

• Mutual exclusion

• Lock & wait

• No preemption

• Circular wait

Mutual Exclusion

Mutual exclusion occurs when multiple threads need to use the same resources and those

resources

• Cannot be used by multiple threads at the same time.

• Are limited in number.

A common example of such a resource is a database connection, a file open for write, a

record lock, or a semaphore.

4. For example, someone adds some debugging output and the problem "disappears." The debugging code "fixes" the problem

so it remains in the system.

Deadlock 337

Lock & Wait

Once a thread acquires a resource, it will not release the resource until it has acquired all

of the other resources it requires and has completed its work.

No Preemption

One thread cannot take resources away from another thread. Once a thread holds a

resource, the only way for another thread to get it is for the holding thread to release it.

Circular Wait

This is also referred to as the deadly embrace. Imagine two threads, T1 and T2, and two

resources, R1 and R2. T1 has R1, T2 has R2. T1 also requires R2, and T2 also requires R1.

This gives something like Figure A-3:

All four of these conditions must hold for deadlock to be possible. Break any one of these

conditions and deadlock is not possible.

Breaking Mutual Exclusion

One strategy for avoiding deadlock is to sidestep the mutual exclusion condition. You

might be able to do this by

• Using resources that allow simultaneous use, for example, AtomicInteger.

• Increasing the number of resources such that it equals or exceeds the number of competing

threads.

• Checking that all your resources are free before seizing any.

Unfortunately, most resources are limited in number and don't allow simultaneous

use. And it's not uncommon for the identity of the second resource to be predicated on the

results of operating on the first. But don't be discouraged; there are three conditions left.

Figure A-3

338 Appendix A: Concurrency II

Breaking Lock & Wait

You can also eliminate deadlock if you refuse to wait. Check each resource before you

seize it, and release all resources and start over if you run into one that's busy.

This approach introduces several potential problems:

• Starvation—One thread keeps being unable to acquire the resources it needs (maybe it

has a unique combination of resources that seldom all become available).

• Livelock—Several threads might get into lockstep and all acquire one resource and

then release one resource, over and over again. This is especially likely with simplistic

CPU scheduling algorithms (think embedded devices or simplistic hand-written

thread balancing algorithms).

Both of these can cause poor throughput. The first results in low CPU utilization,

whereas the second results in high and useless CPU utilization.

As inefficient as this strategy sounds, it's better than nothing. It has the benefit that it

can almost always be implemented if all else fails.

Breaking Preemption

Another strategy for avoiding deadlock is to allow threads to take resources away from

other threads. This is usually done through a simple request mechanism. When a thread

discovers that a resource is busy, it asks the owner to release it. If the owner is also waiting

for some other resource, it releases them all and starts over.

This is similar to the previous approach but has the benefit that a thread is allowed to

wait for a resource. This decreases the number of startovers. Be warned, however, that

managing all those requests can be tricky.

Breaking Circular Wait

This is the most common approach to preventing deadlock. For most systems it requires

no more than a simple convention agreed to by all parties.

In the example above with Thread 1 wanting both Resource 1 and Resource 2 and

Thread 2 wanting both Resource 2 and then Resource 1, simply forcing both Thread 1 and

Thread 2 to allocate resources in the same order makes circular wait impossible.

More generally, if all threads can agree on a global ordering of resources and if they

all allocate resources in that order, then deadlock is impossible. Like all the other strategies,

this can cause problems:

• The order of acquisition might not correspond to the order of use; thus a resource

acquired at the start might not be used until the end. This can cause resources to be

locked longer than strictly necessary.

Testing Multithreaded Code 339

• Sometimes you cannot impose an order on the acquisition of resources. If the ID of

the second resource comes from an operation performed on the first, then ordering is

not feasible.

So there are many ways to avoid deadlock. Some lead to starvation, whereas others

make heavy use of the CPU and reduce responsiveness. TANSTAAFL!5

Isolating the thread-related part of your solution to allow for tuning and experimentation

is a powerful way to gain the insights needed to determine the best strategies.

Testing Multithreaded Code

How can we write a test to demonstrate the following code is broken?

01: public class ClassWithThreadingProblem {

02: int nextId;

03:

04: public int takeNextId() {

05: return nextId++;

06: }

07:}

Here's a description of a test that will prove the code is broken:

• Remember the current value of nextId.

• Create two threads, both of which call takeNextId() once.

• Verify that nextId is two more than what we started with.

• Run this until we demonstrate that nextId was only incremented by one instead

of two.

Listing A-2 shows such a test:

5. There ain't no such thing as a free lunch.

Listing A-2

ClassWithThreadingProblemTest.java

01: package example;

02:

03: import static org.junit.Assert.fail;

04:

05: import org.junit.Test;

06:

07: public class ClassWithThreadingProblemTest {

08: @Test

09: public void twoThreadsShouldFailEventually() throws Exception {

10: final ClassWithThreadingProblem classWithThreadingProblem

= new ClassWithThreadingProblem();

11:

340 Appendix A: Concurrency II

12: Runnable runnable = new Runnable() {

13: public void run() {

14: classWithThreadingProblem.takeNextId();

15: }

16: };

17:

18: for (int i = 0; i < 50000; ++i) {

19: int startingId = classWithThreadingProblem.lastId;

20: int expectedResult = 2 + startingId;

21:

22: Thread t1 = new Thread(runnable);

23: Thread t2 = new Thread(runnable);

24: t1.start();

25: t2.start();

26: t1.join();

27: t2.join();

28:

29: int endingId = classWithThreadingProblem.lastId;

30:

31: if (endingId != expectedResult)

32: return;

33: }

34:

35: fail("Should have exposed a threading issue but it did not.");

36: }

37: }

Line Description

10 Create a single instance of ClassWithThreadingProblem. Note, we must use

the final keyword because we use it below in an anonymous inner class.

12–16 Create an anonymous inner class that uses the single instance of

ClassWithThreadingProblem.

18 Run this code "enough" times to demonstrate that the code failed, but not

so much that the test "takes too long." This is a balancing act; we don't

want to wait too long to demonstrate failure. Picking this number is hard—

although later we'll see that we can greatly reduce this number.

19 Remember the starting value. This test is trying to prove that the code in

ClassWithThreadingProblem is broken. If this test passes, it proved that the

code was broken. If this test fails, the test was unable to prove that the code

is broken.

20 We expect the final value to be two more than the current value.

22–23 Create two threads, both of which use the object we created in lines 12–16.

This gives us the potential of two threads trying to use our single instance

of ClassWithThreadingProblem and interfering with each other.

Listing A-2 (continued)

ClassWithThreadingProblemTest.java

Testing Multithreaded Code 341

This test certainly sets up the conditions for a concurrent update problem. However,

the problem occurs so infrequently that the vast majority of times this test won't detect it.

Indeed, to truly detect the problem we need to set the number of iterations to over one

million. Even then, in ten executions with a loop count of 1,000,000, the problem occurred

only once. That means we probably ought to set the iteration count to well over one hundred

million to get reliable failures. How long are we prepared to wait?

Even if we tuned the test to get reliable failures on one machine, we'll probably have

to retune the test with different values to demonstrate the failure on another machine,

operating system, or version of the JVM.

And this is a simple problem. If we cannot demonstrate broken code easily with this

problem, how will we ever detect truly complex problems?

So what approaches can we take to demonstrate this simple failure? And, more importantly,

how can we write tests that will demonstrate failures in more complex code? How

will we be able to discover if our code has failures when we do not know where to look?

Here are a few ideas:

• Monte Carlo Testing. Make tests flexible, so they can be tuned. Then run the test over

and over—say on a test server—randomly changing the tuning values. If the tests ever

fail, the code is broken. Make sure to start writing those tests early so a continuous

integration server starts running them soon. By the way, make sure you carefully log

the conditions under which the test failed.

• Run the test on every one of the target deployment platforms. Repeatedly. Continuously.

The longer the tests run without failure, the more likely that

– The production code is correct or

– The tests aren't adequate to expose problems.

• Run the tests on a machine with varying loads. If you can simulate loads close to a

production environment, do so.

24–25 Make our two threads eligible to run.

26–27 Wait for both threads to finish before we check the results.

29 Record the actual final value.

31–32 Did our endingId differ from what we expected? If so, return end the test—

we've proven that the code is broken. If not, try again.

35 If we got to here, our test was unable to prove the production code was broken

in a "reasonable" amount of time; our code has failed. Either the code

is not broken or we didn't run enough iterations to get the failure condition

to occur.

Line Description

342 Appendix A: Concurrency II

Yet, even if you do all of these things, you still don't stand a very good chance of finding

threading problems with your code. The most insidious problems are the ones that

have such a small cross section that they only occur once in a billion opportunities. Such

problems are the terror of complex systems.

Tool Support for Testing Thread-Based Code

IBM has created a tool called ConTest.6 It instruments classes to make it more likely that

non-thread-safe code fails.

We do not have any direct relationship with IBM or the team that developed ConTest.

A colleague of ours pointed us to it. We noticed vast improvement in our ability to find

threading issues after a few minutes of using it.

Here's an outline of how to use ConTest:

• Write tests and production code, making sure there are tests specifically designed to

simulate multiple users under varying loads, as mentioned above.

• Instrument test and production code with ConTest.

• Run the tests.

When we instrumented code with ConTest, our success rate went from roughly one failure

in ten million iterations to roughly one failure in thirty iterations. Here are the loop values

for several runs of the test after instrumentation: 13, 23, 0, 54, 16, 14, 6, 69, 107, 49, 2. So

clearly the instrumented classes failed much earlier and with much greater reliability.

Conclusion

This chapter has been a very brief sojourn through the large and treacherous territory of

concurrent programming. We barely scratched the surface. Our emphasis here was on disciplines

to help keep concurrent code clean, but there is much more you should learn if

you are going to be writing concurrent systems. We recommend you start with Doug Lea's

wonderful book Concurrent Programming in Java: Design Principles and Patterns.7

In this chapter we talked about concurrent update, and the disciplines of clean synchronization

and locking that can prevent it. We talked about how threads can enhance the

throughput of an I/O-bound system and showed the clean techniques for achieving such

improvements. We talked about deadlock and the disciplines for preventing it in a clean

6. http://www.haifa.ibm.com/projects/verification/contest/index.html

7. See [Lea99] p. 191.

Tutorial: Full Code Examples 343

way. Finally, we talked about strategies for exposing concurrent problems by instrumenting

your code.

Tutorial: Full Code Examples

Client/Server Nonthreaded

Listing A-3

Server.java

package com.objectmentor.clientserver.nonthreaded;

import java.io.IOException;

import java.net.ServerSocket;

import java.net.Socket;

import java.net.SocketException;

import common.MessageUtils;

public class Server implements Runnable {

ServerSocket serverSocket;

volatile boolean keepProcessing = true;

public Server(int port, int millisecondsTimeout) throws IOException {

serverSocket = new ServerSocket(port);

serverSocket.setSoTimeout(millisecondsTimeout);

}

public void run() {

System.out.printf("Server Starting

");

while (keepProcessing) {

try {

System.out.printf("accepting client

");

Socket socket = serverSocket.accept();

System.out.printf("got client

");

process(socket);

} catch (Exception e) {

handle(e);

}

}

}

private void handle(Exception e) {

if (!(e instanceof SocketException)) {

e.printStackTrace();

}

}

public void stopProcessing() {

keepProcessing = false;

closeIgnoringException(serverSocket);

}

344 Appendix A: Concurrency II

void process(Socket socket) {

if (socket == null)

return;

try {

System.out.printf("Server: getting message

");

String message = MessageUtils.getMessage(socket);

System.out.printf("Server: got message: %s

", message);

Thread.sleep(1000);

System.out.printf("Server: sending reply: %s

", message);

MessageUtils.sendMessage(socket, "Processed: " + message);

System.out.printf("Server: sent

");

closeIgnoringException(socket);

} catch (Exception e) {

e.printStackTrace();

}

}

private void closeIgnoringException(Socket socket) {

if (socket != null)

try {

socket.close();

} catch (IOException ignore) {

}

}

private void closeIgnoringException(ServerSocket serverSocket) {

if (serverSocket != null)

try {

serverSocket.close();

} catch (IOException ignore) {

}

}

}

Listing A-4

ClientTest.java

package com.objectmentor.clientserver.nonthreaded;

import java.io.IOException;

import java.net.ServerSocket;

import java.net.Socket;

import java.net.SocketException;

import common.MessageUtils;

public class Server implements Runnable {

ServerSocket serverSocket;

volatile boolean keepProcessing = true;

Listing A-3 (continued)

Server.java

Tutorial: Full Code Examples 345

public Server(int port, int millisecondsTimeout) throws IOException {

serverSocket = new ServerSocket(port);

serverSocket.setSoTimeout(millisecondsTimeout);

}

public void run() {

System.out.printf("Server Starting

");

while (keepProcessing) {

try {

System.out.printf("accepting client

");

Socket socket = serverSocket.accept();

System.out.printf("got client

");

process(socket);

} catch (Exception e) {

handle(e);

}

}

}

private void handle(Exception e) {

if (!(e instanceof SocketException)) {

e.printStackTrace();

}

}

public void stopProcessing() {

keepProcessing = false;

closeIgnoringException(serverSocket);

}

void process(Socket socket) {

if (socket == null)

return;

try {

System.out.printf("Server: getting message

");

String message = MessageUtils.getMessage(socket);

System.out.printf("Server: got message: %s

", message);

Thread.sleep(1000);

System.out.printf("Server: sending reply: %s

", message);

MessageUtils.sendMessage(socket, "Processed: " + message);

System.out.printf("Server: sent

");

closeIgnoringException(socket);

} catch (Exception e) {

e.printStackTrace();

}

}

private void closeIgnoringException(Socket socket) {

if (socket != null)

try {

socket.close();

Listing A-4 (continued)

ClientTest.java

346 Appendix A: Concurrency II

Client/Server Using Threads

Changing the server to use threads simply requires a change to the process message (new

lines are emphasized to stand out):

void process(final Socket socket) {

if (socket == null)

return;

Runnable clientHandler = new Runnable() {

public void run() {

} catch (IOException ignore) {

}

}

private void closeIgnoringException(ServerSocket serverSocket) {

if (serverSocket != null)

try {

serverSocket.close();

} catch (IOException ignore) {

}

}

}

Listing A-5

MessageUtils.java

package common;

import java.io.IOException;

import java.io.InputStream;

import java.io.ObjectInputStream;

import java.io.ObjectOutputStream;

import java.io.OutputStream;

import java.net.Socket;

public class MessageUtils {

public static void sendMessage(Socket socket, String message)

throws IOException {

OutputStream stream = socket.getOutputStream();

ObjectOutputStream oos = new ObjectOutputStream(stream);

oos.writeUTF(message);

oos.flush();

}

public static String getMessage(Socket socket) throws IOException {

InputStream stream = socket.getInputStream();

ObjectInputStream ois = new ObjectInputStream(stream);

return ois.readUTF();

}

}

Listing A-4 (continued)

ClientTest.java

Tutorial: Full Code Examples 347

try {

System.out.printf("Server: getting message

");

String message = MessageUtils.getMessage(socket);

System.out.printf("Server: got message: %s

", message);

Thread.sleep(1000);

System.out.printf("Server: sending reply: %s

", message);

MessageUtils.sendMessage(socket, "Processed: " + message);

System.out.printf("Server: sent

");

closeIgnoringException(socket);

} catch (Exception e) {

e.printStackTrace();

}

}

};

Thread clientConnection = new Thread(clientHandler);

clientConnection.start();

}

This page intentionally left blank