The first example deals with unnecessary String manipulation, specifically concatenation and consequent splitting of the same string.
This was the example of code that I came across:
Java
public static final String DELIMITER = "#";
public void doSomething(String context, String propSuffix)
{
String propertyName = context + DELIMITER + propSuffix;
boolean propSet = isPropertySet(propertyName);
/* do something based on the value of the propSet variable */
}
/**
* Check if a property is set with a fallback that strips out the property context
*/
private boolean isPropertySet(String propertyName)
{
String val = getProperty(propertyName);
if (val == null) {
String[] tokens = propertyName.split(DELIMITER);
val = getProperty(tokens[1]);
}
if (val != null) {
return true;
}
return false;
}
private String getProperty(String propertyName)
{
/* code to get a property from a properties file */
}
So lets look at what the code does. The method doSomething() is meant to do something based on whether a particular property is set or not. How the property is retrieved is not important, what's important is the name of the property. In this case the property name is composed of a context, delimiter string and a property suffix e.g. something like 'MyContext#CoolProperty'.
To work out whether doSomething() needs to do anything, it calls the isPropertySet() method. Simple enough, but there is a catch. The isPropertySet() method first checks if the property is set and if it finds it is not set, it strips out the context and tries to see if the property is set using the suffix only.
This is where it gets bad. In order to get the property name that doesn't include the context, the code calls the String.split() method using the delimiter value used earlier. Effectively this undoes the concatenation that was done on line 5.
Why is that bad? Simply because it's a waste of resources in terms of memory and processing. Remember that String.split() performs a regex operation and in this case creates an array containing two strings. This has two issues, 1 - regex is an expensive operation and 2 - an extra two Strings are created in memory. This may not be much for a single call of this method but if it's invoked on a regular basis, it can quickly add up. This can have the effect of utilising extra CPU cycles and eating into the available memory. More importantly this is an anti-pattern that creates a String out of substrings only to split it into substrings at a later stage.
The solution is simple, instead of creating the property name inside doSomething(), I pass the context and suffix values to the isPropertySet() method which takes care of the concatenation and at the same time has the necessary components of the property so it doesn't have to split it again.
This what what the code becomes...
Java
public static final String DELIMITER = "#";
public void doSomething(String context, String propSuffix)
{
boolean propSet = isPropertySet(context, propSuffix);
/* do something based on the value of the propSet variable */
}
/**
* Check if a property is set with a fallback that strips out the property context
*/
private boolean isPropertySet(String context, String propSuffix)
{
String val = getProperty(context + DELIMITER + propSuffix);
if (val == null) {
val = getProperty(propSuffix);
}
if (val != null) {
return true;
}
return false;
}
That's much cleaner and more efficient! The updated code is much easier to follow too since it makes it absolutely explicit in what it is doing and how the fallback mechanism works.
If you have an example of bad code practices to share, please send them in and I'll be happy to write about them!
All code examples shown above have been obfuscated from their original versions but still show similarities in terms of style.
-i
