During my professional and private life as a SW developer, I've seen a lot of code. From time to time, I see something of which I really think WTF. Some of these have been collected, sometimes redacted a bit but clearly, the point should be obvious.
Java Project name
ServerCodeAsOn1Juni2010
- Use SVN for versionaing
- project names should be lower-case sperated by dashes
Unreachable code, DRY, hardcoded values
if(result > 60)
{
writeLine("Duration: " + result/60000 + " minutes");
}
else if(result > 3600){
writeLine("Duration: " + result/360000 + " hours");
}
else{
writeLine("Duration: " + result/1000 + " seconds");
}
- 360.000 is not an hour in milliseconds, it should be 3.600.000
- Second if-clause is not reachable, because the first one would match
- All numbers are hard-coded which directly lead to the next issue, that would have been prevented by using constants in the first place:
- Assuming
resultis in milliseconds,if(result > 60)should actually beif(result > 6000). Same goes for the second clause. - Don't repeat yourself:
Duration:andwriteLineshould be used / called only once.
Possible corrected version
private String millisToHumanReadableString(long millis) {
final long HOUR_IN_MILLIS = 3600000;
final long MINUTE_IN_MILLIS = 60000;
final long SECONDS_IN_MILLIS = 1000;
long duration;
String dimension;
if(millis > HOUR_IN_MILLIS){
duration = millis / HOUR_IN_MILLIS;
dimension = "hours";
}
else if(millis > MINUTE_IN_MILLIS)
{
duration = millis / MINUTE_IN_MILLIS;
dimension = "minutes";
}
else {
duration = millis / SECONDS_IN_MILLIS;
dimension = "seconds";
}
return duration + " " + dimension;
}
(...)
writeLine("Duration: " + millisToHumanReadableString(result);
Stupid & misleading exceptions
public class NullArgumentException extends RuntimeException {
public NullArgumentException(String msg) {
super(msg);
}
}
String[] values = command.getValues();
if (values == null) {
throw new NullArgumentException("passed null values");
}
- This has the same effect as directly accessing a null object
- Better use IllegalArgumentException, if necessary
Comments & Variables
Remove commented out lines when checking in. Don't change the meaning of variables
public boolean checkAccount(String account, String token) {
logger.debug("validating account");
AndFilter filter = new AndFilter();
String user = account;
String pass = token;
// SearchControls searchControl = new SearchControls();
// searchControl.setSearchScope(SearchControls.ONELEVEL_SCOPE);
filter.and(new EqualsFilter("objectclass", "device")).and(new EqualsFilter("cn", user));
return ldapTemplate.authenticate(DistinguishedName.EMPTY_PATH, filter.toString(), pass);
// Account account = null;
// try {
// account = findByPrimaryKey(accountName);
// System.out.println("in try");
// } catch (NameNotFoundException e) {
// System.out.println("in catch");
// return false;
// }
// System.out.println("finally"+account.getSecurityToken());
// return account.getSecurityToken().equals(securityToken);
}
- Remove unnecessary comments
- Don't change the meaning from account & token to user & pass
Misleading method names & reflection fuck-up
private void parse(String xml) {
Map helper = new HashMap();
helper.put(HEADER_COMMAND, Command.class);
Object translatedObject = manager.manage(translatorClassName, xml, helper);
if (translatedObject instanceof Command) {
Command command = (Command) translatedObject;
long id = command.getId();
long timeout = command.getDelay();
String[] values = command.getValues();
if (id != myID) {
return;
}
if (values == null) {
throw new NullArgumentException("passed null values");
}
String configurationID = values[0];
String queueName = configurationID + outQueue;
myParameterValues[0] = queueName;
task = new TimerTask(messageConsumerManager, this.myClass, myMethod, myParameterTypes, myParameterValues);
timer = new Timer(task, timeout);
} else {
logger.info("not my type");
}
}
- Yes, NullArgumentException again -.-
- method name suggests, guess what, parsing
- It actually picking some values from an object and starts a new thread/task
- Don't use reflection when not necessary. Here the class is calling ITSELF with full knowledge about the internals but using a reflection mechanism that destroys refactoring and tracibility (find usages).
Unclear comments, bad naming, missing abstraction
o(2) = 8 ' set position transducer to 2 - GET_Position results in 0
ois a byte-array andodoesn't say whats the meaning of the byte-array. It's probably an extreme abbreviation ofoutputData.- Not clear what 8 actually means, should be at least a reasonably named constant, or even better hidden behind a method
- The comment makes it even worse: It says something has been set to 2, but what we did was setting it to 8 And the result is 0. Why? And what does 0 mean in this context
Use exceptions instead of return values
For cLoop As Integer = 1 To MaxRetries
o(0) = 23
If Send(o, i, 1) Then
If i(0) = 23 Then
If Send(vS, i, 1) Then
If i(0) = 0 Then
CurrentSpeed = SollSpeed
If SollSpeed = 0 Then
If SET_Kommutierung(0) Then
Return True
Else : Return False
End If
Else : Return True
End If
End If
End If
End If
End If
Next
mReady = False
RaiseEvent Msg("ERROR SET_SollSpeed")
- 6 (!!) nested if clauses are unreadable, to put it friendly. Implement the best-case and throw exceptions in case something goes wrong.
Use Version Control
'.Temp3 = System.BitConverter.ToInt16(i, 0) / 32 'changed on 5.6.2012
'.Temp2 = System.BitConverter.ToInt16(i, 2) / 32 'changed on 5.6.2012
.LadProz = i(4)
.Temperatur = System.BitConverter.ToInt16(i, 5) / 32 'changed on 5.6.2012 'again changed on 30-09-2012
Consistent Naming
Use one language, for code and naming. Use consistent and obvious terms. An excerpt from the motor controller interface documentation and its command list:
getfeuchtecmd
sendtpfaktcmd
recparcmd
setstromvzcmd
lueftercmd
- Mixed german
feuchteand englishgetandcmd - Completely messed up terminology:
- This is actually an interface spec to interact with the motor controller. However the prefixes are extremely confusing.
recmeans the controller receives the data andsendmeans the controller sends the data. So from the user's point of view, both terms should be exchanged. - To make it worse,
setandgetbehaves as would naturally expect which means you set a value on the control which is contradicting tosendandrec. - Finally, there are even commands that doesn't have a prefix at all. Using
lueftercmdactually lets you disable or enable the fan.
- This is actually an interface spec to interact with the motor controller. However the prefixes are extremely confusing.
- Generally, the suffix
cmdis completely redundant because ALL 57 available commands are suffixed withcmd.
One last thing, four parameters of the recparcmd, which is used to send the configuration:
speedregpshi
speedregishi
speedregpfakt
speedregifakt
It took me like 3 hours reading the whole documentation as well as numerous articles about control engineering on Wikipedia to figure out what the author actually intended to mean with that naming.
- At first, the mix up German and English again,
speedwhich should actually be rotational speed andregwhich is supposedly mean Regeleung, and should be translated to Control. - OK, so its about Rotational Speed Control of the motor, huh? But we're only half-way...
- So, what about
pshi,ishiand such? Well, in control engineering, there is a special kind of control, the PI-control which has a proportional and an integral component, thus PI. - And last but not least, we have
shiandfakt. This last riddle is solved by learning how the microcontroller stores floating point numbers using a mantissa and an exponent. When multiplying such numbers, the mantissa is used as a factorfakt(Yeah, German again) and the the exponent is being used in a bit-shift operation, thusshi. - So these short names mixes up German, English, control engineering knowledge and low-level implementation details of how floating point values are stored and multiplied.
- Obvious, isn't it?
So a longer but more obvious version might be the following:
SpeedControlProportionalComponentExponent
...
SpeedControlIntegralComponentMantissa
Exception handling by recursion
OK, lets write some funky Indian-style code. Damn, we've got a ConcurrentModificationException. How do we handle it? Concurrency... ah let's put a synchronized block around it. It surely will help. Shit, didn't help... What do we do now? Let's call this method recursively until we don't get the Exception (or probably a stack overflow??).
/**
* Checks if a message consumer listeneing on 'queueName' is available or not.
* @param queueName name of the queue
* @return true or false
*/
private boolean isMessageConsumerAvailable(String queueName) {
// synchronized (messageConsumerList) {
try {
for (int i = 0; i < messageConsumerList.size(); i++) {
if (messageConsumerList.get(i) instanceof EttexActiveMessageConsumer) {
EttexActiveMessageConsumer listener = ((EttexActiveMessageConsumer) messageConsumerList.get(i));
if (listener.getQueueName().equals(queueName)) {
return true;
}
}
}
} catch (ConcurrentModificationException concEx) {
//handle this case, maybe just iterate the collection again
//e.g. return search(list);
logger.info("ConcurrentModificationException handling", concEx);
return isMessageConsumerAvailable(queueName);
}
return false;
}
Redundancy
System...Property...DAO...System..DAO...Factory...System... Wait, let me guess, it has something to do with... ah no, I forgot. What was it again?
/**
* Returns the accessor class of SystemProperty.
* @return instance of SystemPropertyDAO
*/
private static SystemPropertyDAO getSystemPropertyDAO() {
SystemDAOFactory daoFactory = (SystemDAOFactory) DAOFactory.getDAOFactory(DAOFactory.SYSTEM_DAO_FACTORY);
return (SystemPropertyDAO) daoFactory.getBean(SystemDAOFactory.SYSTEM_PROPERTY_DAO);
}
- This insanely redundant code. I have to admit, that Java sometimes forces you to do so, but it's not that bad.
- Some of the code could have been factored out into some private helpers or to the constructor or static initializer, if necessary.
- Probably the whole design should be questioned if it enforces redundancy.
Try to add for-loop wherever possible
int counter = 1; //a.getCount();
String routeName = null;
String processorName = null;
for (int i = 0; i < counter; i++) {
routeName = routeNames.get("1");
processorName = routeName + i;
generateProcessorMap(routeName, processorName);
routeBuilders.add(new Route1MyProtocolToOtherFormatTranslatorToMessageProducer(routeName));
(...)
}
- Very good, this really makes a lot of sense! This is probably some legacy code, but doesn't make sense anymore.
- Clean-up & refactor!
Wasting big time - Unit testing Java interfaces
I've just found a set of test cases that all did not compile. After looking into it, I was shocked. The reason for that was not because these test cases were broken, but because they were a ridiculous waste of time. The developer wrote about 15 test classes to test interfaces. She used PowerMock to create a mock class from the interface and simply tested id the mock was working.
CDD - Compiler Driven Design
After spending a massive amount of time on refactoring and writing test cases for legacy code, my colleague were thinking about that CDD was used by this developer. We were joking that she mutated her code just until it started to compile without knowing what to do and then closed the file and left the mess.
Polymorphism using interfacecs
While cleaning up some legacy code (again), I found this nice little pattern:
public interface Packet {
public void init(Object o1, Object o2, Object o3, Object o4);
public void decode();
}
public class PacketWithPayload implements Packet {
private Payload payload;
public void init(Object o1, Object o2, Object o3, Object o4) {
if(null == o2) {
throw new IllegalArgumentException();
}
this.payload = o2;
}
public void decode() {
/* parsing goes here */
}
}
public class PacketWithError implements Packet {
private int errorCode;
public void init(Object o1, Object o2, Object o3, Object o4) {
if(null == o1) {
throw new IllegalArgumentException();
}
this.errorCode = (Integer)o1;
}
public void decode() {
/* parsing goes here */
}
}
public Packet handlePayload (Payload payload) {
/* got some payload */
Packet packet = new PacketWithPayload();
packet.init("foo", payload, null, null);
packet.decode();
return packet;
}
public Packet handleError (Payload payload, int errorCode) {
/* got some other payload */
Packet packet = new PacketWithError();
packet.init(errorCode, payload, null, null);
packet.decode();
return packet;
}
You get the idea? The developer tried to implement polymorphism by defining a "flexible" interface. Actually, this little pattern contains a lot of anti-patterns:
- There are no types in the
init()method. She usesObjectwhich makes it impossible for the compiler to spot illegal types. - Different implementation use a different number of parameters so there are always some null arguments.
- Some implementations may drop the usage of some parameters whereas the calling part of the code does not known and does not get warned by the compiler (Which was the case in my situation)
- There is always the same sequence of calls: Constructor,
init(),decode(). This should be wrapped in one call.
So the straight-forward solution is obviously using the constructor to do all the work. It supports all we need: We have one single call, there are a multiple number of parameters per implementation allowed and if we have the instance returned by the constructor we're sure, that it is a valid and properly parsed packet:
public interface Packet {
}
public class PacketWithPayload implements Packet {
public PacketWithPayload(Payload payload) {
decode(payload);
}
private void decode() {
/* parsing goes here */
}
}
public class PacketWithError implements Packet {
private int errorCode;
public PacketWithError(int errorCode) {
this.errorCode = errorCode;
}
}
public Packet handlePayload (Payload payload) {
return new PacketWithPayload(payload);
}
public Packet handleError (int errorCode) {
return new PacketWithError(errorCode);
}
Nice, isn't it? Less code, type safety and polymorphism.
Use meaningless names and NEVER use the type system
public interface Manager {
/**
* The method delegates the request to a service class.
* @param className name of the service class
* @param objectToBeManaged object which has to be managed by the service class
* @return the result after processing the request
*/
Object manage(String className, Object objectToBeManaged);
/**
* The method delegates the request to a service class.
* @param className className name of the service class
* @param objectToBeManaged object which has to be managed by the service class
* @param helper helps in processing the request
* @return the result after processing the request
*/
Object manage(String className, Object objectToBeManaged,
Object helper);
}
Wow, this is a great interface isn't it? I just found it refactoring some old code base and thankfully this interface isn't used anymore. I just wanted to document this ingenuous piece of work. Manager is always a good name to describe something. Actually everything and everybody could be a manager, let's say a facility manager... Ok, sometimes I am tempted to use Manager or Handler but at least it try to find some better domain-specific name.
But this one is really impressive. It uses a String to specify class name (presumably to use reflection, ALWAYS a great idea in Java). Then it receives an Object instance, that could be virtually anything. And optionally, you could also get a helper Object. Obviously, as a result, you'll also get an Object instance.
What could possibly go wrong. If I should implement such an interface I wouldn't know what to do and if I would get an instance of a class, implementing it I won't know what I should expect calling it's methods.
One of the best features of Java is its type system (Not compared to other languages but compared to other Java features) and with generics, you could actually build software that covers a lot of error cause just by using sensible typing.
However, the Manager interface successfully circumvents Java's type system by using Obejcts and reflection.
