Bad Java code in 7 easy steps
There are many books about how to write good programming code in Java, e.g. Bloch(2008): Effective Java or Martin (2009): Clean Code. However, much less attention is paid in the literature to the art of degrading good source code. Yet anyone can learn this skill with a little practice. In fact, usually only very few steps are necessary, as the following example will demonstrate.
We start with some example code, which is commonly recommended in the books mentioned above
public class DBWrapper {
DB db;
public DBWrapper(DB db){
this.db = db;
}
public final List<Product> search(final String name) {
checkNotNull(name, "Name is null!");
final List<Product> products = db.findAll(name);
if(products == null) {
return Collections.emptyList();
}
return products;
}
private Object checkNotNull(final Object o, String message) {
if(o == null) {
throw new IllegalArgumentException(message);
}
return o;
}
}The DBWrapper class is a wrapper (also called decorator) around a database class. Our attention is focused on the search() method. It finds all products with a given name. The call looks e.g. like this
Product p1 = new Product(“name1”);
Product p2 = new Product(“name2”);
Product p3 = new Product(“name3”);
dbWrapper = new DBWrapper(new DB(p1,p2,p3));
List<Product> results = dbWrapper.search(“name3”);
assertTrue(results.get(0).getName().equals(“name3”));The method has some unpleasant properties. For example, it throws an IllegalArgumentException when passed a nullvalue and never returns nullitself. It is an ideal candidate to learn the art of source code degradation (also called rerefactoring).
We start with
1. Accepting null
public class DBWrapper {
DB db;
public DBWrapper(DB db){
this.db = db;
}
public final List<Product> search(final String name) {
final List<Product> products = db.findAll(name);
if(products == null) {
return Collections.emptyList();
}
return products;
}
}As we can see, by deleting the superfluous nullcheck we have already saved a considerable number of lines of code. This was a good start, our method will now throw a reasonable NullPointerExceptionwhen a nullvalue is passed.
2. Returning null
public class DBWrapper {
DB db;
public DBWrapper(DB db){
this.db = db;
}
public final List<Product> search(final String name) {
return db.findAll(name);
}
}The db.findAll() method (like many incorrectly implemented database APIs) returns null if no search hit can be found. Our DBWrapper should not try to improve this either. Doing that we can save many lines again and we have now already shortened the method code to one line. The beginner may already be satisfied with the result at this point, but we are just starting.
3. Removing final
public class DBWrapper {
DB db;
public DBWrapper(DB db){
this.db = db;
}
public List<Product> search(String name) {
return db.findAll(name);
}
}Every modern developer works in an agile way. In such an environment, the keyword final is just annoying, as it prevents overwriting the method in the derived class for methods and resetting the value for variables. This lack of flexibility is disruptive to the agile development process and must therefore be considered obsolete.
4. Removing of Generics
public class DBWrapper {
DB db;
public DBWrapper(DB db){
this.db = db;
}
public List search(String name) {
return db.findAll(name);
}
}As every developer knows, generics are removed by the compiler anyway. So why keep them in the source code? Without generics, our code has become even shorter and more flexible. Now our list can contain not only objects of the class Product but also e.g. of the class Person or simply strings.
5. Using the base class Object
public class DBWrapper {
DB db;
public DBWrapper(DB db){
this.db = db;
}
public List search(Object name) {
return db.findAll((String) name);
}
}For the agile developer, type safety in a programming language is only a hindrance. But Java offers a way out: the base class Object from which all other classes are derived. To make our search() method as flexible as possible, it makes sense to use Object as the method parameter instead of String. In our case we have to introduce a cast before calling findAll(), but explicit casts are a must for a successful rerefactoring.
6. Dependency Injection into the method
public class DBWrapper {
public static List search(Object s, DB db) {
return db.findAll((String) name);
}
}Dependency injection via the constructor is familiar to everyone. But the clarity of this approach is not helpful for our goals. And since our method has too few parameters anyway, it makes sense to pass the dependency to the database directly to the method. In addition, we save the class variable db and the constructor, because we can also make the method static. The god of performance will love us for this.
7. Returning values by modifying method parameters and using void
public class DBWrapper {
public static void search(Object s, DB db, List results) {
results.addAll(db.findAll((String) s));
}
}As a final highlight, we take advantage of the fact that in Java, when an object is passed to a method, the pointer to that object is copied (see http://javadude.com/articles/passbyvalue.htm). This allows us to pass an object as a parameter to the method and modify that object using the pointer copy within the method. Return values are then no longer needed and we can declare our method as a void method. Methods with surprising side effects like this are what separates the professional code-deteriorator from the novice. If you want to be absolutely sure, you can hide the object that changes in the method somewhere in the middle of the parameter list. And remember: a pro keeps any existing comments untouched when changing the parameter order. Not that someone else uses the method correctly at the first go.
After our 7 simple steps, we have reached our goal. Our code is much shorter due to the rerefactoring. But for that the method call has become much more obscure.
Product p1 = new Product("name1");
Product p2 = new Product("name2");
Product p3 = new Product("name3");
List results = new ArrayList<>();
DBUtil.search("name3", new DB(p1,p2,p3), results);
assertTrue(((Product) results.get(0)).getName().equals("name3"));In addition, our method now accepts arbitrary objects when compiling and also null as query parameter, which should be good for some surprising RuntimeExceptions. In case the search returns no results, the method crashes with a nice NullPointerException (thanks to the call to List.addAll(null)).
We can see that the art of code degradation or rerefactoring is not that difficult, if you only dare to do it. We hope that we have been able to give the interested reader some good basics to get started in this fascinating area of programming and hope you enjoy successfully applying our seven tips in practice.
PS If you need even more inspiration, check out the reference on the topic
