Servlet file uploader

Question

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";
 }

Answer 1

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
}

Answer 2

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);
 }
}