bookmark_borderHow to fry your co-developers brain; and then make it better

Here is a little example of code I’ve been faced with (not written by me), that struck me. Although the syntax is correct (it is javascript), it took me a little while to actually understand what is going on.

Here is the code:

someObject: function(data) {
	  return data.json ? data.json.stateObject ? data.json.stateObject : {} : {};
},

And here is how I refactored it.

   someObject: function(data) {                  
		  if (data.json) {
				if (data.json.stateObject) {
					return data.json.stateObject;
				}
		  }
		  return {};                             
   }

So I mentioned brianpower in a while ago. Michael Feathers though calls such a thing “forming a mental model“. Which is exactly what I meant. You also hear this in the scene of Usability; the less your mental model matches with the actual object you use (expectations), the less likely you’ll probably understand it, let alone *use it*.

So tell me, which one is easier to understand? And how much impact do you think this has?

bookmark_borderCreating (default) test instances of a class, without exposing default constructor.

Did you ever need to just have an instance of a class you cannot instantiate because the default constructor is not available? Do you want to create a test instance just to be used in unit tests? Don’t want to break up the design of your code just for testing? This post might help you:

Sample code:


public MyClass {
final int someField;

private MyClass() {
// may not use this
someField = -1;
}

public MyClass(int someFieldValue) {
someField = someFieldValue;
}

int getSomeField() {
return someField;
}
}

So lets say this class is used in a lot of places. And all we want is have a default test instance. We don’t care about the internals. Lets say we only use it to toss around using an interface which we mock in our tests. Like so:


public void someUnitTestUsingMyClassAndMockingAnInterface() {
MyClass instance = new MyClass(-1); // this works, but its not really descriptive
MyClass someotherMyClass = new MyClass(-1); // like, what is -1? we don't want to be bothered by this
EasyMock.expect(someinterface.doSomethingWithMyClass(instance)).andReturn(someotherMyClass);
...
}

There are two ways to approach this problem:

  • Make private constructor public
  • Use a Factory or a creation method

So, just making the default constructor public sounds easy and is used very often. And, because we want to let our fellow developers know it is just to be used in unit-tests we place a comment in them. This mostly results in something like this:


public MyClass {
final int someField;

public MyClass() {
// Use this only in tests!
someField = -1;
}

}

However, this has a few drawbacks:

  • You cannot see if the default constructor should only be used for tests, unless you look into the code itself and read that comment line
  • You broke with the initial design decision to have only objects instantiated with a given “someField”

So how do we prevent this?

We use a factory, or a creation method. Lets start with the first, the factory: If there is a factory, just use it to get a test instance. If the factory lacks a ‘createTestInstance()’ method, then add it. Like so:

public MyClassFactoryImpl implements MyClassFactory {

public MyClass createTestInstance() {
return new MyClass(-1);
}

}

We could discuss about wether you want the method to be factory method to be static here, and so forth, but the idea is clear.

However, when there is no factory present it is unlikely you want to create a factory just for your unit tests. In that case I suggest you use the creation method, also refered to as the factory method pattern. Simply add a public static method that returns a new instance. The final result would be:

public MyClass {
final int someField;

public static MyClass createTestInstance() {
return new MyClass();
}

private MyClass() {
// may not use this
someField = -1;
}

public MyClass(int someFieldValue) {
someField = someFieldValue;
}

int getSomeField() {
return someField;
}
}

The benefits are clear:

  • The default constructor remains private, so the design is left untouched
  • It is clear what the createTestInstance method does. No comments needed.

And in your unit test (using EasyMock as well) it looks like this:


public void someUnitTestUsingMyClassAndMockingAnInterface() {
MyClass instance = MyClass.createTestInstance();
MyCLass someotherMyClass = MyClass.createTestInstance();
EasyMock.expect(someinterface.doSomethingWithMyClass(instance)).andReturn(someotherMyClass);
...
}

Note: When you wind up with multiple create methods you should consider using the Factory pattern instead. Yes, creating a factory is the best thing once creation logic starts to dominate your class. (ie, think of the Single Responsibility Principle).
Note 2: If you’re a design purist, you might even debate that the public constructor of MyClass should be a creation method. It improves consistency as all instances are created by creation methods. It also has the benefit of describing what kind of instance you get (ie, what the field means). Such improvements are valid and should be made. I consider these improvements small refactorings. They should never be undervalued.

bookmark_borderAn example of refactoring

As I have promised in my previous post, I would post an example of small refactorings in order to greatly improve the readability and understandability of code.

I own a little project called Dune II – The Maker, and I started writing it a little over 10 years ago. In those years I have learned a lot. I did not have much time in those days to apply my new knowledge to the project. You could say the software was rotting. In order to make it better I need to refactor a lot and I encounter the best examples to improve code without pointing fingers :). In any case I have experienced you have to make mistakes in order to get better. I hope you will learn from the mistakes I made.

