Coding style and standards

I am a big proponent of coding standards, unit testing, and integration testing. It is always cheaper to find a problem before the code is released than once it is in the field.

The team that I work with daily chafed and complained when we imposed code formatting standards, started running static code analysis tools that reported coding inconsistencies, bad practices, and out and out bugs, and required certain levels of coverage before code could be pushed to the repository. That is until we had to loan two team members out to another product for four months where the only standard was there were no standards. When they returned to our project their observation was something like: We hated when the rules were first established but having had to work where each file was different from the next made us appreciate the seeming “rigidness” and are thankful to be back.

Now when standards are discussed it can quickly dissolve into a religious like discussion of my style or likes verses your style or likes.

Personally I have my preferred coding style but I’d rather have some style than no style at all. Two space indention, braces on the same line, Google style, Java style, Tijs’ style, or something else I really don’t care. Let’s pick one and stick to it.

As for static coding rules I personally get a little more rigid. A short list of my current favorites include:

  1. No wildcard imports
  2. Literals in comparisons should be on the left side to avoid null pointer issues
  3. Null checks before instanceof are not needed
  4. Multiple variables in one declaration should not be done
  5. Using redundant field initialization is wrong (e.g. no need to set a String to null or an int to0; that is the default)
  6. Use isEmpty() instead of size == 0 (no need to count). True also for length()
  7. Use single character notation for single character strings (e.g., ‘0’ instead of “0”)
  8. Enforce all uppercase static field naming
  9. Use parameterized log messages instead of concatenation
  10. Instead of empty string concatenation to change datatype use String.valueOf()

There are obviously more static rules but for now I’m looking for feedback from the community as to whether stricter coding standards and testing rules are valued and would be generally supported.

Let the discussion begin!

1 Like

Hi David,

Good suggestion. Let me respond to and add a couple of items to your list.
I agree with your items, except for the following items:

  1. Is this only class field initialization or also variable initialization in a method? I fully agree with class field initialization, but variable initialization inside a method I prefer setting a String explicitly to null.

  2. using isEmpty is a good practice, but to not allow .size() == 0 and .length() == 0 is a bit too rigid to my liking.

  3. Single character strings are not used so often. So only allowing single character notation is again a bit too rigid I think.

A couple of styling items I would like to add:

a. Use 4 space indentation (no tabs). We now have 2 space indentation in most of the code, but before the 6.0.0 release we would like to change this in one go.

b. Use a large maximum line width (like 200). I prefer to make use of the long screen width of most laptops/desktops nowadays.

c. an if statement should be formatted like this:

if (“Flowable”.equals(name)) {
// do something

} else if (“Activiti”.equals(name)) {
// do something

} else {
// do something
}

We should come up with an Eclipse and Intellij file that people can easily import and use.

Best regards,

Tijs

1 Like

Tijs,

Re: Style

I agree with your style choices except for the 200 line width. We internally went back and forth and settled on 160 characters based on screen size, font size (some of our eyes are not as young as they used to be), and IDE window layouts. It seemed like a good comprise and was larger than most existing standards: Google is 100, Maven is 120, Java is 80, etc.

We internally use Gradle and have a target that automatically copies the formatting files into our projects for Eclipse. I am not sure if there is a way to accomplish the same thing in Maven.

The files are part of our repository so IntelliJ users add the Eclipse formatting plugin and point at the same files.

We also like to turn on the “Save actions” in Eclipse so the code is formatted and the imports organized automatically on saving a file. There is a corresponding “Save Actions” plugin in IntelliJ.

The eclipse preferences files we use can be found at:
org.eclipse.jdt.core.prefs
org.eclipse.jdt.ui.prefs

Try them out and see if they meet your requirements.

Tijs,

Re: exceptions

Please see inline comments.

Is this only class field initialization or also variable initialization in a method? I fully agree with class field initialization, but variable initialization inside a method I prefer setting a String explicitly to null.

That is a reasonable preference but it seems a bit inconsistent to me but I can live with it.

using isEmpty is a good practice, but to not allow .size() == 0 and .length() == 0 is a bit too rigid to my liking

I did some greps and found the current usage in the project and is seems like isEmpty() is the favored style:

  • .size() == 0 is used 44 times
  • .length() == 0 is used 96 times
  • .isEmpty() is used 706 times

Single character strings are not used so often. So only allowing single character notation is again a bit too rigid I think.

Again I did some grepping and found that single character notation is used over 700 times while single character strings are used over 1400 times.

As with all standards there will need to be comprise I will attempt to enumerate a more complete list in addition to those listed previously for further review and comment.

Cheers,
David

Here is a more complete list of suggested static code checks. The list may at times seem silly or obvious but I can assure one that all of these have or currently exist in the code base.

