Raw code vs. readable code vs. fast code

My usual approach to implementation of some functionality is to capture my mind process in a programming language. After some time of writing, compiling, testing and rewriting I reach the stage where I'm comfortable with it. I usually keep two things in mind: to make the code easily readable and understandable by others; and to make it fast. How you weight these two is up to you.

For example lets have a look at following method implementation:

01 private String getAttribute(String method) {
02   // get past the first 3 chars (ie, "get" or "set").
03   String retVal = method.substring(3);
04   retVal = checkIfPrimaryKey(retVal);
05   if (!"".equals(retVal) && retVal.equals(method.substring(3))) {
06     String firstChar = retVal.substring(0,1).toLowerCase();
07     retVal = firstChar + retVal.substring(1);
08   }
09   return retVal;
10 }
11
12 private String checkIfPrimaryKey(String name) {
13   return "Id".equals(name)? "" : name;
14 }

The code runs and all is OK. All would be fine if I was working on this project by myself, which rarely happens these days. The problems arise when people can not understand some one else's code. We are all different and we think in different ways. The intentions I had were not expressed very well and for some developers it may take a while to understand why I used integer 3 on line 3 and 5 and what are lines 3 to 7 doing.

Let's make it clear! First, let's make the meaning of 3 easy to understand.

01 private String getAttribute(String method) {
02   final int PREFIX_LENGTH = 3; // first 3 chars (ie, "get" or "set")
03   // get past the first 3 chars (ie, "get" or "set").
04   String retVal = method.substring(PREFIX_LENGTH);
05   retVal = checkIfPrimaryKey(retVal);
06   if (!"".equals(retVal) && retVal.equals(method.substring(PREFIX_LENGTH))) {
07     String firstChar = retVal.substring(0,1).toLowerCase();
08     retVal = firstChar + retVal.substring(1);
09   }
10   return retVal;
11 }
12
13 private String checkIfPrimaryKey(String name) {
14   return "Id".equals(name)? "" : name;
15 }

Now it's clear that I'm working with 3-character prefix. PREFIX_LENGTH was also made final in order to avoid its unintentional modification. If you did not notice this before it might be clearer now that PREFIX_LENGTH is used in two places. Furthermore it is used in exactly the same way. This is a small piece of duplicated code and doesn't smell that much. Let's refactor it in order to indicate what it is and also to remove the duplicate method call to retrieve the substring (optimization for speed).

01 private String getAttribute(String method) {
02   final int PREFIX_LENGTH = 3; // first 3 chars (ie, "get" or "set")
03   String attribute = method.substring(PREFIX_LENGTH);
04   String retVal = checkIfPrimaryKey(attribute);
05   if (!"".equals(retVal) && retVal.equals(attribute)) {
06     String firstChar = retVal.substring(0,1).toLowerCase();
07     retVal = firstChar + retVal.substring(1);
08   }
09   return retVal;
10 }
11
12 private String checkIfPrimaryKey(String name) {
13   return "Id".equals(name)? "" : name;
14 }

Notice how the comment disappeared. It was no longer needed as the “good” name of the variable tells us that it holds the core of the method name. Who cares that it is a substring after first three characters.

Sometimes it makes sense to name the return value properly as well. It may not improve readability in small methods as much, but if the method body is long it can be a good idea to call it something meaningful. In this case we can drop “retVal” completely. Just rename all its references to “attribute”.

01 private String getAttribute(String method) {
02   final int PREFIX_LENGTH = 3; // first 3 chars (ie, "get" or "set")
03   String originalAttribute = method.substring(PREFIX_LENGTH);
04   String attribute = checkIfPrimaryKey(attribute);
05   if (!"".equals(attribute) && attribute.equals(originalAttribute)) {
06     String firstChar = attribute.substring(0,1).toLowerCase();
07     attribute = firstChar + attribute.substring(1);
08   }
09   return attribute;
10 }
11
12 private String checkIfPrimaryKey(String name) {
13   return "Id".equals(name)? "" : name;
14 }

