Archive for category sonar

Through the eyes of sonar: recap.

Sonar is an opensource platform to manage code quality. Sonar is combining various java quality tools and collect these possible defects as violations.


Recap of all sonar rules articles

Naming and comments
Collections
Immutable Objects
Complexity
Exception handling
Equals and hashCode
Architural constraints
Rules customization, alert,…

I hope this will make your java code better

Leave a comment

Through the eyes of sonar : equals & hashCode.

Equals and hashCode

Theory

equals

Indicates whether some other object is “equal to” this one.

The equals method implements an equivalence relation on non-null object references:

– It is reflexive: for any non-null reference value x, x.equals(x) should return true.
— It is symmetric: for any non-null reference values x and y, x.equals(y) should return true if and only if y.equals(x) returns true.
— It is transitive: for any non-null reference values x, y, and z, if x.equals(y) returns true and y.equals(z) returns true, then x.equals(z) should return true.
— It is consistent: for any non-null reference values x and y, multiple invocations of x.equals(y) consistently return true or consistently return false, provided no information used in equals comparisons on the objects is modified.
— For any non-null reference value x, x.equals(null) should return false.

The equals method for class Object implements the most discriminating possible equivalence relation on objects; that is, for any non-null reference values x and y, this method returns true if and only if x and y refer to the same object (x == y has the value true).

Note that it is generally necessary to override the hashCode method whenever this method is overridden, so as to maintain the general contract for the hashCode method, which states that equal objects must have equal hash codes.

hashCode

Returns a hash code value for the object. This method is supported for the benefit of hashtables such as those provided by java.util.HashMap or java.util.HashSet.

The general contract of hashCode is:

– Whenever it is invoked on the same object more than once during an execution of a Java application, the hashCode method must consistently return the same integer, provided no information used in equals comparisons on the object is modified. This integer need not remain consistent from one execution of an application to another execution of the same application.
— If two objects are equal according to the equals(Object) method, then calling the hashCode method on each of the two objects must produce the same integer result.
— It is not required that if two objects are unequal according to the equals(java.lang.Object) method, then calling the hashCode method on each of the two objects must produce distinct integer results. However, the programmer should be aware that producing distinct integer results for unequal objects may improve the performance of hashtables.

As much as is reasonably practical, the hashCode method defined by class Object does return distinct integers for distinct objects. (This is typically implemented by converting the internal address of the object into an integer, but this implementation technique is not required by the JavaTM programming language.)

Recommandations

Use the commons lang classes to help your code to be “nullsafe” and easier to implement :

EqualsBuilder
HashCodeBuilder

If you are using hibernate :
— use the getter to avoid “proxy issues”
— avoid using the id as equality (which isn’t always filled)
— read the faq

If you implement them… unit test your equals/hashCode
— There’s a EqualsHashCodeTestCase that you can extends in the junit-addons library
— note that there are also constraint for serialisation/clone
— don’t call hashCode() but toHashCode() on hashCodeBuilder ;)
— avoid reflection based equals/hashCode.

Unit testing

the EqualsHashCodeTestCase is extended in order to test a class’s functional compliance with the equals and hashCode contract.
Override my createInstance and createNotEqualInstance methods to provide me with objects to test against. Both methods should return objects that are of the same class.
will test :
— whether equals holds up against a new Object (should always be false).
— whether equals holds up against null.
— whether equals holds up against objects that should not compare equal.
— whether equals is consistent.
— whether equals is reflexive.
— whether equals is symmetric and transitive.
— the hashCode contract.
— the consistency of hashCode Across Invocations .

Unit Tests and Serialization and serialization

If you adopt a unit-testing methodology, then any serializable class should pass the following three tests:
— If it implements readObject(), it should implement writeObject(), and vice-versa.
— It is equal (using the equals() method) to a serialized copy of itself.
— It has the same hashcode as a serialized copy of itself.

Similar constraints hold for classes that implement the Externalizable interface.

Sonar rules