I have included all those from the original top ten list even if they are currently under discussion and review.

In no particular order here is more complete list of static code checks and suggested changes:

  1. ‘equals()’ called on Enum value
    Calls that use equals() on Enum constants should be replaced by an identity comparison (==) because two Enum constants are equal only when they have the same identity.

  2. ‘expression.equals(“literal”)’ rather than '“literal”.equals(expression)'
    Calls to .equals() whose arguments are String literals on the right side of the condition should be
    specify the String literals as the target of .equals(), rather than argument, thus minimizing NullPointerExceptions.

  3. Missorted modifiers
    All declarations modifiers should be specified in the canonical preferred order as stated in the Java Language Specification.

  4. Multiple variables on one declaration
    Multiple variables should not be declared in a single declaration; use separated statements.

  5. Redundant field initialization
    Fields that are explicitly initialized to the same value that the JVM would initialize them to by default do not need any initialization.

  6. ‘size() == 0’ replaceable with 'isEmpty()'
    Any .size() or .length() comparisons with a 0 literal which can be replaced with a call to .isEmpty().

  7. Unnecessary ‘null’ check before ‘equals()’ call
    Comparisons to null which are followed by an ‘equals()’ call with a constant argument can be changed so the ‘literal’ is the target of the comparison. For example:

if (s != null && s.equals("literal")) {}

Can be replaced with:

if ("literal".equals(s)) {}
  1. Unnecessary call to 'String.valueOf()'
    Any calls to String.valueOf() used in string concatenations and as arguments to the print() and println() methods of java.io.PrintWriter and java.io.PrintStream, the append() method of java.lang.StringBuilder and java.lang.StringBuffer or the trace(), debug(), info(), warn() and error() methods of org.slf4j.Logger are not needed as the to string will be handled by the underlying library methods.

  2. Unnecessary call to 'toString()'
    Same reason as above.

  3. Unnecessary semicolon
    Remove any unnecessary semicolons, whether between class members, inside block statements, or after
    class definitions.

  4. Duplicate condition on ‘&&’ or '||'
    Remove duplicated branches in && or || expressions. Such constructs almost always represents a typo or cut-and-paste error.

  5. ‘if’ statement with identical branches
    Correct if statements with identical “then” and else branches. Such constructs almost always represents a typo or cut-and-paste error.

  6. Redundant 'if’statement
    Replace if statements which can be simplified to single assignment or return statements.
    For example:

   if (foo()) {
     return true;
   } else {
      return false;
   }

