Author: stefanhendriks

bookmark_borderMy answer for – What is wrong with this code #02

Posted on by stefanhendriks

A lot of things can be found in the snippet at “What is wrong with this code #02”. Here it is again:

public MyObject {

	private String myField;
	 
	... (imagine multiple fields) ...

	public GenericObject toGenericObject() {
		GenericObjectFactory genericObjectFactory = GenericObjectFactory.getInstance();
		GenericObject genericObject = genericObjectFactory.create("myObject");
		genericObject.setField("myField", myField);
		// imagine multiple lines using genericObject.setField("field", field)
		return genericObject;
	}

}

Already many things are pointed out in the comments of the initial post.

One thing that immediately struck me is this line:

GenericObjectFactory genericObjectFactory = GenericObjectFactory.getInstance();

Don’t do this.

GenericObjectFactory implies with the getInstance to be a Singleton. Even though it is a singleton, never retrieve and instance inside your method like that.

Even though there “should be only one instance” of the GenericObjectFactory, it still is a dependency for serializing MyObject. Never hide your dependencies. It makes your code hide its intent, it’s hard to decouple, test, refactor, in short hard to maintain. The code should clearly communicate its intent and usage(s).

In my opinion, there are three (well four sort-off) options to deal with this:

1. Deliver GenericObjectFactory as a parameter (invasive in method signature, but automated refactoring tools (i.e. IntelliJ) handle this perfectly):

