My name is Veera. I'm a full stack web developer. I have founded http://www.timethetask.com and currently working on my next product. Veera is a DZone MVB and is not an employee of DZone and has posted 33 posts at DZone. You can read more from them at their website. View Full User Profile

8 Common Code Violations in Java

09.13.2012
| 20731 views |
  • submit to reddit

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)
  return true;
else
  return false;

RECOMMENDED

if(age > 18){
  return true;
}else{
  return 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:

Published at DZone with permission of Veera Sundar, author and DZone MVB.

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)

Comments

Alexey Grigorev replied on Fri, 2012/09/14 - 2:53am

Totally disagree with one single return point. I would prefer a lot of small functions, with some of which having multiple return points like in the example above.

Andreas Radauer replied on Fri, 2012/09/14 - 3:01am

Good examples, but i cannot agree with "Avoid multiple returns (exit points) in methods"!!!

Marcos Antonio replied on Fri, 2012/09/14 - 6:15am

"Avoid multiple returns (exit points) in methods" is totally wrong.

Gundar Abella Posse replied on Fri, 2012/09/14 - 6:25am

For example, instead of using new Boolean(true), use Boolean.valueOf(true)

 

If you know the value of the var is true (or false), you can use Boolean.TRUE (or Boolean.FALSE).If you don't know de value of the var, the Boolean.valueOf(var) is the way to go.

Totally agree with using Integer.valueOf() and String.valueOf() instead of new.

Jeroen Van Dijk replied on Fri, 2012/09/14 - 7:05am

To be honest.... there is SO MUCH you can write over this, and these  arejust the first to start with.

I also disagree with the multiple return statement. But if you make too many return statements, you probably will do better to make some new functions as well... If every special part needs to be checked with a special if-statement for that case, then that can become a new function.

To really write some nice discussion about code style, you can fill a complete forum and this is far too small....

Robert C. Martin wrote a book "Clean Code, I used that a couple of years to state my opinion. Pls have a look on the other forum: http://www.daniweb.com/software-development/computer-science/threads/353722/clean-code-discussed (DaniWeb: Clean code discussed)

Tomasz O. replied on Fri, 2012/09/14 - 8:59am in response to: Alexey Grigorev

And I dissagree with you. You can always avoid multiple exit points. Without them the code is much cleaner.

Besides, we've got Robert C. Martin on our side ;)

http://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882

 

Sathya Sayee replied on Fri, 2012/09/14 - 9:10am in response to: Alexey Grigorev

Totally Agree. It is a very old notion that multiple returns are not good. I quiet often do multiple return statements.

Though the above example is a different usecase. it does not even need a return statement and I believe the author has later demostrated it. 

 

Jonathan Fisher replied on Fri, 2012/09/14 - 9:22am in response to: Marcos Antonio

@Marcos

It is, but it isn't. The fewer return points the better usually, there are some cases were this leads to more complex code. The best advice is to try to be consistent when you choose what, and write the simplest (aka the "least clever") code possible. 

Marcos Antonio replied on Fri, 2012/09/14 - 11:17am in response to: Jonathan Fisher

This is an excerpt from the book "Implementation Patterns", by Kent Beck on page 71:

"Back in the old days of programming, a commandment was issued: each routine shall have a single entry and a single exit. This was to prevent the confusion possible when jumping into and out of many locations in the same
routine. It made good sense when applied to FORTRAN or assembly language programs written with lots of global data where even understanding which statements were executed was hard work. In Java, with small methods and
mostly local data, it is needlessly conservative. However, this bit of programming folklore, thoughtlessly obeyed, prevents the use of guard clauses."

David Lim replied on Fri, 2012/09/14 - 12:30pm

I strongly disagree with not allowing multiple return points. And it's not because you're sometimes forced to create a redundant variable just to hold the return value.

I don't like code blocks like this:

private boolean doSomething(Object someObject)
{
    boolean result = false;

    if (<some condition>)
    {
        // several lines of code, enough to take up vertical space, but not warranting an extra method
        // especially worse if nested if's or loops are in here
    }

    return result;
}

When this is much more readable (if condition is wrong, don't bother reading the rest of the method):

private boolean doSomething(Object someObject)
{
    if ( !<some condition> )
    {
        return false
    }

    // actual logic when condition is met

    return <calculated result>;  // an intermediate variable may be avoided
}

And this is just one example. The primary motivation for multiple returns must be readability and better code structure. Not allowing it is an antiquated rule back when things are black and white (or green). Any decent IDE or dev text editor will properly highlight the return keyword.

Natan Cox replied on Fri, 2012/09/14 - 2:42pm

Well multiple return statements has been commented by now so lets tackle the use of constants.

 I have noticed that someting that seems to be a constant is quite often 2 or even more constants. So unless it is quite obvious I have stopped using them and most of my constants now are actually enums which still is a valid case bcos there you have a limited set of values for a type.

 Especially in the case of logging it is a bit silly bcos one day later I usually notice my logging sucks and have to fix it anyway. In those cases I always go for readabilty not for reusability. It is after all only logging. 

 Upper case static finals is quite often overkill too. Although I'm not ignoring the rule I'm finding many exceptions.  My log variables are never LOG (I hate shouting) and for RDF types we usually have a convention where we have variables like rdf_Class or dc_title following XML casing of the orignal specifications.

Even the {} are not always there. I write if (log.isDebugEnabled()) log.debug("Hello world."); to be the least invasive possible. And quite often my first line in a method is something like: if (param = null) return null; just to quickly handle the exceptions to the rule. 

 

 

Lund Wolfe replied on Sat, 2012/09/15 - 2:16am

I absolutely agree with the author on having a single return point.  I've seen a lot of code with returns sprinkled throughout the method.  The method is often too long and ugly but sometimes longer methods make sense.  Multiple returns are hard to read/understand and hard to maintain because you can't go in later and modify/fix one thing in one place.

I would make an exception in very trivial cases, like in the example with an if return else return, where simpler and shorter is better than adding unnecessary temporary variables.

Geoffrey De Smet replied on Sat, 2012/09/15 - 7:31am

Multiple return statements are (only) good for exceptions and corner cases, especially at the beginning of a method. For example:

public void reopenTicket(Ticket ticket) {
    if (ticket.getStatus() == Status.NEW || ticket.getStatus() == Status.OPEN) {
        return; // Already open
    if (ticket.getStatus() == Status.CLOSED) {
        thrown new IllegalArgumentException("The ticket (" + ticket + ") is closed, so it can not be reopened");
    }

    // Now that all the nonsense has been taken care of, it's time to do the actual business logic.
    // And that part should have a single return statement

    List<People> peopleToNotify = new ArrayList<People>();
    if (ticket.getStatus() >= Status.RESOLVED) {
       peopleToNotify.add(...);
    }
    if (ticket.getStatus() == Status.QA_VERIFIED) {
       peopleToNotify.add(...);
    }
    ticket.setStatus(Status.OPEN);
    fireReopenedEvent(ticket, peopleToNotify);
    // Single return!
}

 

Milos Silhanek replied on Wed, 2012/09/19 - 3:51pm

I don't like multiple exit points of method. Excluding such a called "early quit" (see examples given by others) which is labeled by visible comment "quit ----"

If you deal with 5-lines method with one if or switch may be. But like missing block brackets are wrong by later updates so you can break that methods two years later.

Milso

Sylvain Leroy replied on Wed, 2012/12/19 - 1:33am

If you would like more informations about uncommon / critical rules that are merely controled in Java Softwares, a good complement to this article is http://tocea.com/blog/code-audit-by-example/1991/5-highly-critical-yet-rarely-used-pmd-rules-software-quality-technical-deb.



Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.