S1186 empty constructor

[pplo...@gmail.com]

Hi,

since the update to the latest version of the Java plugin S1186 includes empty cosntructors. 

I want to suggest to disable this behavior. Unfortunately a lot of our classes need to have default constructors. 

There is also a discussion on Stackoverflow: http://stackoverflow.com/questions/32618188/sonar-methods-should-not-be-empty-at-the-constructor

regards

Philipp

[Brian Sperlongano]

Can you give an example where an empty constructor would be required?

[pplo...@gmail.com]

Hi,

we are using CDI constructor injection in all parts of our application.

Unfortunately some components need an additional default constructor such as JAX-RS endpoints and EJBs.

so it usually looks something like this:

class SomeClass {

   public SomeClass(){
   }

   @Inject
   public SomeClass(SomeOtherClass object){
 ....
}

[Adam Gabryś]

Hi,

Why don’t you add a comment?

 

This is compilant:

 

class SomeClass {

   public SomeClass(){

      // required by JAX-RS
   }

   @Inject
   public SomeClass(SomeOtherClass object){
 ....
}

 

Regards,

    Adam Gabryś

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/d00f0100-be9d-49ec-9c45-e18b90ecb7cb%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[pplo...@gmail.com]

Actually I was not precise before. 

Because of the javadoc Puplic-API rule we add the comment as a Javadoc comment like:

class SomeClass {

   /**

    * required by JAX-RS

    */
   public SomeClass(){
   }