So here is a little example I have just checked in the dune2themaker repository, I’ll give you the before (revision 411) and after (revision 412). Of course, I have taken smaller steps to get to the end result. First the original piece of code:

Revision 411 (before)

void cGame::think_winlose() {
	bool bSucces = false;
	bool bFailed = true;

	// determine if player is still alive
	for (int i = 0; i < MAX_STRUCTURES; i++)
		if (structure[i])
			if (structure[i]->getOwner() == 0) {
				bFailed = false; // no, we are not failing just yet
				break;
			}

	// determine if any unit is found
	if (bFailed) {
		// check if any unit is ours, if not, we have a problem (airborn does not count)
		for (int i = 0; i < MAX_UNITS; i++)
			if (unit[i].isValid())
				if (unit[i].iPlayer == 0) {
					bFailed = false;
					break;
				}
	}

	// win by money quota
	if (iWinQuota > 0) {
		if (player[0].credits >= iWinQuota) {
			// won!
			bSucces = true;
		}
	} else {
		// determine if any player (except sandworm) is dead
		bool bAllDead = true;
		for (int i = 0; i < MAX_STRUCTURES; i++)
			if (structure[i])
				if (structure[i]->getOwner() > 0 && structure[i]->getOwner()
						!= AI_WORM) {
					bAllDead = false;
					break;
				}

		if (bAllDead) {
			// check units now
			for (int i = 0; i < MAX_UNITS; i++)
				if (unit[i].isValid())
					if (unit[i].iPlayer > 0 && unit[i].iPlayer != AI_WORM)
						if (units[unit[i].iType].airborn == false) {
							bAllDead = false;
							break;
						}

		}

		if (bAllDead)
			bSucces = true;

	}

	// On succes...
	if (bSucces) {
		// <snip>

	}

	if (bFailed) {
		// <snip>

	}
}

The intention of the think_winlose() function is to determine if the player has won or lost, and if so it transitions the game state. These transitions have been snipped.

So when does a player win or lose? It depends if there is a ‘win quota’, or not. The win quota is a number, whenever it is above zero it means the player has to collect at least that many of credits (spice) in order to win. If the win quota is not set, the default win rule : destroy everything of the enemy, will be used. (do you notice I need this much text for just a simple rule? Which I could have prevented If I had code that said this in the first place? At the bottom of this post you can see what I mean :))

Lets take a look at the code and point out what could be done better:

  • There are two booleans bSuccess and bFailed. Which is confusing and ambigious. What is succesfull? What did fail? Why aren’t they one boolean?
  • There are comments all over the place, meaning we could refactor these pieces to code so comments are not needed. (Comments are seen as clutter and should be removed)
  • The code formatting could be done better. If statements should start with { and end with }, even with one line.

And there are more things you will probably find yourself. What I’ll do is point out a few things that could be improved. If you just want to see the final result, just take a look below.

Lets start with the booleans bSuccess and bFailed. Why are there two booleans and whey are they called so vaguely? A little bit of searching in the code and we find out that bSuccess actually means “Mission is accomplished” (player has won), and bFailed means the player has no units and structures (which implicates the player has lost the game). They are not the same boolean, because a player could be alive and not have yet won the game of course. Now we know they are not actually the same boolean, but their naming was vague. A simple “rename variable” made things easier to understand!

void cGame::think_winlose() {
	bool bMissionAccomplished = false;
	bool isPlayerAlive= true;

(when posting this I realize the two booleans are named differently, consistency is also important to improve readability, so either both should start with “is” or both with a “b”, I prefer the first though)

Right after the booleans a few for loops are used just to find out if there is anything alive for the player. A little bit below we see such for loops again, but for the AI. This is duplicate code and should be removed. Extracting them into a method and make them return a boolean value is easy to do:

bool cGame::playerHasAnyStructures(int iPlayerId) {
    for (int i = 0; i < MAX_STRUCTURES; i++) {
		if (structure[i]) {
			if (structure[i]->getOwner() == iPlayerId) {
				return true;
			}
		}
	}
    return false;
}

(Again, while posting this I realize this could be even improved a bit more, the iPlayerId should be called ownerId (or the getOwner should be a getPlayerId), so it is obvious we match two of the same kind. Now it could confuse us: is an owner the same as the playerId? Since I know it is, why isn’t it called that way?… :))

Since we extract these for loops we can now set the isPlayerAlive boolean immidiately instead of setting a variable within the loop as it was done in the original example above. Reducing 24 lines into one!:

bool isPlayerAlive = playerHasAnyStructures(HUMAN) || playerHasAnyGroundUnits(HUMAN);

