Solve the sonar problem in simple getter and setter methods using Date or List

Question

I am writing this getter / setter to the list from the Eclipse source menu:

public Date getDate() { return date; } public void setDate(Date date) { this.date = date; } 

And Sonar report two issues:

Return a copy of the “date” and keep a copy of the “date”

with an explanation

"Mutable members should not be stored or returned directly"

and example code:

 public String [] getStrings() { return strings.clone();} public void setStrings(String [] strings) { this.strings = strings.clone();} 

I think if my date is NULL, this will throw a NullPointerException. Then I changed my code to:

 public Date getDate() { if (this.date != null) { return new Date(this.date.getTime()); } else { return null; } } public void setDate(Date date) { if (date != null) { this.date = new Date(date.getTime()); } else { this.date = null; } } 

And now another problem is noted:

"Assigning a null object is a code smell. Consider refactoring."

I searched the Internet and installed or returned a new array, not for me, I want to keep my list null if the setter parameter is null to overwrite the existing previous list.

I have the same problem for List and I want to return / save null instead of the new ArrayList for an empty list. And in this case, the installer notes another problem:

"Returns an empty collection instead of null.".

What is the solution to this problem?

Answer 1

If you are in Java 8 and do not want to handle an empty date, you might find it useful to use the option.

Edit: Example of your POJO class

 public class Toto { public Optional<Date> myDate; public Optional<Date> getMyDate() { return this.myDate; } public void setMyDate(final Date myDate) { this.myDate = Optional.ofNullable(myDate); } } 

Code usage example:

 Toto toto = new Toto(); toto.setMyDate(null); System.out.println("Value is null ? " + toto.getMyDate().isPresent()); System.out.println("Value: " + toto.getMyDate().orElse(new Date())); 

Try changing toto.setMyDate (...) with a specific date value to see what happens.

If you do not know what is optional or how to use it, you can find many examples.

BUT . This is only a way to solve the problem with violations, and I completely agree with Brad's remark. Not necessarily intended to be used as a type, but rather as a contract for potential empty / null returns. In general, you do not have to correct your code correctly to fix the violation if the violation is incorrect. And in your case, I think you should just ignore the violation (since most sonar one is unsuccessful)

If you really want to use Java 8 and not necessarily in your code, then you will be a POJO class (this use is optional as a getter only contrat)

 public class Toto { public Date myDate; public Optional<Date> getMyDate() { return Optional.ofNullable(this.myDate); } public void setMyDate(final Date myDate) { this.myDate = myDate; } } 

In this way,

• You bean remain serializable (optional not)
• You still include your "client" code to choose how to behave to the empty / null value of your property.
• Configure your sonar violation as false positive, as you need it instead of changing the code

Answer 2

You do not need to explicitly set null in your setter, just use the value passed this way ...

 public void setDate(Date date) { if (date != null) { this.date = new Date(date.getTime()); } else { this.date = date; } } 

Personally, I would never allow null values ​​to be in my Value objects, wherever they are, but this is just my rooted coding style.

My advice to someone is to give preference to objects of constant value, where you set all the values ​​in the constructor and don't allow the input of zeros. This style may not be acceptable for all third-party libraries that expect a java bean getter / so keep in mind where this can be used effectively to simplify your code.

Edit

If the above code still gives you a warning and you should have the property “property not yet set”, another approach is to define a “null object” like this

 public static final Date NO_DATE = new Date(Long.MIN_VALUE); public void setDate(Date date) { this.date = (date == null) ? NO_DATE : new Date(date.getTime()); } 

Users of this class can reference an NO_DATE object like this, which is still doing for readable code.

 if(toto.getDate() != NO_DATE) ... 

Or encapsulate it in another method so that it is used

 if(toto.hasDate()) ... 

Of course, this does not bring much benefit compared to Java 8. Optional approach from @kij, but it works with any version of Java

Answer 3

As a rule, when using static analysis tools to check the code, it is important not to blindly fix all the warnings that appear on you. You need to analyze the problem and see if it really applies in your context.

Now, to solve the problems you mention

Return a copy of the “date” and keep a copy of the “date”

It seems valid. It’s good practice to be a guard rather than show mutable state through getters / setters. Therefore, you must create a security copy in getter / setter. This can be done the way you did it, or using the new Java Time API, which provides immutable objects.

Assigning a null object is the smell of code. Consider refactoring

IMO is doubtful. The problem is with the PMD plugin (which is a code analysis tool, SonarQube displays a report). The problem arises because of this rule http://pmd.sourceforge.net/pmd-4.3.0/rules/controversial.html#NullAssignment , as you can see, this is a contradictory category. I don’t think that something is wrong with your code, and the correct action may be to ignore this warning and mark the problem as “do not fix”. You can also configure SonarQube not to use this rule in your quality settings.

Returns an empty collection instead of zero.

You did not specify the code that runs it, but this seems to be valid advice. It is usually better to return empty collections rather than zeros.