can be simplified to:

   return foo();
  1. Unnecessary ‘null’ check before ‘instanceof’ expression
    Any null check followed by an instanceof check can be removed. Since the instanceof operator always returns false for null, there is no need to have an additional null check.

  2. Unnecessary ‘return’ statement
    Remove any unnecessary return statements at the end of constructors and methods returning void

  3. Unused declaration
    Remove methods or fields in the current scope that are not used or not reachable from entry points.

  4. Variable is assigned to itself
    Find assignments of a variable to itself. This usually represents a typo or cut-and-paste error.

  5. ’ imports
    Replace any import statements which cover entire packages (’
    imports’) with individual import statements.

  6. Import from the same package
    Remove any import statements which refer to the same package as the containing file. Such imports are unnecessary.

  7. ‘java.lang’ import
    Remove any import statements which refer to the java.lang package. Such import statements are unnecessary.

  8. Unused import
    Remove any import statements that are unused.

  9. Missing @Override annotation
    Add missing @Override to any methods which override methods in a superclass but do not have the @java.lang.Override annotation.

  10. Method is identical to its super method
    Find any method with a signature identical to its super method and either has an identical body to the super method or only calls the super method. Such a method is redundant and can be removed.

  11. Unnecessary boxing
    Find “boxing”, e.g. wrapping of primitive values in objects. Boxing is unnecessary under Java 5 and newer, and can be safely removed.

  12. Unnecessary unboxing
    Find “unboxing”, e.g. explicit unwrapping of wrapped primitive values. Unboxing is unnecessary under Java 5 and newer, and can be safely removed.

  13. Non-constant string concatenation as argument to logging call
    Find non-constant string concatenations used as arguments to logging methods. Non-constant concatenations will be evaluated at runtime even when the logging message is not logged; this can negatively impact performance. Replace with a parameterized log message.

  14. Number of placeholders does not match number of arguments in logging call
    Find logging calls where the number of {}-placeholders in the string literal argument does not match the number of other arguments to the logging call. Correct as needed.

  15. Constant naming convention
    Correct any constants whose names do not follow the regular expression pattern: [A-Z][A-Z_\d]*. Static LOGGER constants are an example.

  16. Enumerated class naming convention
    Correct any enumerated classes whose names do not follow the specified regular expression pattern: [A-Z][A-Za-z\d]*.

  17. Enumerated constant naming convention
    Correct any enumerated constant whose names do not follow the regular expression pattern: [A-Z][A-Z_\d]*.

  18. Non-constant field with upper-case name
    Correct any non-static non-final fields whose names are all upper-case. Such fields cause confusion by breaking a common naming convention, and are often the result of developer error.

  19. Call to ‘Arrays.asList()’ with too few arguments
    Replace any calls to Arrays.asList() with zero arguments or only one argument with either a call to Collections.singletonList() or Collections.emptyList() which will save some memory.

  20. Concatenation with empty string
    Replace string concatenations where one of the arguments is the empty string. Such a concatenation is unnecessary and inefficient, particularly when used as an idiom for formatting non-String objects or primitives into Strings. Use String.valueOf() instead.

  21. Manual array copy
    Replace any manual copying of array contents with calls to System.arraycopy().

  22. Manual array to collection copy
    Replace any copying of array contents to a collection where each element is added individually using a for loop with a call to Collection.addAll(Arrays.asList()) or Collections.addAll().

  23. Redundant 'String.toString()'
    Remove any to call toString() on a String object. This is entirely redundant.

  24. Redundant ‘substring(0)’ call
    Remove any call to String.substring() with a constant argument equal to zero. Such calls are completely redundant, and may be removed.

  25. Redundant call to 'String.format()'
    Remove any calls to String.format() where only a format string is provided, but no arguments. Such a call is unnecessary and can be replaced with just the string.

  26. Single character string argument in ‘String.indexOf()’ call
    Find String literals of length one being used as a parameter in String.indexOf() or String.lastIndexOf() calls. These String literals may be replaced by equivalent character literals, gaining some performance enhancement.

  27. Single character string concatenation
    Find String literals of length one being used in concatenation. These literals may be replaced by equivalent character literals, gaining some performance enhancement.

  28. String concatenation as arguments to ‘StringBuffer.append()’ call
    Find String concatenation used as the argument to StringBuffer.append(), StringBuilder.append() or Appendable.append(). Such calls may profitably be turned into chained append calls on the existing StringBuffer/Builder/Appendable, saving the cost of an extra StringBuffer/Builder
    allocation.

  29. 'String.equals("")'
    Replace .equals() being called to compare a String with an empty string with comparing the length to zero as it is normally more efficient.

  30. ‘StringBuffer.toString()’ in concatenation
    Remove StringBuffer.toString() or StringBuilder.toString() in String concatenations. In addition to being confusing, this code performs String allocation and copying, which is unnecessary.

  31. Array comparison using ‘==’ instead of 'Arrays.equals()'
    Replace any use of == or != to test for array equality, with the java.util.Arrays.equals() method.

  32. String comparison using ‘==’, instead of 'equals()'
    Replace any use of == or != to test for String equality with the equals() method.

  33. Redundant type cast
    Remove unnecessary cast expressions.

  34. Mismatched read and write of array
    Find and correct as needed any array fields or variables whose contents are read but not written, or written but not read. Such mismatched reads and writes are pointless, dead or erroneous code.

  35. Explicit type can be replaced with <>
    Correct all new expressions with type arguments which can be replaced with diamond type <>. That is no need to specify the type again.

  36. Misordered ‘assertEquals()’ arguments
    Fix any calls to JUnit assertEquals() which have a non-literal as the expected result argument and a literal as the actual result argument. Such calls will behave fine for assertions which pass, but may give confusing error reports if their expected and actual arguments differ.

  37. JUnit 4 test method in class extending JUnit 3 TestCase
    Find and correct JUnit 4 @Test annotated methods which are located inside a class extending the abstract JUnit 3 class TestCase. Mixing JUnit API’s like this is confusing and can lead to problems running the tests, for example a method annotated with @Ignore won’t be actually ignored if its name starts with test.

  38. Simplifiable JUnit assertions
    Fix any JUnit assert calls which can be replaced by simpler but equivalent calls; examples include:

assertEquals(true, x()) --> assertTrue(x());
assertTrue(y() != null); --> assertNotNull(y());
assertTrue(z == z()); --> assertSame(z, z());
assertTrue(a.equals(a())); --> assertEquals(a, a());
assertTrue(false); --> fail();
assertTrue(z() == false)); --> assertFalse(z());

Hi David,

That looks like a good list to me.

Best regards,

Tijs

