Create a gist now

Instantly share code, notes, and snippets.

What would you like to do?
Something I wrote back in 2008 when I was dealing with a codebase that was driving me nuts.

Bush league is a general term used to describe an action or thing as being amateur, inferior or crude. In a literal sense, it refers to a low quality minor-league in baseball not associated with any of the major league teams. The term originated from the state of minor-league fields that often were ringed with shrubs and bushes.

- Wikipedia

Secretly using StringBuffer to convert a number to a String

int value = getValue();
String valStr = value + "";

// Eh, I was too lazy to type Integer.toString(value)

In addition to obfuscating the intent of the code, this results in unnecessary object creation. While modern JVMs lessen the expense of unnecessary object creation, it's not zero.

Hardcoding Filepaths

// TODO: make this a property or something
File dataFile = new File("/home/joedeveloper/projects/testdata.dat");

Hard-coding file paths is okay one-off toy applications, but not production-ready systems. At a minimum, you shouldn't require your fellow developers to edit source code just to get an application up and running. And you absolutely should not require a new binary release to support a file system change.

At least, make the path configurable via a command-line Java system property (i.e. -Dmyapp.datafile=/home/joedeveloper/testdata.dat).

Fighting Java's Lexical Scoping

ProtocolMessge msg = null;

if (type == ORDER) {
    msg = buildOrderMessage();
} else if (type == CANCEL) {
    msg = buildCancelMessage();
}

// might be null...but that's somebody else's problem!
return msg;

If you don't have a better solution than pre-initializing a variable to null, you probably don't understand the code. Also, you should strive not to return null from methods (aka "passing the buck") - unless your application is littered with defensive null checks, you're just asking for a NullPointerException.

Exception for Exceptions

Sometimes you need to initialize the variable so you can handle it in a finally block, i.e. closing output streams that have thrown an Exception. See Sun Java Tutorial example.

String Concatenation in a Tight Loop

String zeroPaddedNum = num + "";
for (int i = 0; i < leadingZeros; i++) {

    zeroPaddedNum = "0" + num;    

}

// I hope the young generation GC isn't already full!
// You get that GC memory and CPU cycles for free anyway...AMIRITE?

Same as above - the cost of unnecessary object creation may have been minimized in recent releases of Java, but it's still not zero. Also, java.text.NumberFormat solves this particular problem a lot more succinctly.

See:

java.net: "Oh, go ahead. Prematurely optimize"

Collaborator class and all its member methods are public

Ok, this is kind of nitpicking, but if a class isn't used outside its package, it and all its member methods should be "package-private" (aka default access).

Making everything public is like inviting somebody else to mis-use your class a year from now. The only classes that should be able to see it are the classes that use it.

public HelperClass {

   public void doSomethingImportant() {
      // warning: this kills performance for a few seconds
   }

   public void reset() {
      // reload persistence...
   }

   public void reloadConfig() {
      // etc...
   }

}

Non-final private static member fields accessed from non-static class instances

public class AppStats {
    
    // why oh why can't java have a "global" keyword
    private static int counter;
    
    public AppStats {
        // re-read counter from persist
        counter = recoverCounterFromPersist();
    }

    public void count() {
        counter++;
    }
}

This is actually a version of Singleton Simpleton pattern. Makes code difficult to follow and prone to concurrency issues (count() needs to be synchronized).

Singletons with an init method

public class MySingleton {

   private static String value;

   /** Make sure to call this first before using this singleton! */
   public static void init() {
       value = loadValueFromFile();   
   }
   
   public static String getValue() {
        return value;
   }

   private static loadValueFromFile() {
      // imagine file IO code here...
   }

}

Again, difficult to follow and error prone. What if init() gets called multiple times from multiple threads? Eliminating Singletons from your code and using dependency injection can prevent these errors from even being a possibility.

Returning a "Magic Number" from a Method to Indicate an Exceptional Condition

It's 2008 2013 and people are still indicating exceptional conditions in Java with "return codes" instead of...throwing an exception.

Why?!?

private double calcualteOrderValue(String symbol, int quantity) {
        
    Double price = symbolToPriceMap.get(symbol);
    
    if(price == null) {
        return -1;
    } else {
        return price * quantity;
    }       
}

If you don't feel comfortable throwing an exception from a method, at least have the method return an ADT instead of a primitive (i.e. CalculationResult).

Contriving Map Keys with String Concatenation

private void fetchOrder(ProtocolMessage msg) {

     // need to retrieve something from a map...
     String key = msg.getConnectionName() + "~" + msg.getDirection() + "~" + msg.getSessionId() + "~" + msg.getSequenceNumber();
     
     return orderMap.get(key);

}     

This is bug prone due to the possibility of conflicting keys (what if the connection name includes a ~ character)? Bigger problem is the unnecessary string concatenation operation when you could just contrive a key class.

If you don't want to create your own light-weight key class, use Apache Commons Collections: MultiKey

Further Reading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment