Builder vs. Large arg-list constructor

Today I was browsing one of our module’s code. I wasn’t looking for anything in particular, I just wanted to familiarize myself with that part of the system. However, I found something strange; the code was full of object constructions like:

private MyObject myObject = new MyObject(null, null, "", "", importantData, importantData2);

You might say, yes, this class does way too many things if it takes this many arguments. Believe it or not, it actually made sense to keep those data together. The real problem was passing around null and dummy values across the system. And the situation was even worse in case of unit tests.

Let’s see a toy example – a.k.a. code that’s not confidential. We consider the class:

import java.util.Date;

public class User {

	private String userName;
	private String password;
	private String nickName;
	private Date lastOnline;
	private Address address;
	private Balance balance;

	public User(String userName, String password, String nickName,
			Date lastOnline, Address address, Balance balance) {
		this.userName = userName;
		this.password = password;
		this.nickName = nickName;
		this.lastOnline = lastOnline;
		this.address = address;
		this.balance = balance;
	}
}

For the sake of the example, let’s suppose we have object constructions like:

User user = new User("tamasgyorfi", "lotsOfAsterisks", "", null, null, null);

User anotherUser = new User("", "", "chaster", new Date(), null, null);

Well, this is 100% functional, but not that clean. Null values and empty strings make these lines sort of “noisy”. How do we make this cleaner? I think in such situations it is worth to give Builder Design Pattern a try, as it makes object construction more straightforward.

This is how I usually do it, in six easy steps: I

  1. create a new class and name it UserBuilder (I like to add the word Builder so others know what they’re facing)
  2. copy all the fields of the object under construction into the builder
  3. (as far as I know) Eclipse is not able to generate the methods I need, so I have it generate all the setters for the fields
  4. with find/replace I replace all the words “set” with “with”. Also replace the return types from void to UserBuilder
  5. have all the methods return this
  6. create a method named “build”. It does the actual construction and returns the object constructed.

After step 6, we have something like this:

import java.util.Date;

public class UserBuilder {

	private String userName;
	private String password;
	private String nickName;
	private Date lastOnline;
	private Address address;
	private Balance balance;

	public UserBuilder withUserName(String userName) {
		this.userName = userName;
		return this;
	}

	public UserBuilder withPassword(String password) {
		this.password = password;
		return this;
	}

	public UserBuilder withNickName(String nickName) {
		this.nickName = nickName;
		return this;
	}

	public UserBuilder withLastOnline(Date lastOnline) {
		this.lastOnline = lastOnline;
		return this;
	}

	public UserBuilder withAddress(Address address) {
		this.address = address;
		return this;
	}

	public UserBuilder withBalance(Balance balance) {
		this.balance = balance;
		return this;
	}

	public User build() {
		return new User(userName, password, nickName, lastOnline, address,
				balance);
	}

}

So we can replace the object constructions mentioned above with these calls:

UserBuilder builder = new UserBuilder();
User user = builder.withUserName("tamasgyorfi")
				.withPassword("lotsOfSterisks")
				.build();

and similarly:

UserBuilder builder = new UserBuilder();
User anotherUser = builder.withLastOnline(new Date())
				.withNickName("chaster")
				.build();

Cleaner, more straightforward, don’t care things are left out and only relevant arguments are mentioned. And last but not least object construction is moved to one place and it’s not spread across multiple classes.

Note: the ideas above are only valid when your objects are not required to be immutable.

Advertisements

Self improvement group – some feedback

Well, the following post is about some feedback I’ve recently received on my activity regarding our self-improvement group. I was really about to abandon the whole thing, but my friend Lajos Fülep’s kind words made me continue.

In the last two weeks our little self development group was busy with a brand new topic: namely legacy code (legacy code in the sense of: “code that is working suspiciously, and have no tests around it.”).  Some time ago I’ve posted some really ugly code that is full of bugs and has no tests at all. As discussed there, two rounds should be taken: one for adding tests , another one for refactoring it (and fixing the numerous bugs the code contains).

The first round was a pair programming session. The task was to cover the code as much as possible, and spot the mistakes – spot but, don’t touch them (hint: there are at least four major bugs in there). This task can be implemented in an hour. After that, we choose a reference implementation for the second round.

