Sep 18 2009

Extreme Refactoring or Is Guard clause Considered harmful?

Category: agile,javagiordano scalzo @ 4:56 pm

During the JDave try, the focus was on Bdd side of the exercise, but, nonetheless, the StringTemplater’s code was yelling “Refactor me! Refactor me!” at me :-) .

Actually, that I like least, it’s the Guard Clause, I used to split a marker in key-value form:

private String name(String[] pair) {
		if (pair.length != 2)
			return "";
		return "$" + pair[0].trim();
	}

Usually I like that pattern, it’s a well know way to avoid nested conditional, but Anti-If Campaign, promoted by Francesco Cirillo made me think.

The first try didn’t satisfied me:

private String name(String[] pair) {
		return  (pair.length != 2) ? "" : "$" + pair[0].trim();
	}

Indeed, I believe it’s more cryptic and ugly than the original one.

Looking the code more carefully, I saw another thing: I was violating the principle “Ask, don’t tell“, handling some data that could be tied together.

Creating the new class, I thought another thing:
Why am I using the ‘IF?”
I did for avoid a runtime exception… but is an exception evil?
It isn’t if we can handle it!

So at the end this is my final product:

package biz.scalzo.kata.stringtemplater.jdave;

import java.util.Arrays;
import java.util.Iterator;
import java.util.NoSuchElementException;

public class StringTemplater {

	public String replace(String stringToReplace) {
		return replace(stringToReplace, "");
	}

	public String replace(String stringToReplace, String markers) {
		return replaceEmptyMarkers(replace(stringToReplace, initialIterator(markers)));
	}

	private String replaceEmptyMarkers(String stringToReplace) {
		return stringToReplace.replaceAll("\\$\\w+", "");
	}

	private Iterator initialIterator(String markers) {
		return Arrays.asList(markers.split(",")).iterator();
	}

	private String replace(String stringToReplace, Iterator markers) {
		try {
			return replaceMarker(stringToReplace, markers);
		} catch (NoSuchElementException e) {
			return stringToReplace;
		}
	}

	private String replaceMarker(String stringToReplace,
			Iterator markers) {
		Pair pair = new Pair(markers.next());
		String newString = stringToReplace.replace(pair.name, pair.value);
		return replace(newString, markers);
	}

	private class Pair {
		private String name;
		private String value;

		private Pair(String nameAndValue) {
			init(nameAndValue.split(":"));
		}

		private void init(String[] nameAndValue) {
			try {
				name = "$" + nameAndValue[0].trim();
				value = nameAndValue[1].trim();
			} catch (IndexOutOfBoundsException e) {
				name = "";
				value = "";
			}
		}
	}
}

I think a I could refactor more, but I was enough satisfied: I reached my goal to avoid IF and I’ve start to think the exception in different way.

Technorati Tags: , ,

Tags: , ,