34

I'm working on a class which sends a RequestDTO to a Web Service. I need to validate the request before it is sent.

The request can be sent from 3 different places and there are different validation rules for each "requesttype", e.g. request1 must have name and phonenumber, request2 must have address, etc)

I have a DTO which contains a long list of fields (name, address, city, phonenumber, etc.) and it is the same DTO sent no matter which type of request it is.

I have created 3 different validation methods and based on the type the appropriate method is called.

In each of these methods I have a long list of if-else's to check for the fields that are necessary for each request type.

private void validateRequest1(Request request) {
    StringBuilder sb = new StringBuilder();
    if (null == request) {
        throw new IllegalArgumentException("Request is null");
    }
    if (isFieldEmpty(request.getName())) {  *see below
        sb.append("name,"));
    }
    if (isFieldEmpty(request.getStreet())) {
        sb.append("street,"));
    }
    ...

isFieldEmpty() checks the string for null and isEmpty() and returns a boolean

This gives me a cyclomatic complexity of 28 in one of those methods so my question is.. is it possible to reduce this complexity? - if so, how would I go about doing so?

Ultimately I need to check a lot of fields and I cannot see how this can be done without lots of checks :/

flavian
  • 28,161
  • 11
  • 65
  • 105
Linora
  • 10,418
  • 9
  • 38
  • 49
  • 1
    My idea would be: Use some sort of `FieldChecker` object that encapsulates the emptiness (or some other) check, and the action to be taken (`sb.append()`) etc, and loop over a list of such objects. This makes the code clearer since you have to explicitly define the outputs and inputs of that check. – millimoose May 07 '13 at 11:41
  • hello Herter, one question here: how you calculate complexity? – CodeSlave Mar 17 '21 at 10:09

3 Answers3

47

An easy way is to promote the check into a separate method:

private String getAppendString(String value, String appendString) {
    if (value == null || value.isEmpty()) {
        return "";
    }
    return appendString;
}

And then you can use this method instead of the if blocks:

sb.append(getAppendString(request.getStreet(), "street,");

This will reduce complexity from 28 down to 3. Always remember: high complexity counts are an indication that a method is trying to do too much. Complexity can be dealt with by dividing the problem into smaller pieces, like we did here.

EmirCalabuch
  • 4,756
  • 1
  • 25
  • 20
5

Another approach would be to enforce that contract in the Request object itself. If a field is required or can't be null, say so when the Request is created.

Create the Request in such a way that it's 100% valid and ready to go when the constructor exists.

I'd also create that String version in the Request toString() method. It should know how to render itself.

duffymo
  • 305,152
  • 44
  • 369
  • 561
-1

Decorate StringBuilder, add methods named appendIfHasValue.

philn5d
  • 636
  • 7
  • 12
  • This question is quite old and already has an accepted answer with a high score. Either way, a) subclassing is generally not recommended (prefer composition over inheritance) and b) `StringBuilder` is final and you couldn't subclass it if you wanted. – daniu Apr 10 '21 at 21:39
  • Ah, decorator in that case. Updated answer. You have qualms with adding information for alternative solutions? – philn5d Apr 11 '21 at 23:43
  • There's no issue with alternative solutions if they actually improve on the existing one, but I'd be double checking if it's necessary and not give a single sentence "solution" with three problems, one of them being that it just doesn't work. Which is also the case for "decorating" which is wrapping the implementation of an interface with another implementation - `StringBuilder` is not an `interface` though. Maybe try adding sample code to show how that would work? – daniu Apr 12 '21 at 08:04