Wednesday, March 30, 2011

Duplication is evil, double duplication is worse

Think of an application with a domain model in which a person can have several emails, each of which has a different role (e.g. private, office, preferred, and so on). Persons can choose their preferred email for normal communications, but there are other communications that are normally sent to the email with a given role (e.g. communications about personal health will not be sent to an email which is also read by secretaries); only when an email with this role is missing the system reverts to a default email, which is chosen between the existing ones applying a chain of rules.

In an application like this you could (hypotetically speaking, of course) find a snippet of code that returns the email corresponding to a given role:
public String getEmailForRole(Role role) {
String email = emails.get(role);
return email != null && isValidEmail(email)
? email
: getDefaultEmail();
}

public boolean isValidEmail(String pattern) {
return (...stuff...);
}
The isValidEmail method checks the email against a simple regular expression. This snippet springs at least two different considerations.

Check for correctness

One might think that the check for validity is unnecessary, after all checks are done while inserting and updating, right? Partially. Let's just say all checks should be done there, and let's not forget that data greatly outlive applications, so you could easily have strings with completely different meaning stored where only emails should be. Sounds ugly? Welcome to reality. Sure, you could quite easily find all instances of strings that are not emails, but when the customers asks you to leave stuff as it is you have very little power. Other things I've seen include courses descriptions instead of teachers and documents instead of relatives. To cut a long story short, better safe than sorry.

Be clear on intents

Stick with them and hide mechanics. In particular, here we have a duplicated duplication (pun intended):

  • the check for null values must be duplicated in every snippet that contains emails.get(role)
  • the check duplicates the intent of the isValidEmail method. I think this duplication is worse than the previous one, first of all because the redundancy blatantly hits the eye, but most of all because there is a conceptual duplication: the responsibility for the check should lie in the idValidEmail method itself.

If there's an isValidEmail(String pattern) method I expect it to check for null values without having to bother myself:
public String getEmailForRole(Role role) {
String email = emails.get(role);
return isValidEmail(email)
? email
: getDefaultEmail();
}

public boolean isValidEmail(String pattern) {
return pattern != null && (...stuff...);
}
A more elegant solution could be the creation of an immutable Email value object and the use of the NullObject pattern, a special case of the more general Special Case pattern (another pun intended); in this case the emails object would return a NullEmail object that implements the same interface of the Email class:
public Email getEmailForRole(Role role) {
Email email = emails.get(role);
return email.isValid
? email
: getDefaultEmail();
}
A simplistic implementation of the NullEmail object could be
class NullEmail {

private Role role;

public NullEmail(Role role) {this.role = role;}

public String getValue() {return "";}

public Role getRole() {return role;}

public boolean isValid() {return false;}
}
If we wanted to explicitly return a String instead of an Email object we could slightly modify the getEmailForRole method:
public String getEmailForRole(Role role) {
Email email = emails.get(role);
return email.isValid
? email.getValue()
: getDefaultEmail();
}
As you might have noticed, nothing changed in our NullEmail class - nor in the Email class, which is not shown here.

Of course in this particular case we still have to check for the validity of the email, but in many other places (e.g. print emails of a person for every possilbe role) we can safely operate on our NullObject just as we would on a valid one.

It could be overkill, but it surely gets rid of one of the worst plagues in software: duplication. But that's the subject for another post :-)

Saturday, March 26, 2011

On "Working Effectively with Legacy Code"

In the last few weeks I have spent most of my time working on a legacy system which is going to be partly replaced by a new one which greatly broadens its scope. But what exactly is legacy code? Michael Feathers says that
To me, legacy code is simply code without tests.

It is as simple as that:
Code without tests is bad code. (...) With tests, we can change the behavior of our code quickly and verifiably. Without them, we really don't know if our code is getting better or worse.

Alas, too often the answer is not the one we'd like to hear.

We started out so well

Almost every system, if you have at least decent programmers working on it, starts out as a wonderful green field: defined architecture, code conventions, solid principles, patterns and practices.

After a while the gardeners get busy with other gardens, and a couple of weeds start to poke out. Then some more, then more. And more. Meanwhile seasons pass and rain falls, so our green field gets muddier and muddier. Actually it looks more like it has rained cats and dogs, day after day after day. Mud becomes quicksand, and nobody dares to even get close to it.

The (in)famous spaghetti incident

Too soon the project reaches the point in which whenever you pull a string you have an undesired and often undetected change in a completely different part of the system. When I want to sound important I refer to this as the butterfly effect, a metaphore used in chaos theory. Yes, I know that your projects are different and you have never seen it happen, so try to use your imagination and stay with me a little more, will you?

