CODEWITHSUNDEEP
javasonarqubegetter-setter
Corporativo
Corporativo

Reputation: 185

Solve Sonar issue in simple getter and setter methods with Date or List

I write this getter/setter to list from Eclipse source menu:

public Date getDate() {
    return date;
}

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

And Sonar reporting two issues:

Return a copy of "date" & Store a copy of "date"

with the explanation

"Mutable members should not be stored or returned directly"

and a example code:

public String [] getStrings() {
    return strings.clone();}

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

I think if my Date is null, it will throw a NullPointerException. Then I've 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 marks other issue:

"Assigning an Object to null is a code smell. Consider refactoring".

I've searched in internet and set or return a new array is not a solution for me, I want to preserve my list to null if the setter param is null to overwrite an existing previous list.

I've the same problem for List, and I want to return/preserve null instead of a new ArrayList for an empty List. And in this case, the setter marks one more issue:

"Return an empty collection instead of null.".

What is the solution for this issue?

Upvotes: 11

Views: 19955

Answers (3)

Tibor Blenessy
Tibor Blenessy

Reputation: 4420

Generally, while using static analysis tools to verify the code is valuable, you should not blindly fix every warnings which popups on you. You need to analyze the issue which is triggered and check if it really applies in your context.

Now to address the issues you are mentioning

Return a copy of "date" & Store a copy of "date"

This seems to be valid one. It is good practice to be defensive and not expose mutable state via getters/setters. So creating a defensive copy in getter/setter should be done. This can be done the way you did it, or by using new Java Time API, which provides immutable objects.

Assigning an Object to null is a code smell. Consider refactoring

IMO dubious one. The issue is raised by PMD plugin (which is the tool analyzing the code, SonarQube is displaying the report). Issue is raised by this rule http://pmd.sourceforge.net/pmd-4.3.0/rules/controversial.html#NullAssignment , as you can see it is in controversial category. I don't think there is anything wrong with your code, and proper action might be to ignore this warning and mark the issue as "won't fix". You can also configure your SonarQube to not use this particular rule in your Quality Profile setting.

Return an empty collection instead of null.

You did not provide the code which is triggering it, but this seems to be a valid piece of advice. It is generally better to return empty collections rather than nulls.

Upvotes: 3

kij
kij

Reputation: 1441

If you are in Java 8 and do not want to handle empty date, then maybe usage of Optional would help you.

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);
    }

}

Example of code usage:

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 to change the toto.setMyDate(...) with concrete date value to see what happen.

If you don't know what is Optional or how to use it, you can find plenty of examples.

BUT : This is only a way to solve your violation issue and i totally agree with Brad's remark, Optional are not intent to be used as a type, but more like a contract for potential empty / null returns. In general, you should not correct your code in a bad way just to fix a violation, if the violation is not correct. And in your case i think you should just ignore the violation (as most of Sonar's one unfortunatly)

If you really want to use Java 8 and Optional in your code, then you POJO class would be like this (usage of Optional as a contrat on the getter only)

public class Toto {


    public Date myDate;

    public Optional<Date> getMyDate() {
        return Optional.ofNullable(this.myDate);
    }

    public void setMyDate(final Date myDate) {
        this.myDate = myDate;
    }

}

This way,

  • You bean stay serializable (Optional is not)
  • You still enable your "client" code to have the choice on how to behave to empty / null value of your property
  • Configure your Sonar violation as a false positive as it is what you want instead of changing your code

Upvotes: 3

Brad
Brad

Reputation: 15879

You don't have to explcitly set null in your setter, just use the value being passed in like this...

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 into my Value objects where ever possible, but that is just my opinionated coding style.

My advice to anyone is to prefer immutable value objects where you set all the values in the constructor and don't allow nulls in. This style may not be appropriate for all 3rd party libraries that expect the java bean getter/setters so be aware where it can be used effectively to simplify your code.

Edit

If the above code still gives you the warning and you must have the "property is not set yet" functionality, 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 refer to the NO_DATE object like this, which still makes for readable code

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

Or encapsulate this into another method so it's used like this

if(toto.hasDate()) ...

Of course this doens't add much benefit over the Java 8 Optional approach from @kij but it does work with any version of Java

Upvotes: 2

Related Questions