1
\$\begingroup\$

Can the below code be improved? Are there any major issues with it? Thanks

Writer w = client.sendMessageData();
for (File file : myAttachments) {
    w.append("\r\n").append("--").append(bound).append("\r\n");
    w.append("Content-Type: application/octet-stream\r\n");
    w.append("Content-Transfer-Encoding: base64\r\n");
    w.append("Content-Disposition: attachment; filename=\"" + Tools.getNameForAttach(file.getName()) + "\"\r\n\r\n");
    FileInputStream fileInputStream = new FileInputStream(file);
    ByteArrayOutputStream buffer = new ByteArrayOutputStream();

    int nRead;
    byte[] data = new byte[1024];

    while ((nRead = fileInputStream.read(data, 0, data.length)) != -1) {
        buffer.write(data, 0, nRead);
    }

    buffer.flush();

    fileInputStream.close();

    w.append(Base64.encodeBase64String(buffer.toByteArray()));
    w.append("\r\n");

}
w.close();
\$\endgroup\$
6
  • \$\begingroup\$ Is there any reason why you are not using the Java Mail API? Makes this sort of thing much easier... for (File f : myAttachments) {body.attachFile(f.getPath());} \$\endgroup\$
    – rolfl
    Nov 3, 2014 at 12:09
  • \$\begingroup\$ This is for an Android application and Java mail has a lot of dependencies which make app larger \$\endgroup\$
    – nLL
    Nov 3, 2014 at 12:20
  • \$\begingroup\$ Note that Apache Commons Mail API internally uses the javax.mail.* classes: MultiPartMail \$\endgroup\$
    – rolfl
    Nov 3, 2014 at 12:30
  • \$\begingroup\$ This is not Commons Mail but Commons Net with AuthenticatingSMTPClient \$\endgroup\$
    – nLL
    Nov 3, 2014 at 12:37
  • \$\begingroup\$ You need to include the relevant information in your question. While you are adding the details of your dependencies (put in a link to the API's you are using), you should also include the Base64 implementaiton you are using. \$\endgroup\$
    – rolfl
    Nov 3, 2014 at 12:40

1 Answer 1

1
\$\begingroup\$

SRP - single responsibility principle

The code is violating the SRP, as it does to many things.

It is

  1. adding/composing headers for the attachments
  2. reading files from disc

I would extract the composition of the headers to a separate method.

private String getHeaders() {
    StringBuilder sb = new StringBuilder();

    sb.append("\r\n").append("--").append(bound).append("\r\n");
    sb.append("Content-Type: application/octet-stream\r\n");
    sb.append("Content-Transfer-Encoding: base64\r\n");
    sb.append("Content-Disposition: attachment; filename=\"");

    return sb.toString();
}  

and would assign the result before the for loop to a String variable.

Next we extract the file reading to a separate method. I don't know if Android supports Try with resources so I don't use it here. We also rename the int nRead to bytesRead.

private byte[] readFile(File file) throws FileNotFoundException, IOException {
    FileInputStream fileInputStream = new FileInputStream(file);
    ByteArrayOutputStream buffer = new ByteArrayOutputStream();
    int bytesRead;
    byte[] data = new byte[1024];

    while ((bytesRead= fileInputStream.read(data, 0, data.length)) != -1) {
        buffer.write(data, 0, bytesRead);
    }

    buffer.flush();
    fileInputStream.close();

    return buffer.toByteArray();
}  

next, we add a method which takes a file and returns the content as base64 encoded string

private String getContentAsBase64(File file) throws FileNotFoundException, IOException {

    byte[] fileContent = readFile(file);

    return Base64.encodeBase64String(fileContent);
}  

After using the fluent append() method instead of the string adding using +, this leads to the former code beeing refactored to

Writer w = client.sendMessageData();
String attachmentHeader = getAttachmentHeader();
for (File file : myAttachments) {
    w.append(attachmentHeader);
    w.append(Tools.getNameForAttach(file.getName()));
    w.append("\"\r\n\r\n");
    w.append(getContentAsBase64(file));
    w.append("\r\n");
}
w.close();
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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