Extracting various URL parameters in Rest Service and then passing it to Builder class

I have a service which takes a lot of key value parameters in a URL. Only a few are mandatory and others are optional. It's up to the customer whether they want to set those optional parameters or not.

The mandatory parameters are userid or flowid. They can be set together or independent of each other so below are the possible combinations:

• http://localhost:8080/base/v1/lookup?userid=12345

• http://localhost:8080/base/v1/lookup?flowid=abcdefgh

• http://localhost:8080/base/v1/lookup?userid=12345&flowid=abcdefgh

Now if the full URL is to be made along with other optional parameters, then the URL will look like this:

http://localhost:8080/base/v1/lookup?userid=12345&flowid=abcdefgh&timeout=200&dataflag=false&processflag=true&linkvtou=true&linkutou=false&maxid=3&startdate=12345&enddate=54321&holdertype=id1&holdertype=id2&attr=@kite&count=1&hide=5

Here are the parameters my URL will take and the same parameters I am setting it in my RequestKey builder class. My RequestKey builder class has default values for some optional fields like all boolean variables, maxid field, and timeout.

• UserId: This will always be a String (can be number as well).

• FlowId: This should always be a String.

• Timeout: This should be a long. By default this is 200 ms.

• DataFlag: This should be a boolean. By default this value is true if they are not passing.

• ProcessFlag: This should be a boolean. By default this value is true if they are not passing.

• LinkVToU: This should be a boolean. By default this value is true if they are not passing.

• LinkUToU: This should be a boolean. By default this value is false if they are not passing.

• MaxId: This will be a long. By default this is 5.

• StartDate: This will be a long.

• EndDate: This will be a long.

• HolderType: This is a List of String, List.

```
import javax.ws.rs.core.MultivaluedMap;

private final DataClient dataClient = DataFactory.getInstance();
pr

Here are some comments that I had when reading through your code:

• Avoid calling inputParameters.remove(key) just to retrieve the value for the given key. Not all maps support the remove operation, so you might end up with an UnsupportedOperationException. What you really want is to get the value for a key, so just call inputParameters.get(key).

• I'd refactor the code mapping the Map inputParameters into RequestKey object into a dedicated method: it would avoid having the code manipulating and the code transforming the request in the same place. This is generally a good practice: make your methods small, concise and to the point.

-
The builder logic is awkward:

if (!TestUtils.isEmpty(userid)) {
    builder.setUserId(userid);
}

surrounded by the try-catch mixes 3 differents concepts: validation, defaults and building. The code-smell here is the long try-catch: try-catch blocks should be as minimal as possible, catching the most specific exceptions. The longer they are getting, the more chance there is that things are not decoupled enough.

-
You should first validate all your input parameters: are the boolean (resp. number) values really booleans (resp. numbers)? Are the required parameters present? If one the parameter is incorrect then you can safely return a SC_BAD_REQUEST. I noticed in your current that you are in fact not validating that one of userid or flowid was present. We could then imagine a method containing that logic:

private boolean validateInputParameters(Map inputParameters) {
    // return true or false whether the parameters are correct or not

    // validate that either userid or flowid are set
    String userid = inputParameters.get("userid");
    String flowid = inputParameters.get("flowid");
    if (isEmpty(userId) && isEmpty(flowid)) {
        return false;
    }

    //validate that dataflag is a boolean
    String dataflag = inputParameters.get("dataflag");
    if (!"true".equals(dataflag) && !"false".equals(dataflag)) {
        return false;
    }

    // rest of validation ...

    return true; // everything was correct
}

Note that this could use other helper methods like isNumber that would determine if a String can be parsed into a number. The advantage is that if your requirements change in the future (for example, the timeout must be less than a certain number), you only need to change this method. Even for boolean parameters, there is an advantage: in the future, you might want to add support for "t" or "f".

-
Decoupling defaults with the building process is more trickier and context-dependent: is the builder supposed to know where the value it's using are coming from? Should it know the default values? At the very least, we can create helper methods like private boolean getBooleanParameterOr(String key, boolean defaultValue) that would convert a raw String into a boolean with a default value if the value is empty. Then, we can imagine a method:

private RequestKey convertToRequestKey(Map inputParameters) {
    boolean dataflag = getBooleanParameterOr(inputParameters.get("dataflag"), true);
    // the same for the rest of the parameters
    // ...
    builder.dataFlag(dataflag);
    // ...
    return builder.build();
}

-
Finally, the last part of the code is also converting a beResponse into a responseList. This could also be refactored into a dedicated method:

private List convertToResponseList(List beResponse) {
    // do transformation here
}

With all those comments, you could have the following main method:

public Response lookup(@Context UriInfo uriInfo) {
    Optional client = TestUtils.getClientId();
    if (!client.isPresent()) {
        // return SC_UNAUTHORIZED here
    }
    Map inputParameters = TestUtils.convertMultiValueMapToRegularMap(uriInfo.getQueryParameters());
    boolean valid = validateInputParameters(inputParameters);
    if (!valid) {
        // return SC_BAD_REQUEST here
    }
    RequestKey requestKey = convertToRequestKey(inputParameters);
    List beResponse = dataClient.getUserDataSync(requestKey);
    List responseList = convertToResponseList(beResponse);
    // return SC_OK or SC_INTERNAL_SERVER_ERROR response here
}

which I feel is clearer and separates each part of the code.

StackExchange Code Review Q#117706, answer score: 4