6
\$\begingroup\$

I think this code looks ugly, especially with the multiple try-catch blocks, but I don't know how to rewrite it:

public String handleFileUpload(@RequestParam String url,@RequestParam String fileName, RedirectAttributes redirectAttributes, HttpServletResponse response) {
        InputStream inputStream;
        try {
             inputStream =  new URL(url).openStream();
        } catch (IOException e) {
            LOG.error("Cannot open stream by url=[{}]", url);
            try {
                response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "Cannot open stream by url=" + url);
            } catch (IOException e1) {
                LOG.error("Cannot send error");
            }
            return null;
        }
        try {
            File file = File.createTempFile("tmp", ".txt", new File(System.getProperty("user.dir")));
            byte[] binary = IOUtils.toByteArray(inputStream);
            FileUtils.writeByteArrayToFile(file, binary);
            UploadedMultipartFile multipartFile = new UploadedMultipartFile(file, file.length(), "jpg",
                    "formParameter", fileName);
            MultipartFileWrapper multipartFileWrapper = new MultipartFileWrapper();
            multipartFileWrapper.setMultipartFile(multipartFile);
            redirectAttributes.addFlashAttribute(multipartFileWrapper);
        } catch (IOException e) {
            LOG.error("Cannot save file [{}] from [{}]",fileName, url);
            try {
                response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "Cannot save file " + fileName);
            } catch (IOException e1) {
                LOG.error("Cannot send error");
            }
            return null;
        }
        return "redirect:/member/uploadImage";
    }
\$\endgroup\$
11
\$\begingroup\$

Two try blocks in a method is a smell IMO: your method is trying to do two things, which implies it's not doing one and only one thing.

Let's see:

  • We open a stream from a given url and if all goes well we have an inputStream to work with.
    • Or things go bad, and we log an error, and then try to send an error response before we return null - if returning the error response failed, we log and still return null.
  • We use the stream to create a new file, and return a hard-coded string on success, or null on failure.

That alone looks like a bit of reusable functionality that should be extracted into its own method: one whose responsibility would be to send an error response:

void sendErrorResponse(HttpServletResponse response, string message) {
    try {
        response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, message);
    } catch(IOException e) {
        LOG.error("An error occurred while sending error response: " + e.toString());
    }
}

Next, you need a function that takes a url and gives you an inputStream.

InputStream openStreamFromUrl(string url, HttpServletResponse response) {
    InputStream result;
    try {
        result = new URL(url).openStream();
    }
    catch(IOException e) {
        string message = "Cannot open stream for url=" + url;
        LOG.error(message);
        sendErrorResponse(response, message);
    }
    return result;
}

So calling this openStreamFromUrl function either returns a valid and opened InputStream, or a null reference. This means we can now do this:

public String handleFileUpload(@RequestParam String url,@RequestParam String fileName, RedirectAttributes redirectAttributes, HttpServletResponse response) {
    InputStream inputStream = openStreamFromUrl(url, response);
    if (inputStream == null) {
        return null;
    }

    // handle file upload
}
\$\endgroup\$
6
\$\begingroup\$

Try-with-resources are a great way to isolate and also nest exception handling. In your case, a nested try-catch would be best.

Additionally, a function to handle returning the error-state to the client would help a lot too.

Consider the following:

private static final String handleError(HttpServletResponse response, String message) {

    LOG.error(message);
    try {
        response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, message);
    } catch (IOException e1) {
        LOG.error("Cannot send error");
    }
    return null;

}

Then use that as follows (note, read the InputStream before opening the writer... it avoids unnecessary work in the event the input stream fails part way through...:

public String handleFileUpload(@RequestParam String url, @RequestParam String fileName, RedirectAttributes redirectAttributes, HttpServletResponse response) {
    try (InputStream inputStream = new URL(url).openStream();) {

        byte[] binary = IOUtils.toByteArray(inputStream);

        try {
            File file = File.createTempFile("tmp", ".txt", new File(System.getProperty("user.dir")));
            FileUtils.writeByteArrayToFile(file, binary);
            UploadedMultipartFile multipartFile = new UploadedMultipartFile(file, file.length(), "jpg",
                    "formParameter", fileName);
            MultipartFileWrapper multipartFileWrapper = new MultipartFileWrapper();
            multipartFileWrapper.setMultipartFile(multipartFile);
            redirectAttributes.addFlashAttribute(multipartFileWrapper);

            return "redirect:/member/uploadImage";
        } catch (IOException e) {
            return handleError(response, "Cannot save file " + fileName + " from " + url);
        }

    } catch (IOException e) {
        return handleError(response, "Cannot open stream by url=" + url);
    }
}
\$\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.