The second round is done randori-based. It means that there is a single laptop along with a projector, while people keep rotating in front of the computer. Everyone is allowed to make a single change. A single change can be: extracting a method, extracting a class, renaming TWO methods, variables etc. Basically the “one change” rule can be adjusted as you wish. I was kind of afraid of this randori-session; I thought people would be afraid of being creative while all fellow group members are looking at them. However, it went really well; people kept saying “I want to stay, this was half a change only”. What is very important here is that anyone not in front of the computer can speak only after the next change to be made has been named by the person at the computer. This means that you cannot give tips what to be done next (only if it was requested explicitly), but can argue whether a change is needed or makes sense at all.

Here’s the feedback I’ve received (translated from Hungarian):

I found this session very useful. What I liked about it was the task itself.  It was cool we had to refactor an ugly piece of code. On the other hand it was very useful to me to see how other people were working and thinking. Way to go 🙂

Code refactoring in progress :-).

Hey Ho, Let’s Go (Agile again)

I’ve already complained enough about us not being agile enough. I also realized that complaining and crying will not help us solve the situation we got into. There were a few problems arisen, ranging from tiny to not-so-tiny. Of course most of them will not go away overnight. It’s useless to tell “we’re doing completely wrong” and not to do anything about it. As one of my former colleagues would always say, “the s**t has to go away”. However, it only goes away if we found out which the s**ty parts were. Continue reading “Hey Ho, Let’s Go (Agile again)”

XML operations – a refactoring dojo

The katas we’ve had lately made it clear that our teams need to be introduced to several refactoring concepts. I decided that I would take some not-that-good-of-quality code, and organize a dojo so we can refactor it. It’s not that easy to find bad code on the internet, and it’s not that easy to come up with a badly implemented project either. I wasted some days searching, but then BANG! I came across this site: http://refactormycode.com/refactorings/recent/java. Actually this is a very strange page. People post bad code there, someone comes along, refactors the whole thing, and posts it back. For free! Some people are either really committed to the community or just have a lot of free time. Anyway…

The code I’ve taken from that site (and made it even worse a bit) looks like:

/*
 * Constraint: parses input strings in the form:
 * <xml><SvcRsData>a<![CDATA[<sender>John & Smith</sender>]]></SvcRsData></xml>
 */
public class StringFunctions {

	private static String string = "";

	public StringFunctions(String string) {
		this.string = string;
		doProcess(string);
	}

	public static String doProcess(String string) {
		int index = string.indexOf("<SvcRsData>");
		if (index > 0) {
			int index1 = index + 11;
			int index2 = string.indexOf("</SvcRsData>");
			String s1 = string.substring(0, index1);
			String s2 = string.substring(index2);

			return doIt(new StringBuffer(s1).append(
					escapeCDataCharacters(string.substring(index1, index2)))
					.append(s2).toString(), false);
		}

		return doIt(string, false);
	}

	private static String escapeCDataCharacters(String string) {
		StringBuffer sb = new StringBuffer();
		int index = string.indexOf("<![CDATA[");
		String myString = string;
		while (index > 0) {
			int cIndex1 = index + 9;
			int cIndex2 = myString
					.indexOf("]]>");

			sb.append(
					myString.substring(0,
							index)).append(
					doIt(myString.substring(
							cIndex1, cIndex2), true));

			myString = myString
					.substring(cIndex2 + 3);
			index = myString
					.indexOf("<![CDATA[");
		}
		return sb.append(
				myString).toString();
	}

	private static String doIt(String s, boolean extendedCharacterEscape) {
		StringBuffer retval = new StringBuffer();
		short character;
		for (int i = 0; i < s.length(); i++) {
			character = (short) s.charAt(i);
			if (character > 128 ? true
					: extendedCharacterEscape ? (character == 34
							|| character == 38 || character == 60 || character == 62)
							: false) {
				retval.append("&#");
				retval.append(character);
				retval.append(';');
			} else {
				retval.append((char) character);
			}
		}
		return retval.toString();
	}
}

So basically that’s it. We have a great dojo material here, it seems. There are no tests, the names are not that self-explanatory, methods are doing more things than they should be, and simply it’s messy. Refactoring, here we go. Tehre is a lot of work to do here, so let’s not waste time, start working! Adding test cases that cover the code is at least half an hour, another one, one and a half for refactoring.

I’ve ended up with something like this: https://github.com/tamasgyorfi/Refactoring-Dojo; It might seem to be an overkill having four different classes plus one more for the tests, but anyway, this way I could encourage my audience writing small methods and classes (so they won’t be afraid of decomposing logics to different abstraction levels). Unfortunately, I did not take into account the time needed for adding test cases so we had a dojo of one hour only. Never mind, will finish it in a second part.