Work in the constructor anti-pattern

/**
* This post is intended to be a 101 quickie for the less experienced.
* It does not provide new or innovative ways of solving a certain problem,
* just summarizes a topic the way I see it.
*
**/

This is a topic many have already talked about (a lot), but it still pops up from time to time. Strangely enough, not only in case of junior developers. Continue reading “Work in the constructor anti-pattern”

Advertisements

Awesome self improvement method

The pic below was taken this morning in the office toilet:

SAMSUNG

 

A clean code cheat sheet – loo list! I don’t know who’s got this idea, but it is AWESOME!

Way to go.

Thoughts on clean code

Back in high school I was taught Pascal. It was a really nice way of learning concepts, basics, programming structures in general. As time went by, I discovered Delphi. Now, Delphi (for those who don’t know) is an object oriented language, based on a Pascal-like syntax. And I was like “wow, I can write applications for Windows, but I’m still doing Pascal”. Continue reading “Thoughts on clean code”

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.

No booleans as parameters, please

Ok, first of all; why are boolean parameters bad?

Because they are not input data to the method, but control values. This means that someone from the outside (e.g. the upper level method that calls yours) has explicit control over the method: It simply tells the function what to do. It’s like someone talks to you like: “Draw a smiley face USING A PENCIL” and after that “Sign this paper WITH YOUR PEN”.

You probably know how a drawing is usually created, and you don’t need anyone to tell you what tool to use. The same is true for the signature. There’s a very good example which is usually used for explaining encapsulation: “I don’t need to know how an engine works in order to be able to drive a car”. It’s similar with methods. The caller should not know how the called method achieves a certain goal; it knows that the lower method CAN achieve that goal and that’s it.

They’re also bad because they make your code unreadable. You see that you have to pass  boolean value to a method, but you have no idea what is it used for. So you go to the implementation and check it, wasting precious seconds (or even minutes, if the code is complicated enough)

So, how do we get rid of such methods? Simple. Write two functions:

  1. copy your original function, and give it a self-explanatory name
  2. eliminate the parameter from the newly created method. Replace all the occurrences of the parameter with the value ‘true’
  3. replace the old method call with the new one everywhere where the method was called with true
  4. now rename your original method
  5. eliminate the parameter and replace it with the value ‘false’
  6. solve your compilation problems
  7. refactor the new methods, simplifying boolean expressions (remember? you have trues and falses explicitly hardwired into your code. Those should be eliminated), extracting the common parts and renaming if its needed
  8. be happy, you’ve got rid of your control values

If your booleans are getting out of control, you can try to consider introducing polymorphism. That may lead to even more beautiful design.

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.