Core Java

Common code violations in Java

At work, recently I did a code cleanup of an existing Java project. After that exercise, I could see a common set of code violations that occur again and again in the code. So, I came up with a list of such common violations and shared it with my peers so that an awareness would help to improve the code quality and maintainability. I’m sharing the list here to a bigger audience.

The list is not in any particular order and all derived from the rules enforced by code quality tools such as CheckStyle, FindBugs and PMD.

Here we go!

Format source code and Organize imports in Eclipse:

Eclipse provides the option to auto-format the source code and organize the imports (thereby removing unused ones). You can use the following shortcut keys to invoke these functions.

  • Ctrl + Shift + F – Formats the source code.
  • Ctrl + Shift + O – Organizes the imports and removes the unused ones.

Instead of you manually invoking these two functions, you can tell Eclipse to auto-format and auto-organize whenever you save a file. To do this, in Eclipse, go to Window -> Preferences -> Java -> Editor -> Save Actions and then enable Perform the selected actions on save and check Format source code + Organize imports. Avoid multiple returns (exit points) in methods:

In your methods, make sure that you have only one exit point. Do not use returns in more than one places in a method body.

For example, the below code is NOT RECOMMENDED because it has more then one exit points (return statements).

private boolean isEligible(int age){
  if(age > 18){
    return true;
  }else{
    return false;
  }
}

The above code can be rewritten like this (of course, the below code can be still improved, but that’ll be later).

private boolean isEligible(int age){
  boolean result;
  if(age > 18){
    result = true;
  }else{
    result = false;
  }
  return result;
}


Simplify if-else methods:

We write several utility methods that takes a parameter, checks for some conditions and returns a value based on the condition. For example, consider the isEligible method that you just saw in the previous point.

private boolean isEligible(int age){
  boolean result;
  if(age > 18){
    result = true;
  }else{
    result = false;
  }
  return result;
}

The entire method can be re-written as a single return statement as below.

private boolean isEligible(int age){
  return age > 18;
}


Do not create new instances of Boolean, Integer or String:

Avoid creating new instances of Boolean, Integer, String etc. For example, instead of using new Boolean(true), use Boolean.valueOf(true). The later statement has the same effect of the former one but it has improved performance.

Use curly braces around block statements.

Never forget to use curly braces around block level statements such as if, for, while. This reduces the ambiguity of your code and avoids the chances of introducing a new bug when you modify the block level statement.

NOT RECOMMENDED

if(age > 18)
  result = true;
else
  result = false;

RECOMMENDED

if(age > 18){
  result = true;
}else{
  result = false;
}


Mark method parameters as final, wherever applicable:

Always mark the method parameters as final wherever applicable. If you do so, when you accidentally modify the value of the parameter, you’ll get a compiler warning. Also, it makes the compiler to optimize the byte code in a better way.

RECOMMENDED

private boolean isEligible(final int age){ ... }


Name public static final fields in UPPERCASE:

Always name the public static final fields (also known as Constants) in UPPERCASE. This lets you to easily differentiate constant fields from the local variables.

NOT RECOMMENDED
public static final String testAccountNo = '12345678';

RECOMMENDED
public static final String TEST_ACCOUNT_NO = '12345678';,

Combine multiple if statements into one:

Wherever possible, try to combine multiple if statements into single one.

For example, the below code;

if(age > 18){
  if( voted == false){
    // eligible to vote.
  }
}

can be combined into single if statements, as:

if(age > 18 && !voted){
  // eligible to vote
}


switch should have default:

Always add a default case for the switch statements.

Avoid duplicate string literals, instead create a constant:

If you have to use a string in several places, avoid using it as a literal. Instead create a String constant and use it.

For example, from the below code,

private void someMethod(){
  logger.log('My Application' + e);
  ....
  ....
  logger.log('My Application' + f);
}

The string literal “My Application” can be made as an Constant and used in the code.

public static final String MY_APP = 'My Application';

private void someMethod(){
  logger.log(MY_APP + e);
  ....
  ....
  logger.log(MY_APP + f);
}


Additional Resources:

Reference: Common code violations in Java from our JCG partner Veera Sundar at the Veera Sundar blog.

Subscribe
Notify of
guest

This site uses Akismet to reduce spam. Learn how your comment data is processed.

11 Comments
Oldest
Newest Most Voted
Inline Feedbacks
View all comments
Javi
11 years ago

I’m disagree with the last rule, If you use a duplicated String, JVM only create 1 object…. I think that the code isn’t better.

Daniel Pedroza
11 years ago
Reply to  Javi

I kinda agree with that last one, yes, it only creates 1 object, but it’s better to create it as a constant for future modifications on that string, or identify what that string is for

Francois Montmasson
Francois Montmasson
11 years ago

NOT RECOMMENDED
if(age > 18)
result = true;
else
result = false;
RECOMMENDED
boolean result= age > 18;

David Ha
David Ha
11 years ago

I don’t mind multiple returns since it means I have _less_ code to read. E.g.

if (condition 1) {
throw …;
}

// No else block required.

if (condition 2) {

// Stop reading here. No need to check whether a “result”
// variable is subsequently modified.

return …;
}

// No else block required.

return …;

Daniel Pedroza
11 years ago

I disagree with multiple return statements, they’re very useful, I use them often when I got what I wanted and the next code is not needed anymore

Valentin Baca
Valentin Baca
11 years ago

While it mark too may lines in a legacy codebase, I find it incredibly helpful to turn on more warnings under the “Unnecessary Code” section of Eclipse’s Preferences > Java > Compiler > Errors/Warnings.

Also, word of warning to adjust the settings for formatting comments. Comments should almost always be left exactly as-is, but if Eclipse is allowed to format comments it can mangle them.
In other words, spot test before just applying to your entire workspace.

mike
mike
11 years ago

If you dont return early your methods have the worst case running time in every case.

Edinei
Edinei
11 years ago

I disagree that there shoud be one exit point. It seems so procedural style. Martin Fowler says to remove control flag and use return instead in his refactoring book. http://sourcemaking.com/refactoring/remove-control-flag

Guest
Guest
10 years ago

If a method is long, multiple return statements will make it hard to understand what the method does. Using only one exit point will make the method easier to understand. I think this the reason for the “don’t use multiple return statements” rule. But the method is still long and therefore probably mixes different levels of abstraction, and today a good method should be short and only handle one level of abstraction. Because of this, you should refactor such a method to make it shorter (e. g. extract other methods). Then the method is so easy a single return statement… Read more »

Green Lightning
10 years ago

If a method is long, multiple return statements will make it hard to understand what the method does. Using only one exit point will make the method easier to understand. I think this the reason for the “don’t use multiple return statements” rule. But the method is still long and therefore probably mixes different levels of abstraction, and today a good method should be short and only handle one level of abstraction. Because of this, you should refactor such a method to make it shorter (e. g. extract other methods). Then the method is so easy a single return statement… Read more »

Green Lightning
10 years ago

Instead of using

new Boolean(true);

or using

Boolean.valueOf(true);

you could also use

Boolean.TRUE;

Back to top button