4
\$\begingroup\$

I have an instance of a Java class that I need to call some functions on, using the API exposed by another party. For each call, I need to both check for thrown exceptions, and check the value of the returned boolean. If the boolean is false, I need to read the errorMsg object to see what went wrong. In either error case, I need to throw an exception with a relevant method. Each method takes a different number of String arguments

I think there's a cleaner way to do this using reflection, but I can't figure out how to handle the different numbers of arguments. And there may be a totally different solution I've overlooked. Open to all suggestions. Any thoughts?

Here are two examples so you can see what I'm dealing with:

ClassProvidedWithExposedAPI s;

private void setHttp(JsonObject jo) throws Exception {
    boolean success = true;
    String errorString = null;
    ArrayList<Message> errorMsg = new ArrayList<Message>();
    try {
        JsonObject http = jo.getAsJsonObject("http");
        if (http != null && !http.isJsonNull()) {
            success = s.setHttp(getJsonElementAsString(http, Http.ENABLED_ELEMENT),
                    getJsonElementAsString(http, Http.HTTP_PORT_ELEMENT),
                    getJsonElementAsString(http, Http.HTTPS_PORT_ELEMENT),
                    getJsonElementAsString(http, Http.TIMEOUT_ELEMENT),
                    getJsonElementAsString(http, Http.HTTP_REDIRECT_ELEMENT), errorMsg);
        }

    } catch (Exception e) {
        errorString = e.toString();
    } finally {
        if (!success) {
            if (errorString == null) {
                errorString = errorMsg.toString();
            }
            throw new Exception("Exception in setHttp: " + errorString);
        }
    }
}

private void setSsh(JsonObject jo) throws Exception {
    boolean success = true;
    String errorString = null;
    ArrayList<Message> errorMsg = new ArrayList<Message>();
    try {
        JsonObject ssh = jo.getAsJsonObject("ssh");
        if (ssh != null && !ssh.isJsonNull()) {
            success = s.setSsh(getJsonElementAsString(ssh, Ssh.ENABLED_ELEMENT),
                    getJsonElementAsString(ssh, Ssh.SSH_PORT_ELEMENT),
                    getJsonElementAsString(ssh, Ssh.TIMEOUT_ELEMENT), errorMsg);
        }
    } catch (Exception e) {
        errorString = e.toString();
    } finally {
        if (!success) {
            if (errorString == null) {
                errorString = errorMsg.toString();
            }
            throw new Exception("Exception in setSsh: " + errorString);
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! I hope you get some good answers. \$\endgroup\$ – Phrancis Apr 6 '16 at 17:11
2
\$\begingroup\$

Sounds like a simple use case for polymorphism:

interface JsonOperation {
    void run();
}

private void setHttp(JsonObject jo) {
    runAndHandleErrors(() -> {
        // ...
    });
}

private void setSsl(JsonObject jo) {
    runAndHandleErrors(() -> {
        // ...
    });
}

private void runAndHandleErrors(JsonOperation op) {
    try {
        op.run();
    } catch (Exception e) {
        // ...
    } finally {
        // ...
    }
}

Explanation: JsonOperation is an interface with one method so it is considered a functional interface, meaning a lambda can be considered an implementation of it. runAndHandleErrors takes a JsonOperation so whenever we call runAndHandleErrors with a lambda, the compiler infers the lambda to implement JsonOperation.

A lambda, like an anonymous inner class, captures final variables in enclosing blocks. That means that a lambda, like an anonymous inner class, can use these variables and sees the values of the variables that were assigned when the lambda / anonymous inner class was created. A lambda also captures variables that are effectively final, meaning their values are never changed whether or not they are declared as final.

When setHttp(x) is called, the following happens:

  1. x is assigned to jo
  2. an implementation of JsonOperation capturing jo is created (but not run)
  3. runAndHandleErrors is called and the JsonOperation from step 2 is assigned to op
  4. runAndHandleErrors calls op.run(), causing the JsonOperation to run
  5. the JsonOperation runs, doing whatever it wants with jo - it sees the same value of jo as was assigned in step 2

However, since you posted to the Code Review Stack Exchange and not Programmers Stack Exchange, I'll critique your code:

  • catch Exception and throws Exception are code smells - to the greatest degree possible, catch and declare specific Exception types.

  • The logic regarding success and errorString is convoluted. You could instead separately throw an appropriate subclass of Exception in the try and catch blocks and get rid of the success and errorString variables. Your code reuse isn't actually helping because you need conditional logic in the finally block to account for both cases. Also, the thrown exception in the catch block should properly wrap the original exception by passing the latter directly into the former's constructor.

  • Make a helper method for getJsonElementAsString that takes a varargs array of attribute keys and returns a list of attributes. This method would make your code much terser.

\$\endgroup\$
  • \$\begingroup\$ I like where this is going, but I'm not too familiar with the use of lambdas. Can you explain to me how this all hangs together? How do I get the JsonOperations inside of runAndHandleErrors()? Do the I write the s.setHttp(...) call in a Class that implements JsonOperation? Thanks for the critique, and my bad if I posted this in not-quite-the-correct-stack-exchange. \$\endgroup\$ – PunDefeated Apr 6 '16 at 17:26
  • 1
    \$\begingroup\$ I will explain the lambdas in my post as it provides a better environment for extended description than the comments and it is permanent, unlike comments. \$\endgroup\$ – Solomonoff's Secret Apr 6 '16 at 17:28
  • \$\begingroup\$ If I apply these changes (or any others) am I supposed to post the updated code? \$\endgroup\$ – PunDefeated Apr 6 '16 at 19:22
  • 1
    \$\begingroup\$ @PunDefeated Only if you seek additional feedback, in which case I suggest you post another question. \$\endgroup\$ – Solomonoff's Secret Apr 6 '16 at 21:02

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.