Warn – eclipse generated equals check class

	public boolean equals(Object obj) {
		if (this == obj)
			return true;
		if (obj == null)
			return false;
		if (getClass() != obj.getClass()) // BAD IDEA IF USING HIBERNATE
			return false;

Prefer instanceof check, because the “obj” may be a HibernateProxy dynamically extending MyObject

	public boolean equals(Object obj) {
		if (this == obj){
			return true;
                }
		if (obj == null) {
			return false;
                }
		if (!(obj instanceof MyObject)) {
			return false;
		}

For the fields comparisons… prefer the EqualsBuilder

see http://tomaszdziurko.pl/2009/05/eclipse-and-problem-with-auto-generated-equals/

Bad practice – Class defines equals() and uses Object.hashCode()

This rule checks that classes that override equals() also need to override hashCode(). If you implement equals… you should implement hashCode accordingly.

Bad practice – equals() method does not check for null argument

As stated in the theory… “whether equals holds up against null”.
A simple check with instanceof… could do the trick.

 public boolean equals(final Object object) {
   if (object.getClass().equals(this.getClass())) {
 	DecisionDetail decisionDetail = (DecisionDetail) object;
    	if (getId() == null || decisionDetail.getId() == null) {
    		return object == this;	
   	}
   }
 }

See openpojo to test your model against such issue.

Correctness – equals method always returns false / Correctness – equals method always returns true

Having an equals method like this isn’t a good idea and doesn’t follow the contract on these methods

//BAD
 public boolean equals(Object object) {		
   return true; // or false			
 }

If you want a good default implementation

//GOOD
 public boolean equals(Object object) {		
   return this==object;
 }

Performance – hashCode return a constant

Returning a constant for the hashCode isn’t good for performance, as the hash is use to “compartiment/bucketing” data in structure like HashSet/hashMap.
Returning a constant will put all the element in the same bucket transforming a lookup of O(1) to O(n) function.

If you don’t have a clean equals criteria fallback to default implementation :

  @Override
  public int hashCode() {
 	return System.identityHashCode(this);
  }
  public boolean equals(Object object) {		
   return this==object;
  }

Correctness – equals() used to compare array and nonarray

Comparing apples and oranges, an array of X won’t be equals to an object instance X

  !PersonTypeCode.ALL.equals(searchParameter.getPersonTypeCode())

Where

  public class PersonTypeCode extends Code {
      public static final PersonTypeCode[] ALL = { PROSPECT, MEMBER, PROSPECT_OR_MEMBER, PROSPECT_FAMILY_MEMBER };
      public static final PersonTypeCode PROSPECT = new PersonTypeCode("PROSPECT");
      ...
   }

Equals vs == :Literal Equality

Literal Strings should be compared using equals(), not ‘!=’.

 String text = ...
 if (text != "") { //BAD
   ...
 }

Ideally… you should use StringUtils to compare in a null safeway.
Note that you have StringUtils.isEmpty(...) or StringUtils.isBlank(...)

 String text = ...
 if (!StringUtils.isEmmpty(text)) {
    ...
 }

Class defines equal(Object); should it be equals(Object)?

Surely a typo. If it’s not a typo choose a better name.

Class defines hashcode(); should it be hashCode()?

Surely a typo. If it’s not a typo choose a better name.

Class defines tostring(); should it be toString()?

Surely a typo. If it’s not a typo choose a better name.

Dodgy – Potentially dangerous use of non-short-circuit logic

 return equalsRoot(other) & new EqualsBuilder().append(this.sourceId, other.getSourceId()).isEquals();

This code seems to be using non-short-circuit logic (e.g., & or |) rather than short-circuit logic (&& or ||). In addition, it seem possible that, depending on the value of the left hand side, you might not want to evaluate the right hand side (because it would have side effects (hibernate initialization), could cause an exception (NullPointerException) or could be expensive (hibernate initialization).

 String s = null;   
 if (s == null || s.trim().length() == 0) {  
     // String s is really empty. Do some stuff.  
 }      
 if (s != null && s.trim().length() != 0) {  
    // String s is not empty. Do some stuff.  
 } 

Both examples would throw a NullPointerException when s is null and you use | and & respectively. (Prefer StringUtils for this kind of check)

Non-short-circuit logic causes both sides of the expression to be evaluated even when the result can be inferred from knowing the left-hand side. This can be less efficient and can result in errors if the left-hand side guards cases when evaluating the right-hand side can generate an error.

, , ,

Leave a comment

Through the eyes of sonar : Collections.

Collections usage

After Exception handling, Immutable objects, Complexity, Naming and comments, let’s see what sonar can teach you about Collections.

Use Arrays As List

Use asList instead of tight loops

 final SWTBotTreeItem[] errorItemsFromNode =...
 for (int j = 0; j < errorItemsFromNode.length; j++) {		
    errorItems.add(errorItemsFromNode[j]);
 }

Can be rewritten as :

 errorItems = Arrays.asList(errorItemsFromNode)

Note that the returned list is a non modifiable list. So if another part of the code tries to modify it will receive a UnsupportedOperationException

 java.util.List errorItems = new java.util.ArraysList(Arrays.asList(errorItemsFromNode));

Performance – Inefficient use of keySet iterator instead of entrySet iterator

The following code requires to do lookup (files.get(…)) and so evaluating hashCode on the key and while iterating on a Map and the doing a lookup the map implementation

 Map<String, Map<DateTime, File>> files = ...

  for (Iterator<String> iterator = files.keySet().iterator(); iterator.hasNext();) {
      final String serverName = iterator.next();			
      final Map<DateTime, File> serverStats = files.get(serverName);
      ...
  }			

You can prevent this, let’s use the entrySet() that will give you the key and associated value in a Map.Entry

Map<String, Map<DateTime, File>> files = ...
  for (Map.Entry<String, List<MailServerStats>> entry : files.entrySet()) {
      final String serverName = entry.getKey();			
      final Map<DateTime, File> serverStats = entry.getValue();
      ...
  }

Loose coupling

public class AST
   {
       private HashMap<String, Object> properties = new HashMap<String, Object>();
       
       public HashMap<String, Object> getProperties() { 
              return properties
       }
   }

Please use collection interfaces (Map, Set, List) instead of concrete implementation (HashMap, HashSet, ArrayList, LinkedList,…) :

 public class AST
   {
       private Map<String, Object> properties = new HashMap<String, Object>();
       
       public Map<String, Object> getProperties() { 
              return properties
       }
   }

This would allow you to easily switch implementation, in this case may be you want the map to be sorted.
You just need to change the new HashMap… by a new TreeMap.

Replace Vector With List

Consider replacing this Vector with the newer java.util.List
Vector synchronizes on each individual operation. That’s almost never what you want to do.
Note that Vector is synchronized… and ArrayList/LinkedList are not.
So if the synchronized nature is required in your code base
you should use Collections.synchronized

 List list = Collections.synchronizedList(new ArrayList());

Replace Hashtable With Map

Consider replacing this Hashtable with the newer java.util.Map
Hashtable synchronizes on each individual operation. That’s almost never what you want to do.

note that Hashtable is synchronized… and HashMap/TreeMap/… are not.
So if the synchronized nature is required in your code base
you should use Collections.synchronized

 Map list = Collections.synchronizedMap(new HashMap());

Class Cast Exception With To Array

This usage of the Collection.toArray() method will throw a ClassCastException.

  return (String[]) values.toArray();

the signature of the method : Object[] toArray()
the ‘java.lang.ClassCastException: [Ljava.lang.Object; incompatible with [Ljava.lang.String;’
but you have the other method T[] toArray(T[] a)

  return (String[]) values.toArray(new String[values.size()]);

Note that if you use generics you can drop the cast :

  List<String> values = new ArrayList<String>();
  values.add("1");
  values.add("2");
 return values.toArray(new String[values.size()]);

Correctness – Impossible downcast of toArray() result

Same explanation as previous rules.

	private static Map<String, String> linksreasonidtogroupid = new HashMap<String, String>();
	public void testSample() throws Exception {
		getReasonsid();
	}
	
	public static String[] getReasonsid() throws Exception {
		Collection<String> values = linksreasonidtogroupid.values();
		return (String[]) values.toArray();

	}

You endup with :

java.lang.ClassCastException: [Ljava.lang.Object;
	at CastTest.getReasonsid(CastTest.java:18)
	at CastTest.testSample(CastTest.java:11)

Correctness – Invocation of toString on an array

Suppose you want to logs the variables in the following program :

String[] vars = { "1", "2" };
System.out.println(vars.toString());

the output produced isn’t really helpfull

[Ljava.lang.String;@10b4199

instead use conditionnal logging and org.apache.commons.lang.ArrayUtils.toString (nullsafe and performant)

String[] vars = { "1", "2" };
if (LOG.isDebugEnabled()) {
    LOG.debug(ArrayUtils.toString(vars));
}
[1, 2]

4 Comments

Through the eyes of sonar : Naming & Comments.

Fourth “Through the eyes of sonar” article ( 1, 2, 3 ), let’s review various naming & comments rules.

Guidelines

English: Illustration of all caps .vs. mixed case.

English: Illustration of all caps .vs. mixed case. (Photo credit: Wikipedia)

The excellent work of Cagdas Basaraner provide us these guidelines :

  • Use short enough and long enough variable names in each scope of code. Generally length may be
    • 1 char for loop counters,
    • 1 word for condition/loop variables,
    • 1-2 words for methods,
    • 2-3 words for classes,
    • 3-4 words for globals.
  • Use specific names for variables, for example "value", "equals", "data", … are not valid names for any case.
  • Use meaningful names for variables. Variable name must define the exact explanation of its content.
  • Don’t start variables with o_, obj_, m_, etc. A variable does not need tags which states it is a variable.
  • Obey company naming standards and write variable names consistently in application: e.g. txtUserName, lblUserName, cmbSchoolType, … Otherwise readability will reduce and find/replace tools will be unusable.
  • Obey programming language standards and don’t use lowercase/uppercase characters inconsistently: e.g. userName, UserName, USER_NAME, m_userName, username,
    • use Camel Case (aka Upper Camel Case) for classes: VelocityResponseWriter
    • use Lower Case for packages: com.company.project.ui
    • use Mixed Case (aka Lower Camel Case) for variables: studentName
    • use Upper Case for constants : MAX_PARAMETER_COUNT = 100
    • use Camel Case for enum class names and Upper Case for enum values.
    • don’t use ‘_’ anywhere except constants and enum values (which are constants).
  • Don’t reuse same variable name in the same class in different contexts: e.g. in method, constructor, class. So you can provide more simplicity for understandability and maintainability.
  • Don’t use same variable for different purposes in a method, conditional etc. Create a new and different named variable instead. This is also important for maintainability and readability.
  • Don’t use non-ASCII chars in variable names. Those may run on your platform but may not on others.
  • Don’t use too long variable names (e.g. 50 chars). Long names will bring ugly and hard-to-read code, also may not run on some compilers because of character limit.
  • Decide and use one natural language for naming, e.g. using mixed English and German names will be inconsistent and unreadable.
  • Use meaningful names for methods. The name must specify the exact action (ideally behavior) of the method and for most cases must start with a verb. (e.g. createPasswordHash)
  • Obey company naming standards and write method names consistently in application: e.g. getTxtUserName(), getLblUserName(), isStudentApproved(), … Otherwise readability will reduce and find/replace tools will be unusable.
  • Obey programming language standards and don’t use lowercase/uppercase characters inconsistently: e.g. getUserName, GetUserName, getusername, …
    • use Mixed Case for method names: getStudentSchoolType
    • use Mixed Case for method parameters: setSchoolName(String schoolName)
  • Use meaningful names for method parameters, so it can document itself in case of no documentation.

Sonar Naming Ruleset

Package Name

A package is lowercase and avoid strange character

Name ‘com.mestach.myBeans‘ must match pattern ‘^[a-z]+(\.[a-zA-Z_][a-zA-Z0-9_]*)*$‘.

 
package com.mestach.myBeans; //bad
package com.mestach.model;   //good

Either you repackage to com.mestach.mybeans or com.mestach.model
Note that windows isn’t case sensitive but linux is. So if you are using an old versioning system like CVS,
Renaming once committed to a linux from windows may be harder (packages end up as folder : com/mestach/mybeans vs com/mestach/myBeans)

Member Name

Member Name : Name ‘FIELD_FIRSTNAME‘ must match pattern ‘^[a-z][a-zA-Z0-9]*$‘.

 protected String FIELD_FIRSTNAME = "fieldfirstname";

Either
— it’s a constant not well defined → add static final
— it’s an attribute of the class and so it’s preferable to use the lowercase version : fieldFirstName

Local Variable Name and Parameter Name

Local Variable Name : Name ‘fromDate_db2TimeStamp‘ must match pattern ‘^[a-z][a-zA-Z0-9]*$

String fromDate_db2TimeStamp = DATEFORMAT_db2TimeStamp.format(fromDate);

Avoid underscore in variable name, java naming convention is more camelCase.

String fromDateDb2TimeStamp = DATEFORMAT_db2TimeStamp.format(fromDate);

Parameter and variable should starts by a lowercase :

public void setMember(Member Member) // BAD
 public void setMember(Member member) //GOOD

A more complex sample involving spring injection :

        //BAD
       @Autowired
       private MyService module_MyService;
      public void setModule_MyService(MyService module_MyService){
               this.module_MyService = module_MyService;
      }

Prefer

        //GOOD
        @Autowired
        @Qualifier("module_MyService")
        private MyService myService;
        public void setMyService(MyService myService){
            this.myService=myService;
       }

Static Variable Name

Sometime… it’s just a final which is missing :

private static Logger LOG = Logger.getLogger(BatchLogger.class);   // BAD
private static final Logger LOG = Logger.getLogger(BatchLogger.class); //GOOD

Often in the code base you have (open) enum like this classes

public class ObjectType {
   public static String SRVTYPE = "SERVER";
   public static String PRINTERTYPE = "PRINTER"
   public static ObjectType SERVER = new ObjectType(SRVTYPE );
   public static ObjectType PRINTER= new ObjectType(PRINTERTYPE );
   private String code;
   
   private ObjectType(String c) {
	code = c;
   }
   public String getCode() {
	return code;
    }
  }

Can be fixed by :

 public class ObjectType {
   private static final String SRVTYPE = "SERVER";
   private static final String PRINTERTYPE = "PRINTER"
   public static final ObjectType SERVER = new ObjectType(SRVTYPE );
   public static final ObjectType PRINTER= new ObjectType(PRINTERTYPE );


    ...

– Don’t publish you SERVER string “constant”, make them private
— The api using this ObjectType.SRVTYPE can be fixed by ObjectType.SERVER.getCode()

Malicious code vulnerability – Field isn’t final but should be

– Make your constant final ! It will prevent modifying the constant inadvertently

 
ObjectType.SERVER =ObjectType.PRINTER

Method Name

Starts with lower case and use camelCase

 //BAD
  public String ProductReasonForm(Model model) {

Prefer

 
//GOOD
  public String productReasonForm(Model model) {

“Name methods after what they are intended to do.” (rather than how they do it) Kent Beck

Bad practice – Class names shouldn’t shadow simple name of superclass

This would help other developper to import the “correct” implementation.

  public class DwrController extends org.directwebremoting.spring.DwrController

Just rename the class to avoid ambiguity.

  public class CustomDwrController extends org.directwebremoting.spring.DwrController

Bad practice – Confusing method names

Bad practice – Confusing method names : Confusing to have methods com.mestach.Person.getDeclarationWithincome() and com.mestach.Person.getDeclarationWithIncome()

Did you notice the “case changes in method name “: Withincome vs WithIncome

– either you wanted to override the method… and you mispelled it.
— either you should perhaps rename it with a less confusing name.

Use positive conditionals

Use positive conditionals. Readability of positive conditionals are better than negative ones.

Negated boolean variable names must be avoided.

 boolean isNotFound; // BAD
 boolean isNoError // BAD

Positive !

 boolean isError; // GOOD
 boolean isFound; // GOOD

The problem arise when the logical not operator is used and double negative arises.
It is not immediately apparent what !isNotError means.

Avoid boolean flags in method signature

Don’t use method alternatives with boolean flag variables (public void someMethod(bool flag)). Write more than one method for each flag condition.

Eg :

 Person loadPerson(Long id, boolean withContactInfos)

Prefer

 /** don't the load the ContactInfos by default */
 Person loadPerson(Long id )
 Person loadPersonWithContactInfos(Long id )

Or introduce an initializationMode

 enum PersonInitializationMode {
     WITHCONTACTINFOS, WITHACCOUNTNUMBERS
  }

    Person loadPerson(Long id ,PersonInitializationMode...  initMode)

More samples from Alex Ruiz’s boolean arguments can be evil

In summary, let’s not use boolean parameters. They don’t communicate their purpose. Instead, we can:
— in the case of methods, split them in two
— in the case of constructors, use static factory methods
— in the case of “three-state” booleans, use Java enums

Use understandable and long names for variables.

– Loop variable names can be i, j, k, index etc.,
— local variable names must be longer than loop variables, parameter names must be longer than local variables and static variable names must be longer than parameters; proportional with scope size

Prefer

 int currentLineNumber;
double amount;

over

 int cln;
 double a;

non-Static nested inner classes

If possible make them static :
— They are more loosely coupled to the parent class. In general, the weaker the coupling, the better.
— They do not hold a pointer to the parent instance. This means:
— They can exist without a parent instance.
— They have a slightly smaller memory footprint.
— They don’t prevent the parent from being GCed.
And ideally move them outside, this will make the code more readable.

A typical example where it causes issues : inner class that extends TimerTask gets registered with a Timer, and then the timer’s thread keeps both the TimerTask and the parent instance alive.

Avoid static methods

Think twice before defining a method as static and be sure if you really need to. Static methods are harder to manage :
— test (state, singleton) and
— evolve (methods overriding, use of beans, alternate implementation).

Comments

Prefer a good naming over comments

For example avoid abbreviation

  /** compute the average */
 double compAvg(double a ,double b)

The name of the method explain it’s purpose !

double computeAverage(double a ,double b)

A comment is often never maintained as the code is refactored.
Leaving the maintainer with comments that don’t match the reality anymore.

Bad comment is worse than no comment.

Don’t comment to delete code

Don’t use comment lines to delete code, just delete. Version controlling system will help you if deleted code is required.

Sonar can help you identifying this. In the comments widget you have the commented LOCs.

 Comments
 28.6%
 70,143 lines 
 74.6% docu. API
 4,604 undocu. API
1,770 commented LOCs
Enhanced by Zemanta

3 Comments

Through the eyes of sonar : Complexity

Complexity

I’m continuing the “Through the eyes of sonar” series ( 1 & 2 ) with various code complexity issues: First a small recap of guidelines/rules and then some practical examples.

KISS Keep it simple & stupid

  1. Easy to read : Simple code doesn’t need additional documentation, or it only needs minimal documentation to be understood
  2. Easy to use : Whoever is using your code will find intuitive to use your objects.
  3. Easy to change : Simplicity means that there is no duplicated logic. A change should match a single place in your code.
  4. Doesn’t use any non-necessary third party tool or technology. Through my experience I have observed how some developers just love to use technologies, tools and frameworks just for the sake of making the project more “cool”. Every time you use them, you are adding extra complexity, and extra complexity means that is more difficult to understand, to maintain and is more prone to failures.
  5. It looks simple. If it doesn’t look simple, it is not simple! You know your code is simple if when you finish you are astonished at how simple the final solution is and you ask yourself how it was possible that it took you so long.
  6. Lean : It only does what is necessary, and nothing else. I think most seasoned developers would agree that trying to anticipate the problems your code will have to face in the future is impossible.
  7. Is straightforward. It doesn’t have unnecessary indirections; the most important operations only require a straight method call.

Some basic rules

Lets see some of the basic rules to write maintainable code :

  1. Keep methods short(one screen limit)
  2. Never ever ever reuse a variable for different purpose
  3. Use self-descriptive variable and method names
  4. Define variables as close as possible to the place of their usage
  5. No magic numbers
  6. Be friend with your language
  7. Don’t fight the convention
  8. Watch out for premature optimization
  9. Always refactor the code after you test it
  10. Don’t get sucked into over-engineering
  11. Learn new things by prototyping

All these rules are explained with concrete examples here

Cyclomatic complexity

Tools like checkstyle will check the cyclomatic complexity of methods against a specified limit.

The complexity is measured by the number of if, while, do, for, ?:, catch, switch, case statements, and operators && and || (plus one) in the body of a constructor, method, static initializer, or instance initializer. It is a measure of the minimum number of possible paths through the source and therefore the number of required tests.

Generally :

  • 1-4 is considered good,
  • 5-7 ok,
  • 8-10 consider re-factoring, and
  • 11+ re-factor now !

How we can reduce it… by implementing simple refactoring. Some of them are built-in in eclipse IDE, sample !

Simplify Boolean Return (Conditional logic can be removed)

Sometime we write unnecessary conditional logic :

public boolean occupiesPosition(int x) {
   if (x >= position && x <= position + data.length())
      return true;
   else
      return false;
}

Can be rewritten :

public boolean occupiesPosition(int x) {
  return (x >= position && x <= position + data.length())
}

Note that some developer still like to introduce a boolean temporary variable to ease the debugging or logging, may be for those developer you can disable UnnecessaryLocalBeforeReturn rule.

Simplify Conditional

Sometime our paranoia of NullPointerException leads us to write unnecessary code. In this example there’s no need to check for null before an instanceof.

public static Object[] getSelectedElements(final ISelection sel) {
   if (sel == null || !(sel instanceof IStructuredSelection))
     return null;
   IStructuredSelection strucSel = (IStructuredSelection) sel;
   ...
}

Extract boolean expression in a method

for (Iterator it = this.timeLines.values().iterator(); it.hasNext(); ) {
  final ITimeLine timeLine = (ITimeLine) it.next();
  final long start = timeLine.getStartTime().getMillis();
  final long end = timeLine.getEndTime().getMillis();
  if ( (start <= startTime && end >= startTime) ||
     (start >= startTime && end <= endTime) ||
     (start <= endTime && end >= endTime) ) {
      res.add(timeLine);
  }
}

If you put the logic in a contains method, you end up with 2 methods of lower complexity and easier to read !

for (Iterator it = this.timeLines.values().iterator(); it.hasNext(); ) {
  final ITimeLine timeLine = (ITimeLine) it.next();
  if ( contains(timeLine)) {
      res.add(timeLine);
  }
}

And with the extracted methods :

private boolean contains(ITimeLine timeLine) {
  final long start = timeLine.getStartTime().getMillis();
  final long end = timeLine.getEndTime().getMillis();
  return (start <= startTime && end >= startTime) || (start >= startTime && end <= endTime) || (start <= endTime && end >= endTime) )
}

Replace if else if else by a map or a set

For example sometimes you see a deep if else tree like this

public boolean isManual() {
      if (MANUAL_CLERCK.equals(this))
         return true;
      else if (MANUAL_MEMBER.equals(this))
         return true;
      else if (MANUAL_INSURER.equals(this))
          return true;
      else if (MANUAL_DECL.equals(this))
	     return true;
      return false;
}

Adding a Set may simplify the if-else-if cascade to a simple contains

private static final Set<IndicationOrigin> MANUAL_ORIGINS = new HashSet<IndicationOrigin>();
static {
   MANUAL_ORIGINS.add(MANUAL_CLERCK);
   MANUAL_ORIGINS.add(MANUAL_MEMBER);
   MANUAL_ORIGINS.add(MANUAL_INSURER);
   MANUAL_ORIGINS.add(MANUAL_DECL);
}
public boolean isManual() {
   return MANUAL_ORIGINS.contains(this);
}

Avoid ternary operator

the following code throws a NullPointerException… despite the usage of the ternary operator to check for null value.

DateMidnight startDate = null;
LOGGER.debug(" ==> NPE " + startDate != null ? startDate.toString(Constants.DATE_FORMAT) : "null");

In fact the priotiry in operators, will be evaluated like this (added parentheses) :

LOGGER.debug((" ==> NPE " + startDate)!= null ? startDate.toString(Constants.DATE_FORMAT) : "null");

So the boolean expression is always true (concat string) even if the startDate is null.
To fix this you either drop the ternary operator or add the necessary braces to change the evaluation priority

LOGGER.debug(" ==> correct log but full timestamp format " + startDate);
LOGGER.debug(" ==> correct log " + (startDate != null ? startDate.toString(Constants.DATE_FORMAT) : "null"));

The code maybe shorter, but you’ve won a bug, so avoid using the ternary operator.

Dodgy – Method uses the same code for two branches

This kind of constructs complexify the code :

if (userLanguage == null || userLanguage.equals("fr")) {
   formattedAddress.append(address.street.trim());
} else {
   formattedAddress.append(address.street.trim());
}

Either you can remove the if else…

formattedAddress.append(address.street.trim());

Either you have a copy/paste error

if (userLanguage == null || userLanguage.equals("fr")) {
     formattedAddress.append(address.street.trim());
 } else {
     formattedAddress.append(address.streetNl.trim());
 }

8 Comments

Through the eyes of sonar: Immutable Objects.

Sonar is an open platform to manage code quality. Sonar is combining various java quality tools and collect these possible defects as violations. Let’s see how sometimes we misuse immutable objects.


Immutable Objects

I’m continuing the “Through the eyes of sonar” series with all these immutable objects you use all the times.
Immutable objects are simply objects whose state (the object’s data) cannot change after construction.

Immutable objects greatly simplify your program, since they :
— are simple to construct, test, and use
— are automatically thread-safe and have no synchronization issues
— do not need a copy constructor
— do not need an implementation of clone
— allow hashCode to use lazy initialization, and to cache its return value
— do not need to be copied defensively when used as a field
— make good Map keys and Set elements (these objects must not change state while in the collection)
— have their class invariant established once upon construction, and it never needs to be checked again
— always have “failure atomicity” (a term used by Joshua Bloch) : if an immutable object throws an exception, it’s never left in an undesirable or indeterminate state

Examples of immutable objects
— from the JDK include String, Integer, BigDecimal, Boolean, etc.
— from joda time : DateMidgnight, DateTime,…

More on this topic : ImmutableObjects

java.lang.Integer

Integer Instantiation

Avoid instantiating Integer objects. Call Integer.valueOf() instead. This allows the class to give you back an existing instance if an object was already created for this number. Note this optimization that it’s more for small values of integer and depends on the jre implementation.

 return new Integer(rowCount);

Prefer :

 return Integer.valueOf(rowCount);

java.lang.Double

Why we don’t use Doubles for Financial Calculations

    package com.bloodredsun;

    public class DoubleOperation {

        public static void main(String[] args) {

            double t1 = 10.266d;
            double t2 = 10.0d;

            //Outputs 0.266
            System.out.println(t1-t2);

            double h1 = 100.266d;
            double h2 = 100.0d;

            //Outputs 0.26600000000000534
            System.out.println(h1-h2);
        }
    }

Ouch! That is not what we want but it is the classic behavior of doubles. The inability to represent some decimals in the IEEE-754 format (as binary fractions) causes this. If we want correct precision the answer is to use BigDecimals but we have to remember to use Strings in the constructors or you end up with the same issues that you were trying to avoid.

    package com.bloodredsun;

    import java.math.BigDecimal;

    public class BigDecimalOperation {

            public static void main(String[] args) {

            BigDecimal t1 = new BigDecimal("10.266");
            BigDecimal t2 = new BigDecimal("10.0");

            //Outputs 0.266
            System.out.println(t1.subtract(t2));

            BigDecimal h1 = new BigDecimal("100.266");
            BigDecimal h2 = new BigDecimal("100.0");

            //Outputs 0.266
            System.out.println(h1.subtract(h2));
        }
    }

Notice the usage of the string constructor ! You will read more about it in the the next paragraph about java.math.BigDecimal.

java.math.BigDecimal

Avoid Decimal Literals In Big Decimal Constructor

As the BigDecimal class is immutable, all its methods modifying its value return a new BigDecimal object. The newly created object contains the value modified.

  BigDecimal dec = new BigDecimal("123.54");
  BigDecimal dec2 = new BigDecimal(123.54);
  System.out.println("dec value = " + dec);
  System.out.println("dec2 value = " + dec2);

Result displayed in the console:

 dec value = 123.54
 dec2 value = 123.5400000000000062527760746888816356658935546875

As Java doesn’t parse and represent these two numbers in the same way, the result is different.
For this reason, when you use a BigDecimal object, create it with the constructor using a String in argument.

  BigDecimal dec = new BigDecimal(123.54);
  System.out.println("dec value = " + dec);
  BigDecimal dec2 = dec.setScale(2, BigDecimal.ROUND_HALF_UP);
  System.out.println("dec2 value = " + dec2);

Result displayed in the console:

  dec value = 123.5400000000000062527760746888816356658935546875
  dec2 value = 123.54

Useless Operation On Immutable

As the setScale(newScale, roundingMode) method is used on an immutable object, it returns a new BigDecimal object with the modified value. If you don’t put the result of this method in another BigDecimal object, you will not able to work on this new value.
Remember that the example above is valid for all methods involving the modification of a BigDecimal object’s value.

  BigDecimal dec = new BigDecimal("123.54");
  dec.add(new BigDecimal("1.0"));
  dec.add(BigDecimal.ONE);
  dec.multiply(BigDecimal.TEN);
  System.out.println(dec);

This will display :

 123.54

Fixing by reassigning the dec variable :

  BigDecimal dec = new BigDecimal("123.54");
  dec = dec.add(new BigDecimal("1.0"));
  dec = dec.add(BigDecimal.ONE);
  dec = dec.multiply(BigDecimal.TEN);
  System.out.println(dec);

This will display :

 1255.40

All mathematical methods of the BigDecimal class ask you to give in argument a BigDecimal object containing the value to apply on the value of your BigDecimal object. To avoid the instantiation of a BigDecimal object just to add one or multiply your numbers by ten (as in the example), you can use the constant defined in the BigDecimal class. Use them to avoid creating multiple objects in memory.

org.joda.DateTime/DateMidnight

Use Joda Time

Don’t use the mutable java built-in classes for date/time manipulation, prefer the use the immutable and fluent api of jodatime.

Useless Operation On Immutable

DateTime is also an immutable class. It suffers of the same advantages and inconveniences of the BigDecimal class.
When you use the class, don’t forgot to provide an object to receive the result of your modification or they will be be never apply (cf. the following example) :

public void testAddMonth() throws Exception {
  DateTime time = new DateTime();
  System.out.println(time);
  time.plusMonths(1);
  System.out.println(time);
}

Result displayed in the console :

  2009-10-21T14:34:24.543+02:00
  2009-10-21T14:34:24.543+02:00

No month added… damned.

time=time.plusMonths(1);

java.lang.Boolean

Avoid Instantiation

The Boolean class is used instead of the primitive type in order to offer the possibility to have three values for a boolean (true, false and null). If you just need to use the classical true-false value, use the primitive type.

Use primitives if you can, wrappers if you must

  new Boolean("true"); // bad practice : this will create always new instances prefer Boolean.valueOf(...)

Prefer :

  Boolean.FALSE;
  Boolean.TRUE;
  Boolean.valueOf(x>10)

If it is possible, use the constant defined in the Boolean class instead of creating a new object.

  if (x > 10){
    Boolean.TRUE;
  }
  else {
    Boolean.FALSE;
  }
  Boolean.valueOf(x > 10);

The valueOf(boolean) method can receive in argument a condition which has as result true or false. As you can see in the example before, you can resolve the if-else code by using the valueOf(boolean) method.

java.lang.String

Useless Operation On Immutable

String name = "David";
name.replaceAll("D", "d");
System.out.println("BAD   "+name);
name= name.replaceAll("D", "d");
System.out.println("GOOD  "+name);
BAD David
GOOD david

Correctness – Method ignores return value

I stumble upon this code

String key = ...
key.charAt(keyPos);

Either we forgot something to the index char returned or we can simply drop the call from the codebase.

Avoid String To String

Avoid calling toString on String

    StringBuffer buffer = new StringBuffer(4);
    if (socialState != null) {
  	buffer.append(socialState.toString());
	buffer.append(" ");
     }

Simply :

    StringBuffer buffer = new StringBuffer(4);
    if (socialState != null) {
  	buffer.append(socialState);
	buffer.append(" ");
     }

Note also that in this code we should have used StringBuilder. Check this article :
Using StringBuffer instead of StringBuilder

Performance – Method invokes inefficient new String(String) constructor

Avoid creating a new string :

   protected String getCode(final Code code) {
     return new String(code.getValue() + " " + getLabel(code));
   }

Simply :

 protected String getCode(final Code code) {
     return code.getValue() + " " + getLabel(code);
   }

Correctness – “.” used for regular expression

The dot is a special character in regexp. Using String.split(regexp)… will not split

 String[] splitKeys = this.propertyDescriptor.getClassDescriptor().getName().split(".");

A simpler and safer way is to use StringUtilsor the StringTokenizer where the dot is processed as a normal character

 String[] splitKeys = StringUtils.split(this.propertyDescriptor.getClassDescriptor().getName(),".")

Inefficient String Buffering

Note that string concatenation is often badly coded… some myth tells you that StringBuffer or StringBuilder are the way to go. But one thing that most developers don’t know… is that the compiler is smarter than you ;)

 "4564654"+sdfds+"1231321"

will be compiled to :

 new StringBuilder("4564654").append(sdfds).append("1231321").toString()

In some cases these optimizations are not enough, specially when looping on a list of records :

strBuff.append("; " + contact.targetid);
strBuff.append("; " + contact.office);
strBuff.append("; " + task.dueDate);
strBuff.append("; " + task.objectId);
strBuff.append("; " + task.statusId);

 

public static final String SEPERATOR = "; "

strBuff.append(contact.targetid);
strBuff.append(SEPERATOR);
strBuff.append(contact.office);
strBuff.append(SEPERATOR);
strBuff.append(task.dueDate);
strBuff.append(SEPERATOR);
strBuff.append(task.objectId);
strBuff.append(SEPERATOR);
strBuff.append(task.statusId);

NullSafety and trim/blank

Instead of :

  if  (anAddress.getZipCode() == null || "".equals(anAddress.getZipCode().trim()))
     return false;

Prefer the less error prone version using StringUtils :

  return StringUtils.isNotBlank(anAddress.getZipCode());

8 Comments

Through the eyes of sonar: Exception handling

Sonar is an open platform to manage code quality. Sonar is combining various java quality tools and collect these possible defects as violations. Let’s see some exception handling worst practices detected by these tools.


Avoid Print Stack Trace

Instead of :

catch (Exception e) {
  e.printStackTrace();
}

This exception will often get logged in a separate file (System.err) making debugging more complex than it needs.
Use a logger call instead.


catch (Exception e) {
  logger.warn("failed to ...." ,e);
}

Note the ,e this will tell log4j/slf4j/… to log the full stacktrace. Note that you can configure sfl4j to log the root cause first

Avoid logging only exception message

catch (Exception e) {
  logger.warn("failed to ...."+e);
}

Use the “,” variant of the logging method. In case of null pointer… the log will looks like :

 failed to ....null

This isn’t really helpful for debugging. So please use the other method :

catch (Exception e) {
  logger.warn("failed to ...." , e);
}

UseCorrectExceptionLogging and other pmd rules

Preserve Stack Trace

Intead of loosing track of the InstantiationException

 } catch (InstantiationException e) {
     throw new HibernateException("Could not instantiate resultclass: " + resultClass.getName());
 }

Preserve the stacktrace with a nested exception constructor :

 } catch (InstantiationException e) {
     throw new HibernateException("Could not instantiate resultclass: " + resultClass.getName() ,e);
 }

Avoid Catching Throwable

A catch statement should never catch throwable since it includes errors (OutOfMemoryError,…) :

 catch (Throwable t) {
  log.warn("unmonitored problem",t);
  success = false;
 }

So do this only in rare case (dictated by an api) where severe problem should catched and logged. If you are forced to do so ensure that the error is logged !

Prefer runtime exception, avoid signature that declare throws Exception

It is unclear which exceptions can be thrown from the methods. It might be difficult to document and understand the vague interfaces.

public Set getChildrenNames(Fqn arg0) throws Exception {

Use a class derived from RuntimeException. This also often force the user of your API to catch these exception and as you can saw it’s no easy to get right !
Checked exceptions I love you, but you have to go

A history of exceptions — Exceptions and how they should be handled always end in heated debates between Java developers. It isn’t surprising that Hibernate has some noteworthy history as well. Until Hibernate 3.x, all exceptions thrown by Hibernate were checked exceptions, so every Hibernate API forced the developer to catch and handle exceptions. This strategy was influenced by JDBC , which also throws only checked exceptions. However, it soon became clear that this doesn’t make sense, because all exceptions thrown by Hibernate are fatal. In many cases, the best a developer can do in this situation is to clean up, display an error message, and exit the application. Therefore, starting with Hibernate 3.x, all exceptions thrown by Hibernate are subtypes of the unchecked Runtime Exception, which is usually handled in a single location in an application. This also makes any Hibernate template or wrapper API obsolete.

Avoid Instanceof Checks In Catch Clause

Each caught exception type should be handled in its own catch clause. For example SQLException a subclass DataTruncation that contains more information and in separate catch you can log the parameter that exceeded the db length, but you will probably share a common method that will handle the rollback of the transaction.

Avoid Throwing Null Pointer Exception

NPE means programmer’s mistake and if you throw NPE you are not a nice person.
NPE may be a convention in the JDK, but perhaps it should stay there too.

if (flowName == null) {
   throw NullPointerException("FlowName cannot be null.");

Throwing NPE is really confusing, prefer IllegalArgumentException :

if (flowName == null) {
   throw IllegalArgumentException("FlowName cannot be null.");

A shorter version using spring assert :

import org.springframework.util.Assert;
Assert.notNull(flowName, "FlowName cannot be null.");

Check also commons-lang Validate or Guava Preconditions

Avoid Throwing Raw Exception Types

Rather than throw a raw RuntimeException, Throwable, Exception, or Error, use a subclassed exception or error instead.

    if (StringUtils.isEmpty(props.getString("smtp.config"))) {
      throw new RuntimeException("Bad Configuration for smtp.config");
    }

Prefer throwing specific exception like :

    if (StringUtils.isEmpty(props.getString("smtp.config"))) {
         throw new BadConfigurationException("smtp.config");
    }

Correctness – Exception created and dropped rather than thrown

Silly mistakes. We are human after all… luckily sonar is there to spot it !

This code creates an exception (or error) object, but doesn’t do anything with it. For example, something like

    if (x < 0)
      new IllegalArgumentException("x must be nonnegative");

It was probably the intent of the programmer to throw the created exception:

    if (x < 0)
      throw new IllegalArgumentException("x must be nonnegative");

Empty Try Block

The less you have code to maintain… the less work you have. just drop this kind of code

public void run() {
	try {
	} finally {
		try {
		} catch (Exception e) {
		}			
	}		
}

Bad practice – Method may fail to close stream on exception

Stream from java.io.File access,URLConnection,… should be closed whatever happens in a finally clause.
This will prevent also java.io.IOException: Too many open files

  File f = new File(parent.getAbsolutePath(),"TEST-"+getTestSuiteResult().getModuleName()+".xml");
    try {
	BufferedWriter bw = new BufferedWriter(new FileWriter(f));
	bw.write("<!--?xml version=\"1.0\" encoding=\"UTF-8\" ?-->" + ENDL);
	bw.write(getTestSuiteOpenTag() + ENDL);
	for (UnitTestCaseResult tc : getTestSuiteResult().getTestCaseResults()) {
 	    bw.write(getTestCaseTag(tc) + ENDL);
	}
	bw.write(getTestSuiteEndTag() + ENDL);
	bw.flush();
	bw.close();
    } catch (IOException e) {
	throw new RuntimeException(e);
    }

To fix use the org.apache.commons.io.IOUtils to finally close the writer

    File f = new File(parent.getAbsolutePath(),"TEST-"+getTestSuiteResult().getModuleName()+".xml");
    BufferedWriter bw = null;
    try {
	bw = new BufferedWriter(new FileWriter(f));
	bw.write("<!--?xml version=\"1.0\" encoding=\"UTF-8\" ?-->" + ENDL);
	bw.write(getTestSuiteOpenTag() + ENDL);
	for (UnitTestCaseResult tc : getTestSuiteResult().getTestCaseResults()) {
 	    bw.write(getTestCaseTag(tc) + ENDL);
	}
	bw.write(getTestSuiteEndTag() + ENDL);
	bw.flush();
    } catch (IOException e) {
	throw new UnitTestCaseGenerationException(e);
    } finally {
        IOUtils.closeQuietly(bw);
    }

If you are lucky to use jdk7 take a look at the The try-with-resources Statement.

6 Comments

Follow

Get every new post delivered to your Inbox.