current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 8 down vote favorite

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.

share|improve this question
    
I'm curious, what is the purpose of the return value consisting of an empty or null string? – 200_success♦ May 13 '15 at 18:45
up vote 7 down vote accepted

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;
    }
}
share|improve this answer
    
Well I originally had this method inside utility class. And then I decided to split the functions because I thought that copying streams and setting headers should be in different methods... – user1432980 May 13 '15 at 12:26
    
I can see your logic, and the copy could be in a different method, but the method does not need to be declared inside your method, it can be somewhere else appropriate. Also, the copy sort if is inside it's own method, the method: IOUtils.copy(...) – rolfl May 13 '15 at 12:28
    
no need to have return value in public static String downloadFile(..) if its just dummy value or null – Zavael May 14 '15 at 10:41
up vote 5 down vote

I mostly agree with rolfl, however, let me say that what you did is not wrong, just unconventional (with "unconventional" meaning "strange" and "strange" meaning "possibly hard to understand").

I'd also avoid it because of the two added levels of nesting (which lead to more broken lines and more ugliness).

But what I'd do is just this:

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 setHeader(HttpServletResponse response) {
    response.setHeader(HEADER_KEY, String.format(HEADER_VALUE, TEMPLATE_FILE_NAME));
}

public static void processResponse(HttpServletResponse response) throws IOException {
    byte[] bytes = fileBody.getBytes();
    IOUtils.copy(new ByteArrayInputStream(bytes), response.getOutputStream());
    setHeader(response);
}
  • I won't return a dummy value unless really required to.
  • I let the method throw instead of returning null, so that the caller can decide what to do.
  • By throwing the exception, I don't let the caller ignore the problem.
  • Neither can it forget the null check and die on NPE.
share|improve this answer
    
+1 for void and avoiding null – Zavael May 14 '15 at 10:37
up vote 5 down vote 
    public void processResponse(HttpServletResponse response) throws IOException {
        byte[] bytes = fileBody.getBytes();
        IOUtils.copy(new ByteArrayInputStream(bytes), response.getOutputStream());
        setHeader(response);
    }

I'd rewrite this as follows, reasons after that:

public static void processResponse(final HttpServletResponse response, final String content) {
    try (OutputStream stream = response.getOutputStream()) {
        setHeader(response);
        stream.write(content.getBytes());
        stream.flush(); // commits response!
    } catch (IOException ex) {
        // clean error handling
    }
}

  1. This gets rid of the dependency on IOUtils. I have a personal dislike against Apache Libraries, since they often hide implementation details in the wrong places and are generally rather massive for the functionality they bring.

  2. This gets rid of the new ByteArrayInputStream(bytes), that's completely unneeded.

  3. The response should be flushed after you completed working with it. This commits the response and makes any further accesses to response writing throw an IllegalStateException. You should do this to prevent tampering with the data you wrote to the Response already.

  4. After flushing the output stream now gets closed by the try-with-resources statement. This is useful to prevent memory leakage.

  5. This also makes the class you created moot, reasons are already outlined by rolfl in his excellent answer

Depending on whether you need the response outside of this method (and I hope you don't), you can drop the try-with-resources block and use it somewhere higher in the calling hierarchy

In any way, your method should still document the assumptions it makes. This is overall missing in the code, and should definitely be added...

share|improve this answer
    
i think try-with-resources is only available from java 7, would be fine to mention it if true, but its a nice feature – Zavael May 14 '15 at 10:40
    
@Zavael since Java 6 (and thusly JEE6) are officially EOL'd since 2013 I think it should be safe to assume Java7, which btw. is not updated publicly anymore since April this year... – Vogel612 May 14 '15 at 10:54
    
oh ok, but i think there is still a good market share fo java 6 in production – Zavael May 14 '15 at 11:15

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.