CODEWITHSUNDEEP
javaoop
RandomQuestion
RandomQuestion

Reputation: 6998

Better design for initialization within constructor

I have a class that looks something like this which acts as factory for clients based on credentials retrieved from credentials service. It builds clients one time and returns that on every call.

public class ClientFactory {
   private CredentialService credentialService;
   private ClientA clientA; 

   public ClientFactory(CredentialService credentialService){
      this.credentialService = credentialService;
      //initialization in constructor
      this.clientA = buildClientA(credentialService.getCredentials());
   }

   public ClientA getClientA(){
       return clientA;
   }

   /** Build new ClientA using crendentials*/
   private ClientA buildClientA(String credentials){
     return new ClientA(credentials);
   } 
}

Issue that I see with this is line 2 in the constructor which basically start using dependency 'credentialService' to initialize other dependencies immediately. If some other developer moves around order of code in constructor, it will start failing. Other option is too change method getClientA() to this.

   public ClientA getClientA(){
      if(clientA == null) {
        this.clientA = buildClientA(credentialService.getCredentials());
      }
       return clientA;
   }

But it has thread safety issues. Is there a better way to design above class which avoids concerns I highlighted above?

Thanks

Upvotes: 3

Views: 106

Answers (3)

jonderry
jonderry

Reputation: 23643

What you call ClientFactory looks like it's really just a ClientA. So why not have a factory that depends on the credentials and share a reference to a ClientA if you want to avoid creating extra ClientA objects?

public class ClientFactory {
   private final CredentialService credentialService;

   public ClientFactory(CredentialService credentialService){
      this.credentialService = credentialService;
   }

   public ClientA newClientA(String credentials){
     return new ClientA(credentials);
   } 
}

If you want to have a pool of clients potentially, you can share a Supplier<ClientA> instead.

Supplier<ClientA> simpleSupplier = Suppliers.ofInstance(ClientFactory.newClientA()); // guava
Supplier<ClientA> allNewSupplier = () -> ClientFactory.newClientA(); // java 8 lambda
ClientA cachedClient = simpleSupplier.get();
ClientA newClient = allNewSupplier.get();

Upvotes: 1

Eran
Eran

Reputation: 394146

Well,

this.clientA = buildClientA(credentialService.getCredentials()); relies on the parameter credentialService passed to the constructor, not on the member this.credentialService. Therefore, the order of the initializations doesn't matter.

BTW, just to be safe and avoid confustion, I wouldn't use the same name for the parameter of the constructor and the member.

Upvotes: 3

NESPowerGlove
NESPowerGlove

Reputation: 5496

Just don't save off a reference to theCredentialService as an instance variable, and continue passing credentialService.getCredentials() to bulldClientA. You are already not using the instance variable credentialService.

Upvotes: 1

Related Questions