Here is a status update on applying the code style outlined above:

  1. ‘equals()’ called on Enum value
    ACTION: changes in PR #102

  2. ‘expression.equals(“literal”)’ rather than '“literal”.equals(expression)'
    ACTION: there are over 300 changes; did not change; being conservative

  3. Missorted modifiers
    ACTION: changes in PR #103

  4. Multiple variables on one declaration
    ACTION: changes in PR #115

  5. Redundant field initialization
    ACTION: changes in PR #116

  6. ‘size() == 0’ replaceable with 'isEmpty()'
    ACTION: there are over 60 changes; did not change as there is some difference of opinion

  7. Unnecessary ‘null’ check before ‘equals()’ call
    ACTION: changes in PR #101

  8. Unnecessary call to 'String.valueOf()'
    ACTION: no changes; no occurrences

  9. Unnecessary call to 'toString()'
    ACTION: no changes; no occurrences

  10. Unnecessary semicolon
    ACTION: changes in PR #100

  11. Duplicate condition on ‘&&’ or '||'
    ACTION: no changes; no occurrences

  12. ‘if’ statement with identical branches
    ACTION: no changes; no occurrences

  13. Redundant ‘if’ statement
    ACTION: changes in PR #104

  14. Unnecessary ‘null’ check before ‘instanceof’ expression
    ACTION: no changes; no occurrences

  15. Unnecessary ‘return’ statement
    ACTION: changes in PR #105

  16. Unused declaration
    ACTION: there are 19 occurrences; decided to be conservative in approach and not make changes

  17. Variable is assigned to itself
    ACTION: no changes; no occurrences

  18. ‘*’ imports
    ACTION: changes in PR #114

  19. Import from the same package
    ACTION: no changes; no occurrences

  20. ‘java.lang’ import
    ACTION: no changes; no occurrences

  21. Unused import
    ACTION: no changes; no occurrences

  22. Missing @Override annotation
    ACTION: found over 6800 occurrences; did not change; size of change an issue

  23. Method is identical to its super method
    ACTION: changes in PR #106

  24. Unnecessary boxing
    ACTION: found over 70 occurrences; did not change; many cases the boxing highlighted the logic

  25. Unnecessary unboxing
    ACTION: no changes; no occurrences

  26. Non-constant string concatenation as argument to logging call
    ACTION: changes in PR #117

  27. Number of placeholders does not match number of arguments in logging call
    ACTION: changes in PR #118

  28. Constant naming convention
    ACTION: found over 250 occurrences; many were ‘log’ or ‘logger’; no change because of size of change

  29. Enumerated class naming convention
    ACTION: found only one occurrence where the name contained ""; did not change_

  30. Enumerated constant naming convention
    ACTION: found 2 files; public classes; being conservative in case used by other code

  31. Non-constant field with upper-case name
    ACTION: found over 160 occurrences; not done; deemed too invasive

  32. Call to ‘Arrays.asList()’ with too few arguments
    ACTION: changes in PR #125

  33. Concatenation with emtpy string #
    ACTION: changes in PR #119

  34. Manual array copy
    ACTION: no changes; no occurrences

  35. Manual array to collection copy
    ACTION: changes in PR #107

  36. Redundant 'String.toString()'
    ACTION: found 2 small occurrences; fixed with final cleanup PR #127

  37. Redundant ‘substring(0)’ call
    ACTION: no changes; no occurrences

  38. Redundant call to 'String.format()'
    ACTION: no changes; no occurrences

  39. Single character string argument in ‘String.indexOf()’ call
    ACTION: not changed per discussion

  40. Single character string concatenation
    ACTION: found over 540 occurrences; not changed per discussion

  41. String concatenation as arguments to ‘StringBuffer.append()’ call
    ACTION: found 2 small occurrences; fixed with final cleanup PR#127

  42. 'String.equals("")'
    ACTION: found 15 occurrences; decided not to replace with isEmpty() per discussion

  43. ‘StringBuffer.toString()’ in concatenation
    ACTION: found 4 occurrences; fixed with final cleanup PR#127

  44. Array comparison using ‘==’ instead of 'Arrays.equals()'
    ACTION: no changes; no occurrences

  45. String comparison using ‘==’, instead of 'equals()'
    ACTION: no changes; no occurrences

  46. Redundant type cast
    ACTION: found over 110 occurrences; deemed too risky; no changes

  47. Mismatched read and write of array
    ACTION: found 2 occurrences that would require possible refactoring; too risky; no changes

  48. Explicit type can be replaced with <>
    ACTION: found over 2500 occurrences; no changes because size of change

  49. Misordered ‘assertEquals()’ arguments
    ACTION: changes in PR #120

  50. JUnit 4 test method in class exending JUnit 3 TestCase
    ACTION: changes in PR #121

  51. Simplifiable JUnit assertions # 108 and #126
    ACTION: changes in PR #108 and #126

If anyone is using IntelliJ and wants to run the code inspection action I used for these change, you can import the inspection template and import it from my GitHub IntelliJ Repostory. The name of the inspection file is Flowable.xml,