Cleaning Code Practically Episode 1

8 10 2009

Last Øredev I were at a presentation done by Robert Martin, author of the book Clean Code. I were really impressed by what his message where. So we are trying to follow his word at work and today a topic came up. So a colleague had this code:


public String getRequestString() throws MQException {
String request = "";
request += Helper.leftRightFilled(true, String.valueOf(getSessionNumberAndIncreaseIt()), '0', 3);
request += Helper.leftRightFilled(true, getVersion(), '0', 3);
request += Helper.leftRightFilled(true, getTransaction(), 'X', 4);
request += Helper.leftRightFilled(true, getLanguage(), 'X', 2);
request += Helper.leftRightFilled(true, getCallersPhoneNumber(), '0',16);
request += Helper.leftRightFilled(false, dynamicSettingMQ.getCustomerNumber(), ' ', 16);
request += Helper.leftRightFilled(true, String.valueOf(dynamicSettingMQ.getNumberOfUnsucceededCalls()), '0', 3);
return request;
}

I intermediately though that could be more readable, so I just pushed a lot into functions, and did a little to the helper class:

public String getRequestString() throws MQException {
String request = "";
request += createSessionNumberString();
request += createVersionString();
request += createTransactionString();
request += createLanguageString();
request += createCallersPhoneNumberString();
request += createCustomerNumberString();
request += createNumberOfUnsucceededCallsString();
return request;
}
protected String createCallersPhoneNumberString() throws MQException {
return Helper.leftRightFilled(true, getCallersPhoneNumber(), '0', 16);
}
protected String createCustomerNumberString() throws MQException {
return Helper.leftRightFilled(false, dynamicSettingMQ.getCustomerNumber(), ' ', 16);
}
protected String createLanguageString() throws MQException {
return Helper.leftRightFilled(true, getLanguage(), 'X', 2);
}
protected String createNumberOfUnsucceededCallsString() throws MQException {
return Helper.leftRightFilled(true, dynamicSettingMQ.getNumberOfUnsucceededCalls(), '0', 3);
}
protected String createSessionNumberString() throws MQException {
return Helper.leftRightFilled(true, getSessionNumberAndIncreaseIt(),'0', 3);
}
protected String createTransactionString() throws MQException {
return Helper.leftRightFilled(true, getTransaction(), 'X', 4);
}
protected String createVersionString() throws MQException {
return Helper.leftRightFilled(true, getVersion(), '0', 3);
}
}

At first the colleague thought it was nice, but then remembered he really needed a overview of locations used in the string manipulation. So a thing between the two are needed:

public String getRequestString() throws MQException {
String request = "";
request += createSessionNumberString('0', 3);
request += createVersionString('0', 3);
request += createTransactionString('X', 4);
request += createLanguageString('X', 2);
request += createCallersPhoneNumberString('0', 16);
request += createCustomerNumberString(' ', 16);
request += createNumberOfUnsucceededCallsString('0', 3);
return request;
}
protected String createCallersPhoneNumberString(char fillWith, int length) throws MQException {
return Helper.leftRightFilled(true, getCallersPhoneNumber(), fillWith,length);
}
protected String createCustomerNumberString(char fillWith, int length) throws MQException {
return Helper.leftRightFilled(false, dynamicSettingMQ.getCustomerNumber(), fillWith,length);
}
protected String createLanguageString(char fillWith, int length) throws MQException {
return Helper.leftRightFilled(true, getLanguage(), fillWith,length);
}
protected String createNumberOfUnsucceededCallsString(char fillWith, int length) throws MQException {
return Helper.leftRightFilled(true, dynamicSettingMQ.getNumberOfUnsucceededCalls(), fillWith,length);
}
protected String createSessionNumberString(char fillWith, int length) throws MQException {
return Helper.leftRightFilled(true, getSessionNumberAndIncreaseIt(),fillWith,length);
}
protected String createTransactionString(char fillWith, int length) throws MQException {
return Helper.leftRightFilled(true, getTransaction(), fillWith,length);
}
protected String createVersionString(char fillWith, int length) throws MQException {
return Helper.leftRightFilled(true, getVersion(), fillWith, length);
}

Im not satisfied, but not sure it can be done more clean? And if you are wondering about why we are doing all this string manipulation, we are working with some legacy MQ. If we split up the code without parameters as above we lose the quick overview.. But could the code be simplified even more please come with your suggestion..

Advertisements

Actions

Information

3 responses

8 10 2009
Guðmundur Bjarni

Haven’t written any Java code for almost half a year now, been doing Scala / Python.

My bid for this would be to use the Builder pattern along with a StringBuilder (to be nice to the gc).

I did a rough sketch of this (which I haven’t tried), which you can find at http://friendpaste.com/3kAPhCc7987hSLMNaHiS0k. I suspect that pasting it in this comment would be a bit messy. 🙂

9 10 2009
ninomartinez

While your suggestion does optimize for speed, it does not bring much in terms of readability and overview. We do not have any performance issues on the system. However if one keep saying that then one will probably end up with issues.

Thanks for sharing ideas, the more the better 🙂 I’ll keep the builder pattern in the back of my mind.

29 07 2010
JL

Maybe bit late for that specific problem, but you may find it useful more generally: you may want to consider the nested function pattern, is hard to beat in terms of compactness and readability, only issue is making sure your IDE doesnt spoil it with the wrong formatting:

public String getRequestString() throws MQException
{
return request(sessionNumber(‘0’,3),
version(‘0’,3),
transaction(‘X’,4),
language(‘X’,2),
callersPhoneNumber(‘0’,16),
customerNumber(‘ ‘,16),
numberOfFailedCalls(‘0’,3));
}

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s




%d bloggers like this: