5
\$\begingroup\$

I am in the process of using Java Builder design pattern to write a connector class that has optional member variables and need some suggestions. My question from here Standard practices for employing Java Builder design pattern has been updated and the code now presented is not psuedocode but is specific.

public class Connector {

    // Instance variables
    String appUrl = null;
    String oneTimePwd = null;
    String accessKey = null;

    // Private constructor
    private Connector(Builder builder) {
        this.appUrl = builder.appUrl;
        this.accessKey = builder.accessKey;
        this.oneTimePwd = builder.oneTimePwd;
    }

    public static class Builder {

        // Instance variables
        String appUrl = null;
        String accessKey = null;
        String oneTimePwd = null;

        public Builder() {
            // Empty constructor that accepts no arguments.
        }

        // Setter method for mandatory field/s
        public Builder setMandatoryParameters(String appUrl) {
            this.appUrl = appUrl;
            return this;
        }

        // Setter method1 for optional field/s
        public Builder setAccessKey(String accessKey) {
            this.accessKey = accessKey;
            return this;
        }

        // Setter method2 for optional field/s
        public Builder setOneTimePwd(String oneTimePwd) {
            this.oneTimePwd = oneTimePwd;
            return this;
        }

        public Connector Build() {
            if (accessKey != null) {
                return new Connector(this);
            } else if (oneTimePwd != null) {
                return new Connector(this);
            } else {
                // log.error("Either Access key or One time password must be specified.")
            }
            return null;
        }
    }

    // Other methods of the Connector class
    // ...
}

Question 1: Is it sensible to make use of a method "setMandatoryParameters()" to obtain values for mandatory parameters? This will allow creation of an instance of the Builder without supplying the mandatory parameters which can be a drawback. Is it much better to ask the user to supply the values for mandatory parameters with Builder's constructor?

Question 2: Either access key or one time password is needed to establish a successful connection. Thus, field validations such as null checks have been employed in the "Build()" method. Should such validations be performed in the Connector class's constructor instead?

\$\endgroup\$
  • 1
    \$\begingroup\$ If learning about the builder pattern is a major part of what you want from this code, then keep going as you are. If you mainly just want to have a good builder ready for other code to use, however, you may want to look into the Immutables library. \$\endgroup\$ – Douglas Dec 18 '19 at 19:28
3
\$\begingroup\$

Design improvements

Even with the Builder pattern, the target class shall only provide constructors that allow creation of valid objects. You still need two different constructors: one for constructing the connector with an access key and one for OTP.

The Builder's responsibility is to provide setters and to check at build time that both access key and OTP are not set at the same time and then call the correct constructor. It should never pass itself to the target class. Just the parameters.

SOLID

Your Connector does a lot of things. Instead of having one concrete Connector class for both access key and OTP, you should implement the Connector as an interface or abstract class and have the builder create an AccessKeyConnector or OtpConnector depending on the parameters that were provided to it.

Style

The setMandatoryParameters is a bad name for a method. It should communicate what the parameter does, not if it is mandatory or not. Use setAppUri instead.

| improve this answer | |
\$\endgroup\$
8
\$\begingroup\$

Question 1: If the parameters are mandatory anyway you might as well enforce this through your API. This reads just fine:

Connector con = new Connector.Builder(mandatoryURL)
                    .setAccessKey(key)
                    .build();

Question 2: Either place, since you contained the checks entirely in the Connector class (because of the private constructor) you have full control anyway.

Since either one of them is mandatory and you I'm assuming you won't use both of them, you might also provide 2 separate "build" methods that take one of those parameters as input.

public static class Builder {
    String appUrl = null;

    public Builder(String appUrl) {
        this.appUrl = appUrl;
    }

    public Connector withAccessKey(String accessKey) {
        Connector result = new Connector(appUrl);
        connector.accessKey = accessKey;
        return connector;
    }

    public Connector withOneTimePwd(String oneTimePwd) {
        Connector result = new Connector(appUrl);
        connector.oneTimePwd = oneTimePwd;
        return connector;
    }
}

Quick note that I changed the Connector constructor to also take the URL parameter instead of the builder. Your way works just fine in this case, I just prefer to have the builder be responsible for setting the correct fields instead of making the Connector "know" about the Builder class.


Alternative solution to using a Builder is to use factory methods. Your Connector class could then look something like this:

public class Connector {

    String appUrl = null;
    String oneTimePwd = null;
    String accessKey = null;

    private Connector(String appUrl) {
        this.appUrl = appUrl;
    }

    public static Connector createWithAccessKey(String appUrl, String accessKey) {
        Connector result = new Connector(appUrl);
        connector.accessKey = accessKey;
        return connector;
    }

    public static Connector createWithOneTimePwd(String appUrl, String oneTimePwd) {
        Connector result = new Connector(appUrl);
        connector.oneTimePwd = oneTimePwd;
        return connector;
    }

    //... rest of the class
}

You can now use it as follows:

Connector con = Connector.createWithAccessKey(someUrl, myKey);

This has about the same effect as your Builder idea but with a lot less boilerplate code to reach it.

| improve this answer | |
\$\endgroup\$
6
\$\begingroup\$

It is fine to create Builder without passing mandatory parameters in it's constructor. There can be cases, where you don't know all parameters at creation of builder, but you know them later before calling .build().

What I am really missing here is validation, that object will be in correct state at the beginning of build method. That method should imho always either create valid object or throw exception. You can also choose what kind of instance type to return afterwards as suggested by @JAD. But you seriously don't wanna deal with invalid or null objects returned by your builder.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ A very probable situation and a good reason for not asking for mandatory parameters in the Builder's constructor but, I can now safely say that in my scenario, there will never be a time wherein the user cannot-know the Url. As Imus suggested, I'll enforce the url in the constructor of the Builder. \$\endgroup\$ – simplisagar Dec 18 '19 at 8:11
  • \$\begingroup\$ Yep. Put as much as you can in constructor as you can, if you know, it will be known at builder creation time. \$\endgroup\$ – K.H. Dec 18 '19 at 8:19
4
\$\begingroup\$

One aspect that you are missing here is that most likely, the functionality of Connector will depend on whether it was passed a oneTimePwd or an accessKey. It could even have been passed both. What would happen then?

Imus' answer provides some good insights for the builder side of the code, but you can also leverage the other side. By turning Connector into an abstract class that defines its interface, you can create two separated implementations of that abstract class, depending on the type of key that was passed. One of the strengths of the builder pattern is that you can easily return either of those two types, without having to bother the user of the class with the choice between them:

    public Connector Build() {
        if (accessKey != null) {
            return new AccessKeyConnector(this);
        } else if (oneTimePwd != null) {
            return new OneTimePasswordConnector(this);
        } else {
            // log.error("Either Access key or One time password must be specified.")
        }
        return null;
    }
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ At first I didn't really see how the user of the class doesn't need to know the difference since they already know whether to pass a key or OTP. Then it clicked that the user doesn't need to know what the classes have to do with those things. Great complementary answer to mine! \$\endgroup\$ – Imus Dec 18 '19 at 7:52
  • \$\begingroup\$ That did cross my mind, and in case both the accessKey and oneTimePwd are supplied, I can probably default to using accessKey, and invoke the correct implementation of the Connector besides logging a message stating the use of accessKey in a case where both are provided. \$\endgroup\$ – simplisagar Dec 18 '19 at 8:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.