Category Archives: programming

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

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:

[sourcecode language=”java”]
if (null == sessionProxy.getShoppingCart()) {
throw new IllegalStateException("ShoppingCart may not be null");
}
[/sourcecode]

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:

[sourcecode language=”java”]
if (!sessionProxy.isShoppingCartSet()) {
throw new IllegalStateException("ShoppingCart may not be null");
}
[/sourcecode]

The method isShoppingCartSet() returns true or false. The implementation will look like this:
[sourcecode language=”java”]
public boolean isShoppingCartSet() {
return getShoppingCart() != null;
}
[/sourcecode]

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:

[sourcecode language=”java”]
ShoppingCart cart = sessionProxy.getShoppingCart();
if (cart == null) {
// do some business logic where cart is null
} else {
// other logic
cart.getSomeProperty();
}
[/sourcecode]

turns into:

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

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:
[sourcecode language=”java”]
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;
}
[/sourcecode]

turns into:
[sourcecode language=”java”]

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;
}
[/sourcecode]

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).

Gotcha’s showing error messages with Spring forms (form:errors)

(I have recently encountered this with Spring 3.1)

Want to validate your forms using Spring? Are you using the form tag and bind it with an object? Got the validator working? But still you just can’t seem to get these error messages showing up? Here is a gotcha that might help you out!

Consider this controller:
[sourcecode language=”java”]
@Controller
public class FormController {

@RequestMapping("/form")
public ModelAndView handleGet(@Valid TellAFriendForm backingForm, BindingResult bindingResult) {
ModelAndView modelAndView = new ModelAndView("backingForm");
modelAndView.addObject("backingForm", backingForm);
modelAndView.addObject("result", bindingResult);
return modelAndView;
}

}
[/sourcecode]

With this jsp:

[sourcecode language=”java”]
<form:form method="POST" commandName="myForm" action="?">
<div>
<label for="name">Name*</label>
<form:input path="name" id="name"/>
<form:errors path="name"/>
</div>
<div>
<input type="submit" value="Send"/>
</div>
</form:form>
[/sourcecode]

When the user submits this form, the form should be validated. Whenever a field does not validate, it should inform the user about this.

This is done by using:
[sourcecode language=”java”]
<form:errors path="name"/>
[/sourcecode]
Where ‘path’ refers to a field in the bound form object, in this case myForm. In this example, I have named my form “myForm”. The theory is that when the form is being validated, the BindingResult (put as “result” on the model), contains all fields that have validation errors. Using the form:errors tag you can make use of that and show error messages.

Although this works often, sometimes it does not and you spend some time figuring out why. A common gotcha is that you forget to put the bindingResult on your model (as “result”).

But there is another less intuitive gotcha.

When your given commandName in the form:form tag does not represent the camelcased, name of the type of the form, the form:errors tag will *not* work. So don’t make up your own name. Make sure it is the same as the type, and make sure it is camelcased correctly. I figured this out the hard way.

Solution: The form object is of type: BackingForm, the expected name would then be backingForm. Change the commandName into backingForm and you will see the form error messages showing up.

A (C++) makefile using a maven like directory structure

When you’re used to Java, you build your software using the comforting build-tools. Popular examples are Ant and Maven. Personally I love Maven.

When you’re writing an application in C++ it seems like a big step backwards build-tools wise. If you’re an Ant God you might feel a bit more comfortable with using Make. However, writing makefiles (the ‘equivalent’ of the Ant’s build.xml) are a more tough.

Since I am used to Maven, there are some tasks I want to perform, like:

– clean
– compile
– compile tests
– package

To start with. I have set up a little makefile that does this for me and for those who just want to get started, share the makefile with you. But what will you get? It won’t be exactly like Maven

It took a little while, and I bet it is far from perfect for real C++ developers who eat makefiles for breakfast. But I am happy with it for now.

The directory structure that needs to be in your project is like this (cpp vs mavenized):

[sourcecode]
CPP Mavenized
——————————————
srcmain srcmainjava
srcmainresources srcmainresources
srcinclude none
srctest srctestjava
binmain target
bintest targettest-classes
lib srcmainresources
[/sourcecode]

When you run ‘make’ you will be doing all tasks. Meaning you will clean, compile everything (your ‘normal’ code and test code) and then copy any libraries, resources and binaries.

Caveats:
– I am compiling without using the -c flag, this simplifies a lot of stuff for me (it works). But, the larger your project the longer it will take to compile, since it will compile everything all the time. So even though you change one file, everything will be recompiled.
– You need to specify sub-dirs when you use them, when there are no files in those dirs the compilation fails (atleast using G++)

