Omit private constructors in Java from rule "Methods should not have too many parameters" (squid:S00107)

[andreas....@storytel.com]

The rule "Methods should not have too many parameters" (squid:S00107) complains if the number of arguments to methods and/or constructors is too large.

However, for Java, private constructors are typically used with the Builder pattern, and in this case this rule is not applicable. This is for example used to build up major data objects that are later serialised to xml or json.

I suggest that private constructors are removed from the analysis for this rule. If someone points me in the right direction (where rules are specified) and you also think that this rule is invalid for private constructors, I an happy to do a pull request.

Example:

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;

@JsonDeserialize(builder = Oauth3rdActivateRequestVO.Builder.class)
public class Oauth3rdActivateRequestVO {
    private String uniqueId;
    private String redirectUri;
    private String state;
    private String code;
    private String channelId;
    private String userId;
    private String portalId;
    private String applicationId;
    private String loginUser;
    private String loginPassword;
    private String referenceId;
    private String transactionId;
    private String tag;

    @SuppressWarnings("squid:S00107") // Too many parameters. Should be OK for private constructors
    private Oauth3rdActivateRequestVO(String uniqueId, String redirectUri, String state, String code, String channelId, String userId, String portalId, String applicationId, String loginUser, String loginPassword, String referenceId, String transactionId, String tag) {
        this.uniqueId = uniqueId;
        this.redirectUri = redirectUri;
        this.state = state;
        this.code = code;
        this.channelId = channelId;
        this.userId = userId;
        this.portalId = portalId;
        this.applicationId = applicationId;
        this.loginUser = loginUser;
        this.loginPassword = loginPassword;
        this.referenceId = referenceId;
        this.transactionId = transactionId;
        this.tag = tag;
    }

    public String getUniqueId() {
        return uniqueId;
    }

    public String getRedirectUri() {
        return redirectUri;
    }

    public String getState() {
        return state;
    }

    public String getCode() {
        return code;
    }

    public String getChannelId() {
        return channelId;
    }

    public String getUserId() {
        return userId;
    }

    public String getPortalId() {
        return portalId;
    }

    public String getApplicationId() {
        return applicationId;
    }

    public String getLoginUser() {
        return loginUser;
    }

    public String getLoginPassword() {
        return loginPassword;
    }

    public String getReferenceId() {
        return referenceId;
    }

    public String getTransactionId() {
        return transactionId;
    }

    public String getTag() {
        return tag;
    }

    @JsonPOJOBuilder(withPrefix = "")
    @JsonIgnoreProperties(ignoreUnknown = true)
    public static class Builder {
        private String uniqueId;
        private String redirectUri;
        private String state;
        private String code;
        private String channelId;
        private String userId;
        private String portalId;
        private String applicationId;
        private String loginUser;
        private String loginPassword;
        private String referenceId;
        private String transactionId;
        private String tag;

        public Builder uniqueId(String uniqueId) {
            this.uniqueId = uniqueId;
            return this;
        }

        public Builder redirectUri(String redirectUri) {
            this.redirectUri = redirectUri;
            return this;
        }

        public Builder state(String state) {
            this.state = state;
            return this;
        }

        public Builder code(String code) {
            this.code = code;
            return this;
        }

        public Builder channelId(String channelId) {
            this.channelId = channelId;
            return this;
        }

        public Builder userId(String userId) {
            this.userId = userId;
            return this;
        }

        public Builder portalId(String portalId) {
            this.portalId = portalId;
            return this;
        }

        public Builder applicationId(String applicationId) {
            this.applicationId = applicationId;
            return this;
        }

        public Builder loginUser(String loginUser) {
            this.loginUser = loginUser;
            return this;
        }

        public Builder loginPassword(String loginPassword) {
            this.loginPassword = loginPassword;
            return this;
        }

        public Builder referenceId(String referenceId) {
            this.referenceId = referenceId;
            return this;
        }

        public Builder transactionId(String transactionId) {
            this.transactionId = transactionId;
            return this;
        }

        public Builder tag(String tag) {
            this.tag = tag;
            return this;
        }

        public Oauth3rdActivateRequestVO build() {
            return new Oauth3rdActivateRequestVO(uniqueId, redirectUri, state, code, channelId, userId, portalId, applicationId, loginUser, loginPassword, referenceId, transactionId, tag);
        }
    }
}

[Nicolas Peru]

Hi, 

Allow me to disagree, in the case of the builder pattern, you should not have a private constructor with all the properties of the object but rather passing the builder to the private constructor. 

so something like : 

  public Oauth3rdActivateRequestVO build() {
            return new Oauth3rdActivateRequestVO(this);
        }

Therefore I don't think this exclusion would be relevant and I think it would just hide bad smells in code. 

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/1997ab57-c573-4fa6-83c5-e2116e9048e3%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Nicolas PERU | SonarSource

Senior Developer
http://sonarsource.com

https://twitter.com/benzonico

[andreas....@storytel.com]

You are absolutely right...

Shame on me. :-) I'm in a new code base introducing Sonar, and I just didn't see this badly used builder pattern. But you are of course correct!

[Andreas Mandel]

Hi Nicolas,

unfortunately in this case you create a direct dependency cycle between the 2 classes....

Kind Regards,

Andreas.

[andreas....@storytel.com]

I think this is alright since you put the builder class as an inner class to the POJO.

Using this pattern (with a bidirectional dependency) is the suggested method in Effective Java from Bloch.

However, if you use the built in Refactor method in IntelliJ named "Replace Constructor with Builder...", IntelliJ (ultimate 2016.1) will create a new (outer) class for the builder, and sending the arguments one by one the the pojos constructor. I can assume that the code I attached to the first message in this thread was actually the result of such an automated IntelliJ refactoring. And IntelliJ shows no shame in the amount of parameters. ;-) Also IntelliJ keeps the constructor public although it could have been protected or possibly with no modifier.

I guess it would be possible for SonarQube to detect this scenario and make a more detailed solution description... If someone agrees what is good practice and what is not. ;-)

/Andreas L

[Andreas Mandel]

Hi Andreas,

I missed the inner class topic. However I prefer a outer class solution since it does not "pollute" the immutable pojo. 

Might be the rule should focus on "public" methods?

Kind Regards,

Andreas.

[andreas....@storytel.com]

That was kind of my initial proposal, that private constructors should be omitted.

But like Nicolas pointed out, there is no need for several arguments since you can pass the builder object itself. You can do this in both scenarios, when the builder is an inner class a the constructor is private, or when the builder is a regular class and the constructor is non-private. I'm not sure why IntelliJ generates the builder/constructor pattern with all arguments, maybe since it is a refactoring and it is too tedious to automatically change all code that already uses the constructor. Thus, to keep the code backward compatible, it passes all the parameters instead on the builder object itself. Maybe that's the reason...

I think that a good thing to do is to let SonarQube give a hint of a proper builders pattern when constructors with too many arguments are found.

BR,

Andreas L

[Andreas Mandel]

Hi Andreas,

might be because they weight it to be a less severe smell to have many arguments than to have a cyclic dependency. As written above I would also prefer to have not trace of, or dependency to the builder class in the pojo. 

Kind Regards,

Andreas.

[andreas....@storytel.com]

To me, that fact that the specific pojo _only_ can be constructed with the Builder is a positive thing. Once the object is constructed, there is no longer any trace of the builder. But I guess that is a matter of taste.