public GenericObject toGenericObject(GenericObjectFactory genericObjectFactory ) {
	GenericObject genericObject = genericObjectFactory.create("myObject");

2. Inject GenericObjectFactory as a property of MyObject (invasive in constructor)

public MyObject {

	private final GenericObjectFactory genericObjectFactory;

	public MyObject(final GenericObjectFactory genericObjectFactory) {
		this.genericObjectFactory = genericObjectFactory;
	}

	private String myField;

	... (imagine multiple fields) ...

	public GenericObject toGenericObject() {		
		GenericObject genericObject = genericObjectFactory.create("myObject");

2B. If you find option two too invasive, add the default constructor again but let it call the new constructor (makes point 2 less invasive, remove this default constructor after period of time).

public MyObject {

	private final GenericObjectFactory genericObjectFactory;

	public MyObject(final GenericObjectFactory genericObjectFactory) {
		this.genericObjectFactory = genericObjectFactory;
	}

	// add this default constructor
	public MyObject() {
		this(GenericObjectFactory.getInstance());
	}

	private String myField;

	... (imagine multiple fields) ...

	public GenericObject toGenericObject() {		
		GenericObject genericObject = genericObjectFactory.create("myObject");

3. Introduce protected getInstance method in MyObject (least invasive, increases testability)

public MyObject {

	... snip ...


	protected GenericObjectFactory getInstance() {
		return GenericObjectFactory.getInstance();
	}

	public GenericObject toGenericObject() {
		GenericObjectFactory genericObjectFactory = getInstance();
		GenericObject genericObject = genericObjectFactory.create("myObject");
		...
	}

This is sorted by my personal preference. Although option one and option two trade places from time to time. The third option is less favorable, as you still do not expose the need for dependencies, but it does make it more testable. It is also the least invasive way to refactor, which is sometimes beneficial in legacy projects with lots of untested code.

To conclude, never hide your dependencies, if you ever do see some class using getInstance, please, refactor it. It will make your, and your fellow-programmers, life a lot easier.

Thank you!

bookmark_borderWhat is wrong with this code #02

Posted on by stefanhendriks

Given that the functionality of the method toGenericObject must be preserved; there is something obviously wrong in this code, can you find it?

If so, can you think of an easy solution?


public MyObject {

	private String myField;
	 
	... (imagine multiple fields) ...

	public GenericObject toGenericObject() {
		GenericObjectFactory genericObjectFactory = GenericObjectFactory.getInstance();
		GenericObject genericObject = genericObjectFactory.create("myObject");
		genericObject.setField("myField", myField);
		// imagine multiple lines using genericObject.setField("field", field)
		return genericObject;
	}

}

bookmark_borderMy answer for – What’s ‘wrong’ with this code ? #01

Posted on by stefanhendriks

I asked what was ‘wrong’ in the following code. I had put ‘wrong’ in quotes, because it is an ambigious term. Here is the code:

	if (null == sessionProxy.getShoppingCart()) {
		throw new IllegalStateException("ShoppingCart may not be null");
	}

One thing that we find here, is the use of a session proxy object. This is convenient, because it can do things for us at a centralized place.

So what is wrong?
In my opinion there are several things wrong with this code:
– violation of the Single Responsibility Principle
– dealing with null

How to fix this?
Fixing this may not be as obvious as how to detect flaws. This is existing code we are talking about, and you can’t just make changes without making sure you don’t introduce regression.

Solving the violation of the Single Responsibility Principle
We are reading the state of the sessionProxy to execute business logic. We are actually only concerned if the shoppingCart is set on the session. This could be via a null reference, but it could also be done in a different way. What we want is this:

if (!sessionProxy.isShoppingCartSet()) {
	throw new IllegalStateException("ShoppingCart may not be null");
}

The method isShoppingCartSet() returns true or false. The implementation will look like this:

public boolean isShoppingCartSet() {
	return getShoppingCart() != null;
}

The subtle difference is that we now have delegated the question “is the shopping cart set on session” to the class that is responsible for knowing, the sessionProxy.

Solving: Dealing with null
Another advantage by using the isShoppingCartSet method is that we minimize the amount we have to deal with null. We don’t need to check for null explicitly everywhere, we have centralized in one class.

Dealing with null can cause problems, the sooner you don’t have to worry about things being null, the better.

Instead of using a isShoppingCartSet method we could throw a checked exception in the getShoppingCart method. I like checked exceptions, because it forces you to deal with these exceptional situations. It also solves the null problem, as you know it always returns a value or throws an exception when it is (but should not be) null.

There is on caveat here: what if there is existing code relying on the value being null?

The real question is: What does the null value mean? Often it is abused as a status. Some people even use null as a “third boolean” (ie Boolean is ‘true’, ‘false’, or null for ‘unknown’).

As I see it you have an ideal path you want to execute, and then there are things that can go wrong and must be dealt with. In this case, we would expect a shoppingCart so we can do stuff with it. But, if it is not there, we catch the exception and execute other business logic we would otherwise have done with a ‘is null’ check. Ie:

ShoppingCart cart = sessionProxy.getShoppingCart();
if (cart == null) {
	// do some business logic where cart is null
} else {
	// other logic
	cart.getSomeProperty();
}

turns into:

try {
	ShoppingCart cart = sessionProxy.getShoppingCart();
	cart.getSomeProperty();
} catch (NoShoppingCartSetException e) {
	// do some business logic where cart is null
}

The second example clearly defines a ‘happy path’ (within the try). If you use checked exceptions consistently, you will notice your code will become easier to understand. Try it!

What if I don’t want to add an exception to the getShoppingCart method in my current proxy class?
In these cases I would suggest to create a child class of the sessionProxy, which does throw an exception. In the cases where you are absolutely sure that the shoppingCart may never be null, you can use this stricter version of the sessionProxy and deal with exceptions.

Another way of dealing with null – upon construction?
It is also possible to check for null in the constructor(s) of the sessionProxy class. In the case of a http session proxy, it would mean you have to make it immutable to make this work. The reason is that the http session is mutable, even once you have created a sessionProxy. Checking for null values at construction will not guarentee the values are not set on null later on. To fix this, you should create a ‘snapshot’ of the http session at time of construction of the sessionProxy. You read out properties, check for null and set values in private final fields. When you access the fields, you retrieve the fields themselves, and not access them via the http session.

Ie, this:

private final HttpSession httpSession;

public SessionProxy(HttpSession) {
	this.httpSession = httpSession;
}

public String getProperty() {
	String value = (String)httpSession.getAttribute("PROPERTY_KEY");
	if (value == null) throw new PropertyNotOnSessionException("property was not set on session.");
	return value;
}

turns into:


private final String property;

public SessionProxy(HttpSession) {
	this.property = (String)httpSession.getAttribute("PROPERTY_KEY");
	if (this.property == null) throw new PropertyNotOnSessionException("property was not set on session.");	
}


public String getProperty() {
	return property;
}

Concluding
A few lines of code, and yet so much to improve. We have found that:

– We can push the null check into a method of the responsible class. Making the class itself able to answer this business logic.
– We should throw an exception when we want to return null. Dealing with null is hard. When you don’t have to deal with null, your code will get much easier.
– When throwing an exception has too much impact, create a child class which can throw exceptions to reduce impact on current code.
– Checking for null upon construction, for a session proxy, is not possible. Only if you create a DTO out of it (but not a proxy).

bookmark_borderWorking with legacy code – how to start & reveal intent

Posted on by stefanhendriks

Recently I posted my opinion about regression. Regression bugs are likely to occur on projects with a lot of legacy code. I consider legacy code as untested code.

At the legacy coderetreat we used a small codebase (you can find it here). With that codebase we exercised in sessions to improve the code. The nice thing is that this is very similar with your daily job. You open up a project and you have to make changes in code you haven’t seen before and do not understand.

In order to get a better understanding you can use various techniques. I have practiced them with the legacy coderetreat and also applied this at work. In this blog post I’d like to share my experiences. Btw: If you haven’t experienced a coderetreat yet, join one. Just like kata’s, they are really worth your time (and more fun)!

Step 1: Get a sense of what is happening
Before we can do anything, we have to understand what we need to change and how it has impact on the system. One way to find out is to simply execute the code. You could just run the application, or… you could try to write a simple unit test executing the ‘main method’ you think that should be ran. Poke around with the parameters, and see what happens.

I prefer writing Characterization tests. The benefit is that while I am trying to understand what is happening, I am also building a safety net. Writing a Characterization test goes like this:
– create new test
– do some setup
– run specific piece of code (method) you want to try out
– check outcome / read state
– create assertion to make it pass with the outcome

When I don’t know what it actually does, I call my tests ‘monkey‘. Once I know the behavior with the given input, I rename the test to what the behavior is. Example:

package com.adaptionsoft.games.uglytrivia;

import org.junit.Assert;
import org.junit.Test;

import static org.hamcrest.core.Is.*;
import static org.junit.Assert.*;

public class GameTest {

	@Test
	public void isPlayableReturnsFalseWhenInitialized() {
		Game game = new Game();
		assertThat(game.isPlayable(), is(false));
	}

	@Test
	public void isPlayableReturnsTrueWithTwoPlayers() {
		Game game = new Game();
		game.add("Stefan");
		game.add("Niels");
		assertThat(game.isPlayable(), is(true));
	}

	@Test
	public void monkey() {
		Game game = new Game();
		game.add("Stefan");
		game.add("Niels");
		game.roll(5);
		// no idea yet what happens, need to look into roll method to get a clue
	}

}

So this gives me a rough idea what is happening, and it gives me a suite of tests.

It is important that you focus on black box tests. Try not to bother about the internals. If you are deep-stubbing in your test setup then try to think of a different way to approach the problem. Sometimes it is not possible to do black box testing, only then you need to do white box testing. In these cases deep-stubbing is often needed. Deep stubbing indicates a design problem: your class is bothered with internal states of other objects. You can reduce this by applying Tell Don’t Ask.

Step 2: Reveal intent.
This is even less invasive (actually it is not invasive at all if done well) than the small refactorings I have blogged about in the past.

To reveal intent:
– go through the code, find magic numbers and strings. Introduce constants for them with descriptive names
– find method names that do not describe well their behavior, and rename them. Try to keep the name about behavior, and if it does more then one thing, concate these behaviors with “And”.
– do the same for variables

This may sound trivial, but it really enhances the understandability of the code. As a bonus your understanding of the code is increased a lot, and all you did was renaming things and perhaps introduced a few constants. Let me show you how much it matters:

Can you find things to improve in this code?

if (roll % 2 != 0) {
	isGettingOutOfPenaltyBox = true;

	System.out.println(players.get(currentPlayer) + " is getting out of the penalty box");
	places[currentPlayer] = places[currentPlayer] + roll;
	if (places[currentPlayer] > 11) places[currentPlayer] = places[currentPlayer] - 12;

	System.out.println(players.get(currentPlayer)
			  + "'s new location is "
			  + places[currentPlayer]);
	System.out.println("The category is " + currentCategory());
	askQuestion();
} else {

What about this?

if (roll % 2 != 0) {
	isGettingOutOfPenaltyBox = true;

	System.out.println(players.get(currentPlayer) + " is getting out of the penalty box");
	places[currentPlayer] = places[currentPlayer] + roll;
	if (places[currentPlayer] > PLACE_BEFORE_STARTING_PLACE) places[currentPlayer] = places[currentPlayer] - MAX_PLACES;

	System.out.println(players.get(currentPlayer)
			  + "'s new location is "
			  + places[currentPlayer]);
	System.out.println("The category is " + getCurrentCategoryForCurrentPlayerOnPlace());
	askQuestionAndRemoveFromQuestionFromDeck();
} else {

This method name is called “roll” initially. If you would sum up all its behavior it would be more like:

	public void movePlayerAmountRolledAndAskQuestionOrWhenInPenaltyBoxIfUnevenRolledGetOutOfPenaltyBox(int roll) {

Who would ever accept such a long method name? I would, but it should trigger something. This method name tells you there is way too much going on in one place. And, since the method is public, we communicate to other classes what this thing is doing.

It is ok to rename multiple times. The longer you work with the code, the better you understand it. When the method names do not reflect their real intent, make it clearer and improve their names. Communicating what the code actually *does* is important, make it explicit. especially if the method name violates conventions (ie, a getSomething() method that is not getting a property, but does more than that.)

It is very tempting to extract expressions and methods
Before you do this. Make sure you have the Characterization tests and integration tests in place. The tests will tell you if you have broken something while refactoring using extract method or extract conditions into variables. Yes, even such small refactoring’s could cause bugs.

Here an example, take this expression:

	if (rolled % 2 != 0) {

Which you could turn into (extract into variable):

	boolean isUnevenRoll = roll % 2 != 0;
	if (isUnevenRoll) {

Or extract method:

	if (isUneven(roll)) {

I prefer (automated!) extract method over extracting into variables. The main reason is that extracting into methods introduce very small pieces of code that you can re-use. You could eventually even find that the methods are not particularly bound to the current class’ behavior and move them out of this class into a new class. With variables this is much harder to see and refactor.

With these two steps, we could have brought the code we had earlier into a state like this:

if (isUneven(roll)) {
	isGettingOutOfPenaltyBox = true;

	System.out.println(getCurrentPlayer() + " is getting out of the penalty box");
	moveCurrentPlayer(roll);

	System.out.println(getCurrentPlayer()
			  + "'s new location is "
			  + places[currentPlayer]);
	System.out.println("The category is " + getCurrentCategoryForCurrentPlayerOnPlace());
	askQuestionAndRemoveFromQuestionFromDeck();
} else {

Conclusion
When working with legacy code, it is of importance to understand the code before making changes. In order to understand the code we can use introduce constants or rename methods to make the code reveal its intent. Using Characterization tests we can fixate the current behavior and label it in our tests names. Then, once we have this test suite, we can start using small refactoring’s like extract method or extract variable to make conditionals reveal their intent.

When creating a test suite, creating mostly black box tests will help us in the future when refactoring opposed to white box tests. Sometimes white box tests cannot be avoided.

Without any tests we can already have more insight in what is happening. With a test suite we can more safely start refactoring.

More about coderetreats
I have been greatly inspired by the legacy code retreat day, where we could experiment more in our spare time. Just like the previous time I have learned a lot, and I am convinced that others will benefit from this as well. Therefor I have decided to lend a hand and offer to organize and facilitate a coderetreat myself at the end of this year. Stay tuned!