The final result of revision 412 is shown below. It will clearly show the major improvement regarding readability and understandability. Any other developer who comes to this code can see what it does and it is almost a no-brainer.

Result revision 412

void cGame::think_winlose() {
	bool bMissionAccomplished = false;
	bool isPlayerAlive = playerHasAnyStructures(HUMAN) || playerHasAnyGroundUnits(HUMAN);

    if (isWinQuotaSet()) {
		bMissionAccomplished = playerHasMetQuota(HUMAN);
	} else {
		bool isAnyAIPlayerAlive = false;
		for (int i = (HUMAN + 1); i < AI_WORM; i++ ) {
			if (playerHasAnyStructures(i) || playerHasAnyGroundUnits(i)) {
				isAnyAIPlayerAlive = true;
				break;
			}
		}

		bMissionAccomplished = !isAnyAIPlayerAlive;
	}

    if (bMissionAccomplished) {
		// <snip>
		
	} else if (!isPlayerAlive) {
		// <snip>

	}
}

bookmark_borderThe tremendous power of tiny refactorings

More and more I am being intrigued by the power of a small code refactorings. The positive impact it has on the readability, the maintainability and understandability of your code is great. It keeps code clean(er) and since the changes you make are really small (I’ll demonstrate how small), the chance they will break things is small. Of course, with unit tests (you are writing them right?) making sure you did not break anything: a small refactoring is a low-risk high-benefit practice.

In my experience, small refactorings are undervalued. In fact, I undervalued them much myself since not too long ago. They are disregarded as refactorings that don’t help at all, because it is obvious what the code does. However, the flaw in this rationale, as I see it, is that the intended audience is not only you but also the other developer you work with. Also, you know what code does right now. But would you understand it as quickly if you did not look at it for a week and came back? Would another developer understand the code right away?

When working on code, you’re constantly trying to ‘translate’ the code in your mind in order to know what it is doing. Doing this it leads you to where the bugs are or the areas where you need to make changes, etcetera. This process of ‘translating’ code in your mind comes at a price. Literally the energy you need to burn in your brain to grasp the meaning of a piece of code: brainpower; The easier we understand code, the less brainpower we need. The less energy we burn by understanding what is going on, the more energy we have left to create new things, or fix that bug.

I’ve created a little example. The code below represents an implementation of a mail service. The mail service allows you to send an email using a method that uses 4 parameters: to, from, the subject and the message. When all parameters are filled, the email needs to be sent. That is the only requirement for now. Of course, later we might want to validate if the given email adress of from and to are valid. But for the sake of the argument, lets keep it simple. The following code is ‘mind-boggling’, compared to its simple intention:

public class MailServiceImpl implements MailService {

	public void sendMail(String from, String to, String subject, String message) {
		if (from != null && !"".equals(from) &&
			to != null && !"".equals(to) &&
			subject != null && !"".equals(subject) &&
			message != null && !"".equals(subject)) {
			// send the email
		}
	}

}

Basically what this says it that any field may not be null or an empty string. It took 4 lines for just to show. Even though you recognize the pattern of a ‘null or empty check’, it costs you time and energy to make that translation happen. So here is a first suggestion to make it read easier:

public class MailServiceImpl implements MailService {

	public void sendMail(String from, String to, String subject, String message) {
		if (parametersAreNotNullOrEmpty(from, to, subject, message)) {
			// send the email
		}
	}

	private boolean parametersAreNotNullOrEmpty(String from, String to, String subject,
			String message) {
		return from != null && !"".equals(from) &&
			to != null && !"".equals(to) &&
			subject != null && !"".equals(subject) &&
			message != null && !"".equals(subject);
	}

}

When another developer is reading the sendMail method, he will now know that when the parametersAreNotNullOrEmpty the mail will be sent. It does not need any translation, the method name just says what it does! Simple! By doing this, you greatly reduce the needed brainpower to understand what is going on. The refactoring method used is called Extract method.

Reading code is sometimes easy for your brain to handle. Sometimes your brain seems to explode because of the complex statements and context you need to be aware of. It is strongly tied with the Cyclomatic Complexity, the Coupling between Objects (CBO) and the lack of Cohesion in your code. If you are using any tools to measure your code, like Sonar for example, look for these metrics to find code that needs attention. But it is always better to refactor while you have made the translation in your brain, if you see things can be written simpler to reduce the needed brainpower, by all means do so. Not giving software the appropriate attention might let your code rot. Small refactorings help you prevent that.

I hope you have seen a bit of the power of small refactorings. I will get back to them in my future posts as I will post more concrete examples and how I would/have dealt with them. To me, small refactorings need to be part of your system and are introduced when you do TDD. All too often when the code works, it is not looked at again. Making these small refactorings can make a big difference and take relatively no time.