Before going any further I want to point out that “checkIfPrimaryKey” method smells. And the smell is quite bad. Not only the method name does not implement what its name indicates but it does not make clear why it returns empty string and then our method has not work around it. Let's fix it.

01 private String getAttribute(String method) {
02   final int PREFIX_LENGTH = 3; // first 3 chars (ie, "get" or "set")
03   String attribute = method.substring(PREFIX_LENGTH);
04   if (!isPrimaryKey(attribute)) {
05     String firstChar = attribute.substring(0,1).toLowerCase();
06     return firstChar + attribute.substring(1);
07   }
08   return "";
09 }
10
11 private boolean isPrimaryKey(String name) {
12   return "Id".equals(name);
13 }

You can also notice that the empty string was taken out from the original checkIfPrimaryKey and introduced in our getAttribute method. You could also question the reason for having a one-line method. The reason is re-use. If you find this type of line reused over and over again it's best to have it as a method. Compilers are quite smart these days and they might inline this code in the places where it is used. So don't worry about the speed. Another good point in this case is that if the method was not private it could provide a good extension point. Subclasses could override this method to do some more sophisticated checks.

Another not very obvious issue is the index 1 in the block of code that changes the first character to lower case. Although its occurrence is in two consecutive lines, it is essentially the same case as was PREFIX_LENGTH. Let's make it a constant named ONE.

01 private String getAttribute(String method) {
02   final int PREFIX_LENGTH = 3; // first 3 chars (ie, "get" or "set")
03   String attribute = method.substring(PREFIX_LENGTH);
04   if (!isPrimaryKey(attribute)) {
05     // change first character to lower-case
06     final int ONE = 1;
07     String firstChar = attribute.substring(0, ONE).toLowerCase();
08     return firstChar + attribute.substring(ONE);
09   }
10   return "";
11 }

Let's refactor this method by pulling lines 5 to 8 out into a new method and replacing them with a call to this new method. This refactoring is called, as you would've guessed, “extract method”. Some clever IDEs can make this task automated. The following screen shots were made from Eclipse.

First select the code you want to extract

Right-click on it and extract the method as show in the menu

01 private String getAttribute(String method) {
02   final int PREFIX_LENGTH = 3; // first 3 chars (ie, "get" or "set")
03   String attribute = method.substring(PREFIX_LENGTH);
04   if (!isPrimaryKey(attribute))) {
05     return lowerFirstChar(attName);
06   }
07   return "";
08 }
09
10 // change first character to lower-case
11 private String lowerFirstChar(String s) {
12   final int ONE = 1;
13   String firstChar = s.substring(0, ONE).toLowerCase();
14   return firstChar + s.substring(ONE);
15 }

Some of us believe that less is better and try to shorten the number of lines as much as possible. The following listing exhibits this approach:

01 private static String getAttribute(String method) {
02   final int PREFIX_LENGTH = 3; // first 3 chars (ie, "get" or "set")
03   String attName = method.substring(PREFIX_LENGTH);
04   return isPrimaryKey(attribute) ? "" : lowerFirstChar(attribute);
05 }

This makes the method shorter, but the code became a little bit less readable and some junior programmers will find this harder to read or understand.

Some final notes

If you are using constants, i.e. variables that are defined final, it is better if you declare them on the class level with appropriate JavaDoc comment, and if only used internally make them private, e.g.

/** method name prefix length, e.g. “get” or “set” */
private static final PREFIX_LENGTH = 3;

This way the code is less cluttered, better documented and only created once (about this I am not sure as it may depend on the compiler). The last one is especially crucial for creation of constants that take a long time to create or consume a lot of resources.

BTW: Did you notice that this method does not account for boolean getters, e.g. "isGreen", where the prefix only takes two characters ("is")?

Conclusion

The are several rules that are worth when programming with clarity and speed in mind:

• “one and only one place”
  means ease of change, testing and debugging

• name matters
  naming conventions and good names need no code comments

• if it's too hard to understand, refactor it to break the complexity down