4
\$\begingroup\$

I have this below code where i am sending some content over a POST and then consuming the response. I would like to know if there can be any optimizations that can be done and is there any possibility of connection leak.

import java.io.*;
import java.net.HttpURLConnection;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.util.Calendar;

class Test implements Serializable {

    static String getR(String xml, String requestUrl, int retryWaitTime, int socketTimeOut) throws IOException {
        long startTime = Calendar.getInstance().getTimeInMillis();
        StringBuilder xmlString = new StringBuilder();
        String out;
        HttpURLConnection connection = null;
        try {

            URL url = new URL(requestUrl);
            connection = (HttpURLConnection) url.openConnection();
            connection.setConnectTimeout(retryWaitTime);
            connection.setReadTimeout(socketTimeOut);
            connection.setDoInput(true);
            connection.setDoOutput(true);
            connection.setRequestMethod("POST");
            connection.setRequestProperty("Accept", "application/xml");
            connection.setRequestProperty("Content-Type", "application/xml; charset=UTF-8");
            OutputStreamWriter writer = new OutputStreamWriter(connection.getOutputStream(), StandardCharsets.UTF_8);
            writer.write(xml);
            writer.close();

            int statusCode = connection.getResponseCode();
            if (statusCode == HttpURLConnection.HTTP_OK) {
                BufferedReader br = new BufferedReader(new InputStreamReader(connection.getInputStream()));

                String line;
                while ((line = br.readLine()) != null) {
                    xmlString.append(line);
                }

                br.close();

               out = xmlString.toString();


            } else {

                out = statusCode + "^" + xml;
            }
            if (connection.getInputStream() != null)
                connection.getInputStream().close();

        }
        catch (Exception e) {

            out = "X^" + xml;
            assert connection != null;
            if (connection.getErrorStream() != null)
                connection.getErrorStream().close();
        }
        String Response = out.replace("\n", "").replace("\r", "");
        return Response;
    }

}
\$\endgroup\$
  • \$\begingroup\$ Well, the assert probably won't work in production code. \$\endgroup\$ – markspace Nov 15 '19 at 16:13
  • \$\begingroup\$ We can give better reviews when more of the code is provided, is there any way you could provide the entire class? \$\endgroup\$ – pacmaninbw Nov 15 '19 at 16:18
  • \$\begingroup\$ @pacmaninbw I have updated the code \$\endgroup\$ – ben Nov 15 '19 at 16:29
  • \$\begingroup\$ @markspace, what do you suggest ? \$\endgroup\$ – ben Nov 15 '19 at 16:37
1
\$\begingroup\$

Star imports are discouraged. It's preferable to specify what classes you're importing in most cases.

getR is a very poor method name unless R is well defined inside the domain you're operating in. It's preferable to clearly express, without abbreviations, what the method will be doing. get is also misleading because it implies a simple local operation to retrieve a value from a class, not a remote call.

When variables will not be reassigned, it's preferable to mark them as final. This expresses design intent and tells the reader they don't have to worry about the value of the variable changing.

Minimizes scope wherever possible. getR should probably be either private or public. Package-private seems unlikely to be correct.

startTime is never used and can be removed. Rather than creating a Calendar instance, System.currentTimeMillis() would be preferable to find the current time.

If getR needs a URL, it should probably take it as a parameter, rather than accepting a String. In particular, your error handling of a bad URL is highly dubious. The caller gets no notification of any type that the URL string was incorrect. Make them build a correct URL and handle the case where they can't before the getR call.

There's an argument to be made that setDoInput(true) is noise, and one to be made that it makes things clearer.

The only exception your method throws is if the error stream can't be closed. It's unclear what you expect the caller to be able to do about that.

Use try-with-resources where possible to leverage automatic closing of resources. Otherwise you need to always close resources in finally blocks or you risk exceptions preventing you from closing them.

Always use curly braces, even if they're not required. You prevent an annoying and difficult-to-find class of error, and you enhance readability in most cases.

In idiomatic java, catch belongs on the same line as }.

Always catch the most specific type of exception possible. Catching Exception may silently swallow current or future exceptions you didn't intend to handle.

Using assert to ensure invariants in non-production environments is a good practice, but note that assertions must be engaged at both compile time and run time to function. Most production environments won't (and shouldn't) have assertions enabled. Many development environments and compilations also won't have assertions enabled, so make sure that they're correctly activated if you intend to use them.

It's unclear to me why you're explicitly closing the error stream in that fashion, since you never need or use it otherwise. If you must close it, put it in a try-with-resources block.

You should almost certainly log the exception you're handling for debugging later.

Early returns can clarify the code. The only one return location paradigm is a holdover from when it was not possible to embed returns in the middle of a block of code. A helper method could do the replacements for you.

In idiomatic java, variables start with a lowercase letter. return would be preferable to Return.

Turning the if check into a guard clause would reduce indentation and simplify the logic.

As of Java 9, I believe (but you should confirm) that string concatenation is actually faster than using a StringBuilder in the simple case of appending strings to one another. I think the readability is a wash.

If you were to make all these changes, your code might look more like:

private static String getR(
        final String xml,
        final URL requestUrl,
        final int retryWaitTime,
        final int socketTimeOut) {

    final StringBuilder xmlString = new StringBuilder();
    HttpURLConnection connection = null;

    try {
        connection = (HttpURLConnection) requestUrl.openConnection();
        connection.setConnectTimeout(retryWaitTime);
        connection.setReadTimeout(socketTimeOut);
        connection.setDoInput(true);
        connection.setDoOutput(true);
        connection.setRequestMethod("POST");
        connection.setRequestProperty("Accept", "application/xml");
        connection.setRequestProperty("Content-Type", "application/xml; charset=UTF-8");

        try (final OutputStreamWriter writer =
                new OutputStreamWriter(connection.getOutputStream(), StandardCharsets.UTF_8)) {
            writer.write(xml);
        }

        final int statusCode = connection.getResponseCode();
        if (statusCode != HttpURLConnection.HTTP_OK) {
            return sanitize(statusCode + "^" + xml);
        }

        try (final BufferedReader br = new BufferedReader(new InputStreamReader(connection.getInputStream()))) {
            String line;
            while ((line = br.readLine()) != null) {
                xmlString.append(line);
            }
        }
        return sanitize(xmlString.toString());

    } catch (final IOException e) {
        e.printStackTrace();
        return sanitize("X^" + xml);
    }
}

private static String sanitize(final String string) {
    return string.replace("\n", "").replace("\r", "");
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the review @Eric. \$\endgroup\$ – ben Nov 17 '19 at 18:33

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.