The makefile:
[sourcecode language=”bash”]
CC=g++
CFLAGS=-Wall

# MAIN properties
SRC=src/main
INCLUDE=src/include
BIN=bin/main
RESOURCES=src/main/resources
LIB=lib

# TEST properties
SRC_TEST=src/test
BIN_TEST=bin/test
RESOURCES_TEST=src/test/resources

all: clean compile compile-test copy-resources copy-libraries

bin: clean compile copy-resources copy-libraries

copy-libraries:
cp $(LIB)/*.* $(BIN)

copy-resources:
cp -R $(RESOURCES)/ $(BIN)

compile: clean-main prepare-main
$(CC) $(SRC)/*.cpp $(SRC)/domain/*.cpp -I$(INCLUDE) -I$(INCLUDE)/domain -o $(BIN)/d2tm -lmingw32 -lSDLmain -lSDL $(CFLAGS)

compile-test: clean-test prepare-test
$(CC) $(SRC_TEST)/*.cpp -I$(INCLUDE) -o $(BIN_TEST)/tests $(CFLAGS)

clean: clean-main clean-test

clean-main:
rm -rf binmain

prepare-main:
mkdir binmain

clean-test:
rm -rf bintest

prepare-test:
mkdir bintest
[/sourcecode]

Want to see how it is used in a project? Look here.

How stackoverflow helps me to solve problems, without posting questions…

I really love stackoverflow. It brings developers, good ones, together. And because of the community active there, many questions I had where answered.

However, I dare to say half of my questions where answered by not posting any question

The reason why? Its simple. In order to get the answer you seek, you need to specify the problem precisely. Just posting “it does not work” with some vague description is not enough. It really helps to tell how you stepped through the problem, post snippets of code and so forth. If you are able to specify your problem like that, chances are high (at least with me) that the solution comes along with it. A so called “aha” moment. To me it is like Rubber Ducking. By the time you are half-way through writing down the exact problem you probably figured out the problem yourself.

The good news:

  • You probably found 9 out of 10 ‘doh ofcourse its so obvious’ issues. Preventing you to post ‘stupid’ questions…
  • You end up posting really hard questions (making you look so smart).

The better news:

  • There are people out there who are even smarter than you! And make you smarter in return!
  • You do not have to talk to a real rubber duck at your desk, just type your question at stackoverflow. (your co-workers will stop worrying about your mental state, talking to that duck all the time).

Its a win win win!

In other words…
[sourcecode language=”java”]
Answer answer = null;
try {
Question question = formulateQuestionPrecisely();
answer = stackOverFlowService.postQuestionAndGetAnswer(question);
} catch (AhaMomentException ame) {
// never mind, found the solution myself!
answer = ame.getAnswer();
}
[/sourcecode]

Thanks stackoverflow!

(don’t forget to credit those who give the best answers.. it’s highly appreciated)

Now I just read that posting your question, while you know the answer already, is a good thing. So the next time… consider that (but make sure your question has not been asked for already :)).

The ten commandments of egoless programming

Ever heard of the ten commendments of egoless programming? I came accross them a while ago. I just thought of putting them here for history sake, these ten are actually quite good and should be cached on every search engine’s server as many times as possible.

I really like nr 6 a lot. How about you?

So, here a recite:

1. Understand and accept that you will make mistakes.
The point is to find them early, before they make it into production. Fortunately, except for the few of us developing rocket guidance software at JPL, mistakes are rarely fatal in our industry, so we can, and should, learn, laugh, and move on.

2. You are not your code.
Remember that the entire point of a review is to find problems, and problems will be found. Don’t take it personally when one is uncovered.

3. No matter how much “karate” you know, someone else will always know more.
Such an individual can teach you some new moves if you ask. Seek and accept input from others, especially when you think it’s not needed.

4. Don’t rewrite code without consultation.
There’s a fine line between “fixing code” and “rewriting code.” Know the difference, and pursue stylistic changes within the framework of a code review, not as a lone enforcer.

5. Treat people who know less than you with respect, deference, and patience.
Nontechnical people who deal with developers on a regular basis almost universally hold the opinion that we are prima donnas at best and crybabies at worst. Don’t reinforce this stereotype with anger and impatience.

6. The only constant in the world is change.
Be open to it and accept it with a smile. Look at each change to your requirements, platform, or tool as a new challenge, not as some serious inconvenience to be fought.

7. The only true authority stems from knowledge, not from position.
Knowledge engenders authority, and authority engenders respect—so if you want respect in an egoless environment, cultivate knowledge.

8. Fight for what you believe, but gracefully accept defeat.
Understand that sometimes your ideas will be overruled. Even if you do turn out to be right, don’t take revenge or say, “I told you so” more than a few times at most, and don’t make your dearly departed idea a martyr or rallying cry.

9. Don’t be “the guy in the room.”
Don’t be the guy coding in the dark office emerging only to buy cola. The guy in the room is out of touch, out of sight, and out of control and has no place in an open, collaborative environment.

10. Critique code instead of people—be kind to the coder, not to the code.
As much as possible, make all of your comments positive and oriented to improving the code. Relate comments to local standards, program specs, increased performance, etc.

How 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:
[sourcecode language=”javascript”]
someObject: function(data) {
return data.json ? data.json.stateObject ? data.json.stateObject : {} : {};
},
[/sourcecode]

And here is how I refactored it.
[sourcecode language=”javascript”]
someObject: function(data) {
if (data.json) {
if (data.json.stateObject) {
return data.json.stateObject;
}
}
return {};
}
[/sourcecode]

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?

Creating (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:

[sourcecode language=”java”]
public MyClass {
final int someField;

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

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

int getSomeField() {
return someField;
}
}
[/sourcecode]

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:

[sourcecode language=”java”]
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);

}

[/sourcecode]

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:

[sourcecode language=”java”]
public MyClass {
final int someField;

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

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:

[sourcecode language=”java”]
public MyClassFactoryImpl implements MyClassFactory {

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

}
[/sourcecode]

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:

[sourcecode language=”java”]
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;
}
}

[/sourcecode]

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:

[sourcecode language=”java”]
public void someUnitTestUsingMyClassAndMockingAnInterface() {
MyClass instance = MyClass.createTestInstance();
MyCLass someotherMyClass = MyClass.createTestInstance();
EasyMock.expect(someinterface.doSomethingWithMyClass(instance)).andReturn(someotherMyClass);

}

[/sourcecode]

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.

An 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)
[sourcecode language=”cpp”]
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>

}
}
[/sourcecode]

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!

[sourcecode language=”cpp”]
void cGame::think_winlose() {
bool bMissionAccomplished = false;
bool isPlayerAlive= true;
[/sourcecode]
(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:

[sourcecode language=”cpp”]
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;
}
[/sourcecode]

(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!:

[sourcecode language=”cpp”]
bool isPlayerAlive = playerHasAnyStructures(HUMAN) || playerHasAnyGroundUnits(HUMAN);
[/sourcecode]

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
[sourcecode language=”cpp”]
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>

}
}
[/sourcecode]

The 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:

[sourcecode language=”java”]
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
}
}

}
[/sourcecode]

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:

[sourcecode language=”java”]
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);
}

}
[/sourcecode]

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.

Applying a GIT patch on your local SVN repository

Recently someone is helping me out on my Dune II – The Maker project. He works with GIT, a version control system like SVN. I don’t care about the differences between them at this point, except for one difference: The patch files.

So here is the situation: Someone works with GIT, makes some changes and creates a patch file out of them. He sends it to me so I can apply it to my SVN repository. In theory, I thought, this would work fine. Atleast I think the possibility to share patches among different versioning systems would be great, but there is a problem.

The GIT patch file format is different compared to a ‘normal unified diff’ patch, and therefor my SVN client (I use TortoiseSVN) does not understand the patch file and will not apply it. Side note: It does not mention it doesn’t understand it, it simply does nothing.

I found a sollution for this that works 90% of the time (for me). There is TortoiseGIT , which basically offers the same windows integration as TortoiseSVN. However, looking into the ToirtoiseGIT install dir (within the BIN sub-dir) you can find TortoiseMerge.

TortoiseMerge is used within TortoiseSVN for merging and applying (unified diff) patches. But the TortoiseMerge tool within TortoiseGIT understands GIT patch files.

So here is the blunt solution: Open up TortoiseMerge from TortoiseGIT. Open the GIT patch file with it and apply it on your checked-out SVN repository. TortoiseMerge will apply the patch 9 out of 10 times for you.

Sometimes you might encounter an error that TortoiseMerge cannot apply the patch due ‘missing lines’. In those cases I found myself reading the patch file itself and apply it by hand.

I hope this helped you out, if it did (or not) let me know.