2
\$\begingroup\$

I have a method which does URL decoding on the string value passed along with using Charset:

  public String decodeValue(String value, Charset charset) {
    if (!Strings.isNullOrEmpty(value)) {
      try {
        value = URLDecoder.decode(value, charset.name());
      } catch (UnsupportedEncodingException ex) {
        // log error
        return null;
      }
    }
    return value;
  }

Now if URLDecoder.decode throws UnsupportedEncodingException the first time, then I want to run the same value against these three lines:

value = value.replaceAll("%(?![0-9a-fA-F]{2})", "%25");
value = value.replaceAll("\\+", "%2B");
value = URLDecoder.decode(value, charset.name());

And if then again URLDecoder.decode line throws exception second time, then I will log the error but only second time and return null value otherwise return the value which is decoded so I came up with this:

  public String decodeValue(String value, Charset charset) {
    if (!Strings.isNullOrEmpty(value)) {
      try {
        value = URLDecoder.decode(value, charset.name());
      } catch (UnsupportedEncodingException ex) {
        try {
          value = value.replaceAll("%(?![0-9a-fA-F]{2})", "%25");
          value = value.replaceAll("\\+", "%2B");
          value = URLDecoder.decode(value, charset.name());
        } catch (UnsupportedEncodingException uex) {
          // log error
          return null;
        }
      }
    }
    return value;
  }

I'd like to see if there is any better way to write the above code. As of now, it duplicates the code.

\$\endgroup\$

bumped to the homepage by Community 7 hours ago

This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.

0
\$\begingroup\$

Check this can be any help.

public String decodeValue( String value, Charset charset ) {

    if ( !Strings.isNullOrEmpty( value ) ) {
        try {
            return URLDecoder.decode( value.replaceAll( "%(?![0-9a-fA-F]{2})", "%25" ).replaceAll( "\\+", "%2B" ), charset.name() );
        } catch ( UnsupportedEncodingException ex ) {
            // log error
        }
    }
    return null;
}

or another possible solution I've is

public String decodeValue( String value, Charset charset ) {

    if ( !Strings.isNullOrEmpty( value ) ) {
        try {
            return URLDecoder.decode( value, charset.name() );
        } catch ( UnsupportedEncodingException ex ) {
            try {
                return URLDecoder.decode( value.replaceAll( "%(?![0-9a-fA-F]{2})", "%25" ).replaceAll( "\\+", "%2B" ), charset.name() );
            } catch ( UnsupportedEncodingException uex ) {
                // log error
            }
        }
    }
    return null;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and how it improves upon the original) so that the author can learn from your thought process. \$\endgroup\$ – Heslacher Jan 2 '17 at 16:20
0
\$\begingroup\$

First, let's move the replacements into a separate fixErrors method for readability.

String fixErrors(String value) {
    return value
        .replaceAll("%(?![0-9a-fA-F]{2})", "%25")
        .replaceAll("\\+", "%2B");
}

Now, my solution would be to write a method that returns the result of URLDecoder.decode as an Optional. Then, instead of nesting try/catch statements, we can just use Optional::or. This should look something like

public String decodeValue(String value, Charset charset) {
    if (!Strings.isNullOrEmpty(value)) {
        return tryDecode(value, charset)
            .or(() -> tryDecode(fixErrors(value), charset))
            .orElse(null);
    }
    return value;
}

Optional<String> tryDecode(String value, Charset charset) {
    try {
        return Optional.of(URLDecoder.decode(value, charset.name()));
    } catch (UnsupportedEncodingException ex) {
        return Optional.empty();
    }
}

Unfortunately, this solution precludes logging details of only the second error. To do this effectively, we could add error handling to tryDecode by passing it a Consumer<UnsupportedEncodingException>. Then we would have:

public String decodeValue(String value, Charset charset) {
    if (!Strings.isNullOrEmpty(value)) {
        return tryDecode(value, charset, ex -> {})
        .or(() -> tryDecode(fixErrors(value), charset, ex -> {
                // log ex
            }))
        .orElse(null);
    }
    return value;
}

Optional<String> tryDecode(String value, Charset charset,
                   Consumer<UnsupportedEncodingException> exConsumer) {
    try {
        return Optional.of(URLDecoder.decode(value, charset.name()));
    } catch (UnsupportedEncodingException ex) {
        exConsumer.accept(ex);
        return Optional.empty();
    }
}

Hopefully this solution meets your requirements.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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