current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

Below is my Interface:

public interface IDBClient {
    public String read(ClientInput input);
}

This is my implementation of the interface:

public class DatabaseClient implements IDBClient {

    @Override
    public String read(ClientInput input) {

    }
}

Now I have a factory which gets the instance of DatabaseClient like this:

IDBClient client = DatabaseClientFactory.getInstance();
....

Now I need to make a call to read method of my DatabaseClient which accepts the ClientInput parameter and below is the class for the same. This class was not written by me so that is the reason I am having a question on this and I am pretty much sure this is the wrong way of doing it.

public final class ClientInput {

    private Long userid;
    private Long clientid;
    private Long timeout_ms = 20L;
    private boolean debug;
    private Map<String, String> parameterMap;

    public ClientInput(Long userid, Long clientid, Map<String, String> parameterMap, Long timeout_ms, boolean debug) {
        this.userid = userid;
        this.clientid = clientid;
        this.parameterMap = parameterMap;
        this.timeout_ms = timeout_ms;
        this.debug = debug;
    }
}

So when customer make a call to read method of DatabaseClient, they will create the ClientInput parameter like this and then use the factory to get the Instance of DatabaseClient and then call the read method accordingly.

Map<String, String> paramMap = new HashMap<String, String>();
paramMap.put("attribute", "segmentation");

ClientInput input = new ClientInput(109739281L, 20L, paramMap, 1000L, true);

IDBClient client = DatabaseClientFactory.getInstance();
client.read(input);

Is this the right way of creating the ClientInput and then passing it to the read method? If not, then what's the best way of doing it? Where should I be doing validation check to see whether the customer has passed valid user IDs or valid client ID, etc.?

share|improve this question

2 Answers 2

up vote 2 down vote accepted

I have no major concern with setting the values of ClientInput via the constructor. What is your concern with this anyway? My minor concerns are:

  1. There are already three arguments of the Long type, it may not clear to other developers which position is for which field (especially during those long nights...).
  2. If more inputs are required, it will make the constructor declaration longer.

Based on the name, I will prefer validation to be done within ClientInput. Oh, and it should have getter methods that your IDBClient implementations can use, and I'm guessing that's already the case as the fields are private.

On a related note, how else will ClientInput be used by IDBClient, besides the read() method?

share|improve this answer
    
Thanks hjk.. ClientInput will be used only to retrieve the various input parameters and use them to make a call and it will be used only at that place.. And I agree with both of your issues.. How can I avoid first issue? Should it be long instead of Long object. And regarding your second issue, do you think is there any way of avoiding this issue by some better way? – david Jan 10 '14 at 5:40
    
For the first issue, making it long or Long doesn't matter since the compiler will perform auto-boxing, and only a smart IDE (with the right settings) will prompt you about that. For both issues, consider just using setter methods for ClientInput. However, if you really go down this path, then you should re-evaluate why do you need a wrapper class just to 'hold' (layman terms) a bunch of input parameters together. You should consider @rolfl's answer too, if that's the case. – h.j.k. Jan 10 '14 at 7:19
up vote 3 down vote

Like most times, it is best to validate your data before you use it. In your case, since the input data needs to be validated by the Database infrastructure, I recommend adding it to the IDBClient interface.

I also assume that the paramMap is the query you want to run, and not part of the user/login information.

I suspect the logical thing to do is to extend your interface to look like:

public interface IDBClient {

    public interface SessionHandle extends AutoCloseable, Closeable {
        public boolean isAlive();
    }

    public SessionHandle createSession(Long userid, Long clientid,
              Long timeout_ms, boolean debug) throws InvalidLoginException;

    public String read(SessionHandle session, Map<String, String> parameterMap)
              throws ReadNotPossibleException, SessionExpiredException;
}

Now you are forced to create a SessionHandle instance that is pre-verified, and immutable. The content of that SessionHandle is whatever it needs to be for your actual IDBClient implementaton. The IDBClient implementation will know which handles are valid, and can do other things with the handle as needed.

Because times are good, and Java7 is better, you should also ensure that your SessionHandle is AutoCloseable so you can use try-with-resources.

Your Exceptions (which you should create custom versions of) for your interface should all extend a single Exception class, for example:

public DBClientException extends Exception { ..... }
public ReadNotPossibleException extends DBClientException {....}
public SessionExpiredException extends DBClientException {...}

by doing this, you can allow the user to catch just the database exception, or more-specific database exception sub-types if needed....

The 'user experience' will look like:

IDBClient client = DatabaseClientFactory.getInstance();
try (IDBClient.SessionHandle session =
          client.createSession( 109739281L, 20L, 1000L, true) ) {

    String result = client.read(session, paramMap);

} catch (DBClientException e) {
    .....
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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