That was exactly where I was, all tangled up in obscure dependencies: the perfect spaghetti code.


As I tweeted, I love spaghetti when they are in my dish, not in my code, so I had to do something (even because deadlines are always waiting in ambush). To sharpen my tools I retrieved my copy of "Working Effectively with Legacy Code". The book covers many aspects of what we developers have to face daily: instead of the "write once, run anywhere" mantra, we have to deal with "write once, read it at least ten times", but too often our code is obscure. Yes, our code. And we don't have tests, or at least we don't have enough. Do you have a complete coverage? If you do, you have all my respect, otherwise welcome in the family.

How do we get out of this?

First of all, we have to get our code into a test harness, but it is not as easy as it sounds. The book contains a series of 24 different dependency breaking techniques, each of which presented in several almost-real situations, to minimize the impacts of changes and ensure that we are not breaking anything. It sounds like the classic catch-22 thing:

When we change code, we should have tests in place. To put tests in place, we often have to change code.


This is where the techniques come in handy. I was pleasantly surprised when I discovered that, after some years of TDD, almost every problem and solution described in the book sounded familiar, so I skimmed it more than actually reading it. I still remember when I first read it: man it was hard, not the writer's fault, but because of the reader.
One of the most important things is that you have to preserve the behavior of your system when you refactor it (and preserve the rest of the system when you introduce new features or change existing ones), so you should at least know what it does. What is better than some characterisation tests?

Cleaning it up

I love NetBeans. I always have, since it was Forte4Java. Now I love it even more because it has a heap of automated refactorings that make "Refactoring" needless (ehm... that's not true, NetBeans deals with the mechanics, but you should know what you're doing, so stop reading and go buy your copy if you still don't have it. Done? OK, let's get on).

Do you need to create a new test class? nothing simpler, just a simple SHIFT + CTRL + U and a template with all the skeleton methods is ready for you. Just punch in the starting conditions and the expected results and you're done. Feathers describes all refactorings steps by steps, but a few keystrokes are all NetBeans needs to extract superclasses, methods, interfaces, pull up or push down members and methods, delegate, and so on and so forth. Nevertheless, you should know what you're doing.

While working your code could get uglier in some places. It could be temporary, or temporary in the Italian way (which means definitive). Even in the latter case, at least you would have tests in place so that you know you're not breaking anything.

In several places Feathers suggests to reduce incapsulation to put code into a test harness. Somebody might find it strange, extremists might find it insane. Yet...

Encapsulation isn't an end in itself: it is a tool for understanding.


Remember when I wrote that you write code once and read the very same code over and over? What 's the point of an encapsulated design if I spend hours for the most trivial task? Mnd you, I'm not suggesting every field must be public and collections should be directly exposed, but nobody is going die if a private method becomes protected for the sake of testing.

Where have my spaghetti gone?

After a while, one after the other, your pull spaghetti out of your dish, slowly replacing them with neat and tidy sushi.

Before you realize it, you will look at your codebase with different eyes, and chances are that you will enjoy dealing with legacy code. Don't enjoy too much though, even if you have a lot of technical debt to pay: you don't have to pay it for the sake of doing it, but because it allows you to move faster when you need it (and you know you will need it).

Let me stress this again: if I didn't have some hundreds of tests already in place, these weeks would have been months. Baby steps, nip and tuck, red and green bar. But I only introduced tests and refactored legacy code when I needed it, otherwise I would spend too much time on activities that were not a priority. See something rotten? pen it down, fix what you're doing and if you still have time you can deal with it. Look for decisions that could change, but change your code just in time (the YAGNI rule rules).

Tuesday, March 8, 2011

How to use Java to call an IBM i program

This post is a late update to a post I wrote a couple of years ago about exposing RPGLE programs so that we could call them from our webapps. That worked fine, but I didn't particularly like the workflow we used to produce the wrapper, so I tried to get rid of some muda.
The worst one was, in my opinion, the need to use two different IDEs, thus necessarily involving the collaboration of two persons (not every developer has IBM tools) that creates a bottleneck.
After some researches I found some official documentation (the problem with IBM is not scarcity but abundance), dug for our jt400.jar and started to experiment a little.

A small word of caution: the article is written in an introductory style, showing the TDD process that lead to the implementation of the functionality I needed. If you're only interested in the results you can easily skip to the end of the article (I will not know it, so I won't get offended).