   @Inject
   public SomeClass(SomeOtherClass object){
 ....
}

[G. Ann Campbell]

I'd suggest you use exclusions to prevent that rule from running on the packages in question.

Ann

[sea...@gmail.com]

So the recommended approach for empty default constructors is a comment? Seems silly to require essentially a do nothing comment to make it compliant. Rule makes sense for me outside of empty constructors which I would think is a more valid use case compared to other methods

[G. Ann Campbell]

Empty default constructors are provided by... default. So on the face of it, adding them is completely redundant.

Okay, so Jax-RS requires them to be explicit. Are you using any other marker on these default-constructor-required classes that would identify them as being used by Jax-RS?

---

G. Ann CAMPBELL | SonarSource

Product Owner

http://sonarsource.com

--
You received this message because you are subscribed to a topic in the Google Groups "SonarQube" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sonarqube/xXGqFn42Azg/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/02223b7d-1a12-4145-a5a9-319a77291aa9%40googlegroups.com.

[Sean Carroll]

Default was the wrong word...should have said no-arg. Completely understand that in the case in which you have only one constructor which is the no-arg I can understand the reason for the sonar issue as its provided by default. I'm referring to the use case in which there is more than one constructor and one is no-arg Jax-RS or not. Adding a do nothing comment seems unnecessary in that case.

[Michel Pawlak]

Hi,

Indeed adding "does nothing" is completely useless, but adding "required by JAX-RS" or "required by JAXB" can help your colleagues better understand your code, and avoid wasting their time (should I remove this dead code ? it seems unused.)

Michel

[Nicolas Peru]

Hi all, 

I would +1 Michel on this. This is an empty method, has that been forgotten during a refactor or does it serves a purpose ? It is one of the rare case where a comment might make sense for future reader (IMO to overcome some flaws of the framework here but that's another debate). 

Cheers, 

You received this message because you are subscribed to the Google Groups "SonarQube" group.

To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/f9a33c41-15d1-4246-9962-c34b4dbebd90%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Nicolas PERU | SonarSource

Senior Developer
http://sonarsource.com

https://twitter.com/benzonico

[Michel Pawlak]

Hi again,

A cleaner / smarter approach could be to annotate your constructor with @Deprecated (and add the @deprecated tag to your existing javadoc.) @Deprecated is a good candidate here because the constructor only exists for technical reasons, it may disappear / change anytime (in DDD wording, it is part of the "infrastructure".) Using this annotation you tell your colleagues that the constructor should not be directly used (they can even see it in their IDE), you keep your existing comment and do not need to add anything else than an annotation and a javadoc tag, you can enforce easily at compile time that the constructor is not used by your code (or detect it in SQ if you prefer), and S1186 should not complain that the constructor is empty (I hope it won't complain, if it does this should be fixed IMHO)

Michel

[Sean Carroll]

I appreciate the feedback and I think Nicolas is right concerning JAX-RS and JAX-B, or at least I can see value in adding comments for that scenario but taking those out of the conversation for a moment about about other instances in which you  have a class that has multiple constructors; one being the no-arg. 

To try and help illustrate what about a sonar example what about RuleParamDto. Nothing wrong with the code and I do rather like it. Typical pattern I see is that createFor is which sets the rule then I some variations. 

Name, type, description, default value are set such as in 

https://github.com/SonarSource/sonarqube/blob/66e51209252064e2528825895d8f9e391332e727/server/sonar-server/src/test/java/org/sonar/server/rule/ws/UpdateActionMediumTest.java#L126

Name and type only are set in

https://github.com/SonarSource/sonarqube/blob/9c4e7a049ce145694c76307dcb6868be4fd9e8d2/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileComparisonMediumTest.java#L69

An alternative approach that I do not think is uncommon would be to provide 3 constructors. No-arg, name/type, name/type/description/default

This is a better solution? Probably not. I like what you have better but its an alternative. In this instance, is a comment necessary in the no-arg constructor and if so what value does it provide?

Perhaps an example that isn't as contrived might be the following. Is a comment really necessary here? What value would it provide?  

https://github.com/spring-projects/spring-framework/blob/master/spring-jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java#L159

Also, from your personal experience(s) do you find the following comment is helpful to you or other colleagues? 

https://github.com/SonarSource/sonarqube/blob/master/sonar-db/src/main/java/org/sonar/core/permission/GlobalPermissions.java#L47

Above information might not be helpful but wanted to at least provide some feedback.

Appreciate your time and the product

Sean

[Sean Carroll]

Oops in my contrived alternative example for RuleParamDto I forgot to include the ruleId in the constructors outside of the no-arg one

[andreas....@storytel.com]

Here is a code example when this rule gets very annoying. I would say that this is a perfectly regular piece of code. Even if the default constructor is there by misstake, it is not a MAJOR issue:

public static class Builder {
    private long id;
    private String firstName;
    private String lastName;
    private String email;

    /**
     * Creates a builder used for cloning an existing user
     * @param user User to clone
     */
    public Builder(User user) {
        this.id = user.id;
        this.firstName = user.firstName;
        this.lastName = user.lastName;
        this.email = user.email;
    }

    /**
     * Creattes a Builder used for creating a new user from scratch
     */
    public Builder() {}

// Rest of the builder...

[Nicolas Peru]

Hi Andreas, not sure if i'm following you here : is your suggestion to change this rule severity ? 

Cheers, 

--

You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/7f901455-ccdd-45da-a953-3460b1bd4e28%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[andreas....@storytel.com]

The previous discussion ended with people thinking that this was sort of a JAX-RS issue, that it was still a valid rule. Suggestions like putting comments that "Needed for JAX-RX" should be a valid solution. I don't agree. I think that having an empty no-args constructor is not an issue at all if there also exist another constructor with arguments. If people don't agree, maybe you agree that it's at least not a major issue...

Den tisdag 17 november 2015 kl. 12:08:59 UTC+1 skrev pplo...@gmail.com:

[jeanchrist...@sonarsource.com]

The reason why it is critical and why it will stay critical is that it is a serious maintainability issue that can be very easily solved by the proper comment.

Some day, maybe soon, someone, maybe even the initial author, will look at this and say "This is useless, let's clean it" and will break something.

So, in our view, it is critical to be nice to future maintainers and explain, with a comment, why this is empty and why it shouldn't be removed.

Hope this makes sense.

[Andreas Lundgren]

In Java language, an empty constructor is not useless. It's like saying that an ignorant developer may remove methods with no return value because he does not understand that they may work by side effects. Should Sonar require a comment explaining to future develops not to remove those functions too?

If you remove a used constructor (empty or not) it will most likely result in compilation error (or startup error if a bean). I don't agree that this is a major issue, or even an issue at all.

[daniel...@gmail.com]

This rule is extremely useful for empty methods or even constructors with args, but it does not make any sense to apply it for no-args constructors.

You're required to add no-arg constructor if you (use it and) add a constructor with args.

That's a Java rule, not a code smell.

And if the constructor is required because of some framework, the developers would have to add the comment in every single file. That does not make the comment helpful. It only makes it extremely repetitive.

This validation should not exist or at least not be in the same rule as the empty methods.

Maybe we could separate it into two different rules (but to be honest, I would always disable it).

[karl....@gmail.com]

This rules is great for methods but is pretty useless for constructors if the project uses Java EE with CDI, JPA, JAX-RS etc. Also empty constructors are not so bad and the JDK has many of them. Have you never used new ArrayList<>()? Have you never used new ArrayList<>(collection)? 

It is a poor rule that needs to be separated from empty methods because they are fundamentally different in the language 

[Nicolas Peru]

Hi Karl, 

(always appreciated to use greeting around here).

Not sure I am following you : the constructor with no arguments from JDK ArrayList is not empty.

As discussed previously in this thread : if  a public empty constructor is required, why shouldn't it deserve a comment ? 

Cheers, 

--

You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/e20b2c7a-aa1f-47f8-8ba8-f8c69c887407%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Karl Kildén]

Hi!

Thanks for the reply.

If empty constructor is viable API design then designing it that way should be considered fine. ArrayList might initialize a field but the case I am making is that the api consumer expects defaults. This is a language design since fields can have default values.

The implementation might be satisfied with having all fields being defaulted (ie null/0,false etc) in the empty constructor and the reader of the code will assume that something needs this defaulted instance.

For me to put hundreds of comments across the code base will be awful for because comments have their drawbacks as well (Clean Code covers this pretty well). Rather everyone in our project knows that a class with @ApplicationScoped or @Entity needs to comply with Java Bean spec and the comments are simply dead.

Since this is and will continue to be a common concern it seems wise to separate constructors from methods?

On 27 January 2017 at 08:42, Nicolas Peru <nicola...@sonarsource.com> wrote:

Hi Karl, 

(always appreciated to use greeting around here).

Not sure I am following you : the constructor with no arguments from JDK ArrayList is not empty.

As discussed previously in this thread : if  a public empty constructor is required, why shouldn't it deserve a comment ? 

Cheers, 

Le jeu. 26 janv. 2017 à 11:05, <karl....@gmail.com> a écrit :

This rules is great for methods but is pretty useless for constructors if the project uses Java EE with CDI, JPA, JAX-RS etc. Also empty constructors are not so bad and the JDK has many of them. Have you never used new ArrayList<>()? Have you never used new ArrayList<>(collection)? 

It is a poor rule that needs to be separated from empty methods because they are fundamentally different in the language 

On Tuesday, November 17, 2015 at 12:08:59 PM UTC+1, pplo...@gmail.com wrote:

Hi,

since the update to the latest version of the Java plugin S1186 includes empty cosntructors. 

I want to suggest to disable this behavior. Unfortunately a lot of our classes need to have default constructors. 

There is also a discussion on Stackoverflow: http://stackoverflow.com/questions/32618188/sonar-methods-should-not-be-empty-at-the-constructor

regards

Philipp

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.

To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/e20b2c7a-aa1f-47f8-8ba8-f8c69c887407%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[philipp.a....@gmail.com]

Hello Group!

I am still wondering why we dont have an option to ignore empty constructors.

In our code, there are >500 issues because of that rule.

90% because of empty default constructors.

I am not going to add a comment to those 450 constructors.

More likely i will disable the rule and ignore the real 50 issues.

Have a nice day.

Looking forward to the upcoming updates of SonarQube!

You build a great tool.

[Amaury Leve]

Hi philipp,

It is in the pipe! Depending on which language plugin you are talking about the fix is either already delivered or will come on the next release.

Have a nice day!

--

You received this message because you are subscribed to the Google Groups "SonarQube" group.

To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/8a47bd18-d7e0-483f-85cf-4d36b108fc6b%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Amaury LEVE | SonarSource
Software Developer

http://sonarsource.com

[Amaury Leve]

Hi again,

My bad I should have double checked before sending this. We did some work around S1172 (unused function parameters should be removed) on empty methods...

Cheers

[sea...@gmail.com]

Can you point me to the one related to java?

[G. Ann Campbell]

Hi,

We've added this exception: https://jira.sonarsource.com/browse/SONARJAVA-2220

Ann

[Philipp Steinwender]

Thank you very much!

Looking forward to the update.

Is there a roadmap page, that tells the upcoming release dates?

When will SonarJava 4.8 be ready for download?

[G. Ann Campbell]

Hi,

You can always check Jira: https://jira.sonarsource.com/browse/SONARJAVA/?selectedTab=com.atlassian.jira.jira-projects-plugin:versions-panel

:-)

Ann

---

G. Ann CAMPBELL | SonarSource

Product Manager

http://sonarsource.com