patternjavaMinor
Sending a file in servlet response, using a class inside a method
Viewed 0 times
filemethodresponsesendingusingservletclassinside
Problem
I am processing file inside my method - basically preparing it for the download.
But I decided to split actions for setting headers and reading bytes.
As Java does not support method declaration inside methods, I decided to go with class inside method.
But I am not sure whether the idea I've come up with is good or bad. Basically there is no reason for method separation because they are not doing a lot of work.
But I decided to split actions for setting headers and reading bytes.
As Java does not support method declaration inside methods, I decided to go with class inside method.
public static String downloadFile(final String fileBody, HttpServletResponse response) {
class FileDownloader {
private static final String HEADER_KEY = "Content-Disposition";
private static final String HEADER_VALUE = "attachment; filename=\"%s\"";
private static final String TEMPLATE_FILE_NAME = "filename.html";
private void setHeader(HttpServletResponse response) {
response.setHeader(HEADER_KEY, String.format(HEADER_VALUE, TEMPLATE_FILE_NAME));
}
public void processResponse(HttpServletResponse response) throws IOException {
byte[] bytes = fileBody.getBytes();
IOUtils.copy(new ByteArrayInputStream(bytes), response.getOutputStream());
setHeader(response);
}
}
try {
new FileDownloader().processResponse(response);
return StringUtils.EMPTY;
} catch (IOException e) {
e.printStackTrace();
return null;
}
}But I am not sure whether the idea I've come up with is good or bad. Basically there is no reason for method separation because they are not doing a lot of work.
Solution
Yeah..... no. Not a good idea.... ;-)
You are using a class to do the job of a method. The real question is why you feel you need to declare the method inside your other method (for the record, it is highly unconventional to declare a formal class inside a method too... an anonymous class is common, but a formal class is odd)
You really just want to have a private-static method that does the work for you... something like:
then your code becomes:
which in turn, probably means the functional extraction is too much... how about just:
You are using a class to do the job of a method. The real question is why you feel you need to declare the method inside your other method (for the record, it is highly unconventional to declare a formal class inside a method too... an anonymous class is common, but a formal class is odd)
You really just want to have a private-static method that does the work for you... something like:
private static final String HEADER_KEY = "Content-Disposition";
private static final String HEADER_VALUE = "attachment; filename=\"%s\"";
private static final String TEMPLATE_FILE_NAME = "filename.html";
private static void processResponse(HttpServletResponse response, String fileBody) throws IOException {
byte[] bytes = fileBody.getBytes();
IOUtils.copy(new ByteArrayInputStream(bytes), response.getOutputStream());
response.setHeader(HEADER_KEY, String.format(HEADER_VALUE, TEMPLATE_FILE_NAME));
}then your code becomes:
public static String downloadFile(final String fileBody, HttpServletResponse response) {
try {
processResponse(response, fileBody);
return StringUtils.EMPTY;
} catch (IOException e) {
e.printStackTrace();
return null;
}
}which in turn, probably means the functional extraction is too much... how about just:
public static String downloadFile(final String fileBody, HttpServletResponse response) {
try {
byte[] bytes = fileBody.getBytes();
IOUtils.copy(new ByteArrayInputStream(bytes), response.getOutputStream());
response.setHeader(HEADER_KEY, String.format(HEADER_VALUE, TEMPLATE_FILE_NAME));
return StringUtils.EMPTY;
} catch (IOException e) {
e.printStackTrace();
return null;
}
}Code Snippets
private static final String HEADER_KEY = "Content-Disposition";
private static final String HEADER_VALUE = "attachment; filename=\"%s\"";
private static final String TEMPLATE_FILE_NAME = "filename.html";
private static void processResponse(HttpServletResponse response, String fileBody) throws IOException {
byte[] bytes = fileBody.getBytes();
IOUtils.copy(new ByteArrayInputStream(bytes), response.getOutputStream());
response.setHeader(HEADER_KEY, String.format(HEADER_VALUE, TEMPLATE_FILE_NAME));
}public static String downloadFile(final String fileBody, HttpServletResponse response) {
try {
processResponse(response, fileBody);
return StringUtils.EMPTY;
} catch (IOException e) {
e.printStackTrace();
return null;
}
}public static String downloadFile(final String fileBody, HttpServletResponse response) {
try {
byte[] bytes = fileBody.getBytes();
IOUtils.copy(new ByteArrayInputStream(bytes), response.getOutputStream());
response.setHeader(HEADER_KEY, String.format(HEADER_VALUE, TEMPLATE_FILE_NAME));
return StringUtils.EMPTY;
} catch (IOException e) {
e.printStackTrace();
return null;
}
}Context
StackExchange Code Review Q#90641, answer score: 8
Revisions (0)
No revisions yet.