The guinea pig for my experiment is a simple program that given a table name returns a serial number which is unique for that table. As I had have to use this program in an application that already uses a serial number generator, first I need an interface, so I use the NetBeans Extract Interface refactoring on my existing service:
public interface SerialsService {
long getSerialNumber(String tableName);
}
That done I create an implementing class that gets the serial from the iSeries:
public class MyIseriesSerialsService implements SerialsService {
public long getSerialNumber(String tableName){
throw new UnsupportedOperationException();
}
}
OK now we can start the real stuff. First thing first, we write a couple of simple tests in a new test class:
@Test(expected = "IllegalArgumentException.class"
public void testWithWrongTableNameShouldThrowException() {
MyIseriesSerialsService instance = new MyIseriesSerialsService();
instance.getSerialNumber("WrongName");
}
CTRL + F6 to start the test and NetBeans gives me a red bar. I change the type of exception thrown, CTRL + F6 again and here's a nice green bar. That was easy, wasn't it? now we have a service that behaves correctly when you don't invoke it correctly. This is good, but if your application is like mine I suppose you expect your service to behave correctly even when it is correctly invoked (yes, sometimes it happens).

To call an IBM i program you first have to connect to an iSeries, and for this you use the AS400 class, which IBM documentation actually calls AS400 IBM® Toolbox per Java™: I'll stick to that, so that is what I mean when I simply write AS400 - the same holds true for all other registered trademarks. If this is enough to keep lawyers at bay, as I hope, this ends the disclaimer.

The AS400 class, amongst other things, manages socket connections on behalf of a user, so at least we have to tell our service what is the iSeries we want to connect to, and which user we want to impersonate. This requires to change the constructor of the service (obviously it is not the only way, but I'd rather use an immutable object). We could pass in the name of the server (or the ip address if you don't want to rely on a DNS service), a username and a password, or we could create an AS400 object and pass it to the constructor of the service. This option separates responsibilities better (and can be tested more easily), so I choose it and write a simple test to check the connection to our system:
@Test
public void testConnection() throws Exception {
AS400 system = new AS400(host, username, password);
assertNotNull(system.getRelease());
System.out.println(system.getRelease());
}
I am just experimenting, so as always I defer all the exception dealings. Green bar. I can now add a field to the constructor of the service...
public class MyIseriesSerialsService implements SerialsService {

private AS400 system;

public MyIseriesSerialsService(AS400 system) {
this.system = system;
}

public long getSerialNumber(String tableName) {
throw new IllegalArgumentException();
}
}
...and modify the tests accordingly, also removing a small duplication:
@Before
public void setUp() {
instance = new MyIseriesSerialsService(createValidSystem());
}

private AS400 createValidSystem() {
return new AS400(host, username, password);
}

@Test(expected = IllegalArgumentException.class)
public void testWithWrongTableNameShouldThrowException() {
instance.getSerialNumber("WrongName");
}
Skipping (for the sake of the article) all tests on invalid systems and other exceptions we can now start writing a more interesting test:
@Test
public void testGetSerialNumberReturnsPositiveNumber() throws Exception {
long result = instance.getSerialNumber(tableName);
assertTrue(result > 0);
}
CTRL + F6, red bar (as expected). This is where the rubber hits the road.

The MYPGM program is contained in the MYLIB library in the QSYS, and it also uses the MYOTHERLIB library (by the way, watch for the maximum length of the names of the libraries, which is 10). The wrapper class for a program call (the Command pattern in action) is ProgramCall (fantasy does not abound in IBM, but that's good as I easily found what I was looking for). It needs to know the system we're operating on, the path to the program and the parameter list. We have the system, so we need the path to the program, for which we use the QSYSObjectPathName class, and the parameter list, for which we use the ProgramParameter class.

The QSYSObjectPathName constructor takes as parameters the name of the library, the name of the program and it extension. Once we have a QSYSObjectPathName object we can ask it the path to the program, which is what precisely what we need:
QSYSObjectPathName pgmName = new QSYSObjectPathName(myLib, myPgm, pgmExtension);
The parameter list is actually an array of ProgramParameter objects, which cannot be null. Each parameter wraps an array of bytes, so we have to use the proper converter; luckily there are some converters ready for us.

Our program has an input/output parameter (the name of the table) and an output parameter (the serial number for the given table), so we have the following:
ProgramParameter[] paramList = new ProgramParameter[2];
The first parameter is the name of the table, which is a String. To deal with it we can use the AS400Text converter:
AS400Text textConverter = new AS400Text(10, system);
byte[] key = textConverter.toBytes(tableName);
paramList[0] = new ProgramParameter(key);
The first parameter for the constructor is the length of the IBM i text, the second one is the system we're operating on.

The output parameter is a long, so we'll have to use a different converter. Up to now let's just initialize it:
paramList[1] = new ProgramParameter(32);
Our command is now ready to be born:
ProgramCall pgm = new ProgramCall(system, pgmName.getPath(), paramList);
To execute it we simply call the run method, which returns a boolean. If the result is true we can extract data from the output parameter and convert it:
byte[] data = paramList[1].getOutputData();
AS400PackedDecimal pdconverter = new AS400PackedDecimal(12, 0);
long result = ((BigDecimal) pdconverter.toObject(data)).longValue();
And this is it. Let's see:
Testcase: testGetSerialNumberReturnsPositiveNumber(testserialias.MyIseriesSerialsServiceTest):
Caused an ERROR
6
Wow that's illuminating... Luckily we aldready know what's going on: if you check above you'll notice I wrote that the program needs another library, which is not loaded when we first connect to the system. To add the library we need to issue a command, thus we use the CommandCall class:
CommandCall cc = new CommandCall(system);
cc.setCommand("ADDLIBLE " + myotherLib);
cc.run();
Now we're going somewhere... As we trust but also want to control we add another simple test to check that the service returns bigger numbers on consecutive calls:
@Test
public void testGetSerialNumberReturnsBiggerNumbersOnFurtherCalls() {
firstResult = instance.getSerialNumber(tableName);
long secondResult = instance.getSerialNumber(tableName);
assertTrue(firstResult < secondResult);
}
If we want to have more informations we can ask the program wrapper for an array of AS400Message objects:
private String buildMessage(final ProgramCall pgm) {
AS400Message[] messageList = pgm.getMessageList();
String message = "";
for (int i = 0; i < messageList.length; i++) {
AS400Message aS400Message = messageList[i];
message += aS400Message.getText();
message += "\r\n";
}
return message;
}
The RPGLE program already ensures that no serial can be duplicated, but I thought that adding a little bit of safety wouldn't be too much damage, so the main call is synchronized. The final draft:
import com.ibm.as400.access.AS400;
import com.ibm.as400.access.AS400Message;
import com.ibm.as400.access.AS400PackedDecimal;
import com.ibm.as400.access.AS400Text;
import com.ibm.as400.access.CommandCall;
import com.ibm.as400.access.ProgramCall;
import com.ibm.as400.access.ProgramParameter;
import com.ibm.as400.access.QSYSObjectPathName;
import java.math.BigDecimal;
import java.util.logging.Level;
import java.util.logging.Logger;

public class MyIseriesSerialsService implements SerialsService {

private final String myLib = "MYLIB";
private final String myOtherLib = "MYOTHERLIB";
private final String myPgm = "MYPGM";
private final String pgmExtension = "PGM";
private final AS400 system;
private QSYSObjectPathName pgmName;

public MyIseriesSerialsService(final AS400 system) {
this.system = system;
addLibraries();
createPathName();
}

public long getSerialNumber(final String tableName) {
synchronized (this) {
ProgramParameter[] paramList = createInputParameters(tableName);
ProgramCall pgm = createProgramCall(paramList);

try {
pgm.run();
} catch (Exception ex) {
Logger.getLogger(MyIseriesSerialsService.class.getName()).
log(Level.SEVERE, ex.getMessage(), ex);
String errorMessage = buildMessage(pgm);
throw new RuntimeException(errorMessage, ex);
}

long result = extractResult(paramList);

return result;
}
}

private void addLibraries() {
try {
CommandCall cc = new CommandCall(system);
cc.setCommand("ADDLIBLE " + myOtherLib);
cc.run();
} catch (Exception ex) {
Logger.getLogger(MyIseriesSerialsService.class.getName()).
log(Level.SEVERE, ex.getMessage, ex);
throw new RuntimeException("Unable to inizialize service", ex);
}
}

private void createPathName() {
pgmName = new QSYSObjectPathName(myLib, myPgm, pgmExtension);
}

private ProgramParameter[] createInputParameters(final String tableName) {
ProgramParameter[] paramList = new ProgramParameter[2];
paramList[0] = createInputParameter(tableName);
paramList[1] = new ProgramParameter(32);
return paramList;
}

private ProgramParameter createInputParameter(final String tableName) {
AS400Text textConverter = new AS400Text(10, system);
byte[] key = textConverter.toBytes(tableName);
return new ProgramParameter(key);
}

private ProgramCall createProgramCall(final ProgramParameter[] paramList) {
return new ProgramCall(system, pgmName.getPath(), paramList);
}

private long extractResult(final ProgramParameter[] paramList) {
byte[] data = paramList[1].getOutputData();
AS400PackedDecimal pdconverter = new AS400PackedDecimal(12, 0);
return ((BigDecimal) pdconverter.toObject(data)).longValue();
}

private String buildMessage(final ProgramCall pgm) {
AS400Message[] messageList = pgm.getMessageList();
String message = "";
for (int i = 0; i < messageList.length; i++) {
AS400Message aS400Message = messageList[i];
message += aS400Message.getText();
message += "\r\n";
}
return message;
}
}
And the test class which assisted me in all the small refactorings:
import com.ibm.as400.access.AS400;
import org.junit.After;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
import static org.junit.Assert.*;

public class MyIseriesSerialsServiceTest {

private final String host = "myVeryExpensiveHost";
private final String username = "myUsername";
private final String password = "myPassword";
private final String tableName = "myTable";
//
private MyIseriesSerialsService instance;

@Before
public void setUp() {
instance = new MyIseriesSerialsService(createValidSystem());
}
private AS400 createValidSystem() {
return new AS400(host, username, password);
}

@Test
public void testConnection() throws Exception {
AS400 system = new AS400(host, username, password);
assertNotNull(system.getRelease());
}

@Test(expected = IllegalArgumentException.class)
public void testWithWrongTableNameShouldThrowException() {
instance.getSerialNumber("WrongName");
}

@Test
public void testGetSerialNumberReturnsPositiveNumber() throws Exception {
long result = instance.getSerialNumber(tableName);
assertTrue(result > 0);
}

@Test
public void testGetSerialNumberReturnsBiggerNumbersOnFurtherCalls() {
long firstResult = instance.getSerialNumber(tableName);
long secondResult = instance.getSerialNumber(tableName);
assertTrue(firstResult < secondResult);
}
}
As I said, this is but a draft, and could be improved in many ways, e.g. the tests are quite coarse and don't consider all the small things that could go wrong, some of which I discovered with a quick debugging while I was setting up the tests. Calling a system.disconnectAllServices() when you finish would not be bad either. Yet, it is easily readable and hopefully understandable, so I hope this will help everyone to get rid of the extra stack required by the application server when it is not needed (does that ring a bell?).

Thursday, March 3, 2011

About planning and emergency

The title was inspired by Davide Bianchi, that posted out of his office a sign that reads "Pianificazione schifosa dalla tua parte non è emergenza dalla mia parte", which more or less means "if you don't know what planning is about, don't come here complaining, because I have my own priorities". Or, to quote something a little farther on the timeline, "frankly, my dear, I don't give a damn".


It is true that no plan survives the contact with the enemy, but planning is essential. It is one of the core practices of risk management, which, as Tom DeMarco and Timothy Lister write in Waltzing with Bears, is "Project Management for Adults". What are the risks? You have to look ahead, and decide how you want to deal with them. Of course hoping for luck is an option... but how good is it? Sometimes you can count on it, maybe because even if your luck abandons you it'll affect but a small portion of the whole project. But, honestly, do you really think that none of those twenty or thirty bad events is actually going to happen? Do you know what the odds are for a chance like this? I was pretty good at statistics, and as far as I can remember the answer is "very, very, very low".

And this is when emergency comes into play. And, obviously, as there were no risk mitigation plans, there are no contingency plans, and your luck is having a holiday, there is a problem. As strange as it may seem, this comes as a complete surprise to many.

When planning fails (if ever there was any) the hunting season for scapegoats open. Little matters the fact that the scapegoats themselves have spent a good part of the previous months asking for a plan, or at least for directions, and trying to raise alarms of all kinds. Don't think scapegoating is something you can ad-lib, it is an art: there is also an article about the art of scapegoating in IT projects. Wikipedia reports that "Scapegoating is a known practice in management where a lower staff employee is blamed for the mistakes of senior executives. This is often due to lack of accountability in upper management."

Unlike as in Davide's office, in my experience bad planning by someone else does transform into an emergency happily hopping and bouncing on your desk. At this point, at least in the eyes of the stakeholder, quality becomes absolutely tradable, because the deadline has stopped soaring and is swooping down on them. Too bad, as what seems a saving today will cost blood and sweat tomorrow (not in a month, but within a week). Ok, maybe not blood. But I can guarantee for sweat - and money. Moreover

poor Martin Fowler
will feel so sad for all that
which is way too bad

featuring both a haiku and a link to an interesting article at the same time.