HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Sending a file in servlet response, using a class inside a method

Submitted by: @import:stackexchange-codereview··
0
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.

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:

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.