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

Question

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<String>.

---

import javax.ws.rs.core.MultivaluedMap;

private final DataClient dataClient = DataFactory.getInstance();
private static final int PETA_ID = 100;

public Response lookup(@Context UriInfo uriInfo) {
    Optional<String> client = TestUtils.getClientId();
    if (!client.isPresent()) {
        // logging here
        return Response
                .status(HttpServletResponse.SC_UNAUTHORIZED)
                .entity(Collections.singletonList(new DataResponse(null, new ResponseError(
                        HttpServletResponse.SC_UNAUTHORIZED, "client id is null or empty",
                        "FAILURE")))).build();
    }

    Map<String, String> inputParameters = TestUtils.convertMultiValueMapToRegularMap(uriInfo.getQueryParameters());
    if (MapUtils.isEmpty(inputParameters)) {
        // logging here
        return Response
                .status(HttpServletResponse.SC_BAD_REQUEST)
                .entity(Collections.singletonList(new DataResponse(null, new ResponseError(
                        HttpServletResponse.SC_BAD_REQUEST, "Input Keys are null",
                        "FAILURE")))).build();
    }

    String userId = inputParameters.remove("userid");
    String flowid = inputParameters.remove("flowid");
    String timeout = inputParameters.remove("timeout");
    String dataflag = inputParameters.remove("dataflag");
    String processflag = inputParameters.remove("processflag");
    String linkvtou = inputParameters.remove("linkvtou");
    String linkutou = inputParameters.remove("linkutou");
    Long startdate = Longs.tryParse(inputParameters.remove("startdate"));
    Long enddate = Longs.tryParse(inputParameters.remove("enddate"));
    Integer maxid = Ints.tryParse(inputParameters.remove("maxid"));
    String holdertype = inputParameters.remove("holdertype");
    List<String> linkHoldertype = new ArrayList<>();
    if (!TestUtils.isEmpty(holdertype)) {
        linkHoldertype = Arrays.asList(holdertype.split(","));
    }

    org.springframework.http.HttpHeaders reqHeader = new org.springframework.http.HttpHeaders();
    reqHeader.add("client_id", client.get());

    RequestKey requestKey = null;
    try {
        Builder builder = new RequestKey.Builder(PETA_ID)
                .addHeaders(new HttpEntity<Object>(reqHeader))
                .addTimeout((long) (Longs.tryParse(timeout) == null ? 200 : timeout))
                .dataFlag(TestUtils.isEmpty(dataflag) ? true : Boolean.parseBoolean(dataflag))
                .processFlag(TestUtils.isEmpty(processflag) ? true : Boolean.parseBoolean(processflag))
                .linkUToU(
                        TestUtils.isEmpty(linkutou) ? false : Boolean
                                .parseBoolean(linkutou))
                .linkVToU(
                        TestUtils.isEmpty(linkvtou) ? true : Boolean
                                .parseBoolean(linkvtou));

        if (!TestUtils.isEmpty(userid)) {
            builder.setUserId(userid);
        }
        if (!TestUtils.isEmpty(flowid)) {
            builder.setFlowId(flowid);
        }
        if (!TestUtils.isEmpty(startdate)) {
            builder.startDate(startdate);
        }
        if (!TestUtils.isEmpty(enddate)) {
            builder.endDate(enddate);
        }
        if (!TestUtils.isEmpty(maxid)) {
            builder.maxId(maxid);
        }
        if (!TestUtils.isEmpty(linkHolderType)) {
            builder.addHolderType(linkHolderType);
        }
        // now only attributes will be left in the map
        if (!TestUtils.isEmpty(inputParameters)) {
            builder.paramMap(inputParameters);
        }
        requestKey = builder.build();
    } catch (Exception ex) {
        // logging here
        return Response
                .status(HttpServletResponse.SC_BAD_REQUEST)
                .entity(Collections.singletonList(new DataResponse(null, new ResponseError(
                        HttpServletResponse.SC_BAD_REQUEST, ex.getMessage(), "FAILURE"))))
                .build();
    }

    List<HolderResponse> beResponse = dataClient.getUserDataSync(requestKey);

    boolean successFlag = false;
    List<DataResponse> responseList = new ArrayList<>();
    for (HolderResponse response : beResponse) {
        int errorCode = response.getError().getCode();
        if (errorCode == HttpServletResponse.SC_OK) {
            successFlag = true;
        }
        ResponseError error = new ResponseError(errorCode, response.getError().getStatus(), response.getStatus()
                .name());
        ResponseData data = new ResponseData(response.getResponse(), response.isLink(),
                                            response.getType(), response.getCreationTime(),
                                            response.getDay(), response.getInfo());
        DataResponse cosResponse = new DataResponse(data, successFlag ? null : error);
        responseList.add(cosResponse);
    }

    return Response.status(successFlag ? HttpServletResponse.SC_OK : HttpServletResponse.SC_INTERNAL_SERVER_ERROR)
            .entity(responseList).build();
}

convertMultiValueMapToRegularMap method:

public static Map<String, String> convertMultiValueMapToRegularMap(MultivaluedMap<String, String> input) {
    Map<String, String> map = new HashMap<String, String>();
    if (input == null) {
        return map;
    }
    for (Entry<String, List<String>> entry : input.entrySet()) {
        StringBuilder sb = new StringBuilder();
        for (String s : entry.getValue()) {
            if (sb.length() > 0) {
                sb.append(',');
            }
            sb.append(s);
        }
        map.put(entry.getKey(), sb.toString());
    }
    return map;
}

The RequestKey builder takes all these input parameters which has default values for all boolean variables and for all non boolean variables. If you try to set with empty, null values, it throws Exception, so that's why I am doing an empty, null check before setting it.

Also, if any response is successful with my array, it gives a 200 HTTP status code. If all the responses have an error, then it returns a 500 HTTP status code.

I'd like any improvements here. I am on Java 7 as of now.

Answer 1

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<String, String> 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<String, String> 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<String, String> 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<DataResponse> convertToResponseList(List<HolderResponse> beResponse) {
      // do transformation here
  }
  

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

public Response lookup(@Context UriInfo uriInfo) {
    Optional<String> client = TestUtils.getClientId();
    if (!client.isPresent()) {
        // return SC_UNAUTHORIZED here
    }
    Map<String, String> inputParameters = TestUtils.convertMultiValueMapToRegularMap(uriInfo.getQueryParameters());
    boolean valid = validateInputParameters(inputParameters);
    if (!valid) {
        // return SC_BAD_REQUEST here
    }
    RequestKey requestKey = convertToRequestKey(inputParameters);
    List<HolderResponse> beResponse = dataClient.getUserDataSync(requestKey);
    List<DataResponse> 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.