code review exercise for PCA 2014

gistfile1.java

package com.somecompany.app.process;

import java.awt.image.BufferedImage;
import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.FileOutputStream;
import java.io.InputStream;
import java.io.OutputStream;

import javax.imageio.ImageIO;

public class PersonPictureProcess extends APPProcess {

	public static byte[] getImage(String id) throws Exception {
		try {
			byte[] imageData = null;

			String filePath = System.getProperty("app.imageRepository")
					+ getFrameworkContext().getPrincipal().getTenant().getId()
					+ "/" + id + ".png";

			File file = new File(filePath);

			if (file.exists()) {
				BufferedImage image = ImageIO.read(new File(filePath));
				ByteArrayOutputStream baos = new ByteArrayOutputStream();
				ImageIO.write(image, "png", baos);
				imageData = baos.toByteArray();
			}

			return imageData;
		} catch (Exception e) {
			throw e;
		}
	}

	public static void setImage(String id, InputStream inputStream,
			String mimeSubType) throws Exception {
		try {
			String filePath = System.getProperty("app.imageRepository")
					+ getFrameworkContext().getPrincipal().getTenant().getId()
					+ "/" + id + "." + mimeSubType;

			File file = new File(filePath);
			file.getParentFile().mkdirs();
			OutputStream outpuStream = new FileOutputStream(new File(filePath));
			int read = 0;
			byte[] bytes = new byte[1024];
			outpuStream = new FileOutputStream(new File(filePath));
			while ((read = inputStream.read(bytes)) != -1) {
				outpuStream.write(bytes, 0, read);
			}
			outpuStream.flush();
			outpuStream.close();
		} catch (Exception e) {
			throw e;
		}
	}

	public static void deleteImage(String id) throws Exception {
		try {
			String filePath = System.getProperty("app.imageRepository")
					+ getFrameworkContext().getPrincipal().getTenant().getId()
					+ "/" + id + ".png";

			File file = new File(filePath);
			file.delete();
		} catch (Exception e) {
			throw e;
		}
	}
}

Answer the following:
This code was checked in as DONE on a project as part of a web services solution for saving images. All tests for the code are included.
Do you see any issues with the code below?
If so, please list them and any suggestions on how to improve.

Only the base Exception class is used and no corrective actions are taken except to throw up one level. May as well just let the Exception bubble on its own.
deleteImage will throw an exception if the file is not a .png file or will erroneously delete image.png when told to delete image.jpeg

getImage only deals with .png as well.

.png files are heavy and their use should be avoided in web applications. Use jpeg instead if possible.
The following code appears 3 times and should be refactored into a separate function:

String filePath = System.getProperty("app.imageRepository")
                    + getFrameworkContext().getPrincipal().getTenant().getId()
                    + "/" + id + ".png";

Best Practices

• getImage and deleteImage are hard coded to png images, but setImage allows the user to specify the mimeSubType. They should all allow the user to specify the mimeSubType to provide a consistent interface.
• In deleteImage, the code isn't checking if the file exists before attempting to delete it.
• The name of the system property "app.imageRepository" is hard coded and duplicated. It should be defined as a class or application level constant.
• There are no tests. Tests should be written in a separate file.

Clean Code

• The method setImage is hard to read due to lack of line breaks.
• There are a few places where code can be extracted into it's own method (2 examples below).
• The lines 18-22, 40-44, and 62-66 can be extracted into a helper method... protected File getFile(String id, String mimeSubType).

String filePath = System.getProperty("app.imageRepository")
        + getFrameworkContext().getPrincipal().getTenant().getId()
        + "/" + id + ".png";

File file = new File(filePath);

• Also the code at lines 24-29 could be extracted into a method since it is performing a concise task (getting the image data for a file).

if (file.exists()) {
    BufferedImage image = ImageIO.read(new File(filePath));
    ByteArrayOutputStream baos = new ByteArrayOutputStream();
    ImageIO.write(image, "png", baos);
    imageData = baos.toByteArray();
}

Naming Conventions

• line 26: variable "baos" is not descriptive, it's an acronym, and it's not consistent with how other variables are named. Perhaps name it "byteArrayOutputStream" or just "outputStream" instead
• All Exceptions are named "e". Single letter variable names are not descriptive. Should be named "exception".
• The base class APPProcess casing is not consistent. It should be named AppProcess to be consistent with UpperCamelCase.
• The class names PersonPictureProcess and APPProcess are vague. Should be more descriptive.

Exceptions

• The methods aren't doing anything with caught exceptions and just rethrowing them.
• Exception detail is being exposed to web service subscribers, which may be undesirable. Especially if the web service is public facing.
• Generic Exceptions are being thrown from the web service, which don't provide any information as to what caused the exception.
• Only base Exceptions are being caught instead of catching several types of exceptions and handling them each appropriately.

Exception Solutions

•   Catch different types of exceptions and handle them appropriately (such as IOException or FileNotFoundException).
  

• Throw exceptions that are web service specific and provide the necessary exception detail while hiding the internal Exception.
• Log the Exception.

Wow, not much to add. +1 on adding tests, extracting the filePath code, handling exceptions properly and renaming baos. The only other thing I saw was that outpuStream is misspelled and is re-initialized unnecessarily.

I found the page at the attached URL interesting: http://examples.javacodegeeks.com/enterprise-java/rest/jersey/jersey-file-upload-example/

Exact same misspelling of outpuStream and improper double initialization of the same.

In setImage:

• On line 49 (or 46), outpuStream should be constructed using the "file" variable from line 44 instead of constructing a new File.
• If an exception is thrown, outpuStream will not be closed. It should be closed in a finally block.

+1 to Andy's comment about throwing service-appropriate exceptions. My current application is failing a security scan in part because it exposes implementation details in exception messages that bubble all the way up to the user interface.

12
What class level comment could provide the reader context for PersonPictureProcess?
Would ImageFileManipulation more accurately describe this class?
What does APPProcess do? How do we benefit from having subclassed it?

18-20, 40-42, 62-64
Any reason not to DRY up these three repetitions into a new method?
Should some or all of said new method live outside this class (see 19, 41, 63 below)?
Does getting the system property, digging for the id, and concatenating the characters merit further breaking said new method into parts in order to sepearate functionality and avoid mixing abstraction levels?

19, 41, 63
From where did getFrameworkContext originate? Why do we reach so far into it? Should/Do we know it that well?
Could Tenant provide a getPNGPathByImageID method to avoid repeatedly calling methods of return values of previous calls?
Would getTenantDirectory better separate concerns (image file manipulation here, paths elsewhere)?

19-20, 41-42, 63-64
Could we make the structure of filePath clearer by naming tenantID and imageID before building the path?

32, 55, 68
Does catching a bare exception only to throw it again really help or would simply ommitting the catch entirely serve equally well?

14-35
Would getImageAsByteArrayByIDIfExistsElseNull more explicitly define this function's current behavior?
Would returning an empty byte array rather than a bare null simplify calls on this method?
Would factoring out two new methods (getPNGByImageID & getByteArrayFromPath) simplify, clarify, and document this function?

22, 44, 65
Do we ever really need to know the file path, or can we simply just receive a handle to the file?
Would getImageFileByID serve even better, since we don't appear to ever need the path again?

25, 46, 49, 66
Do we gain anything by repeatedly instantiating new versions of this file or can we simply use the ones we already have (from 22, 44, 65)?

45
What does getParentFile return? Should we feel entitled to call its method mkdirs?
What does mkdirs do? Why doesn't it take any arguments? What dirs does it make and why do we want/need them?

48
What name would best communicate the purpose of or need for the magic number 1024?

44-54
Does this lengthy process actually represent the java best practice for the presumably common task of writing an input stream to a file?

No Tests
Does the project not yet practice TDD?
Could the above refactorization suggestions provide opportunities to include tests?

Overall, my comments are in agreement with the others. Specifically,

• The need for a class comment explaining the purpose of this class. The name (PersonPictureProcess) is not descriptive. Someone very familiar with the existing application might understand, but others will not.Of course, picking a descriptive class name would be better and not require the class-level comment.

• The repeated calls to system.getProperty(“app.imageRepository”) … should be refactored into a separate method. Everything up to using the id argument could be computed once in the class constructor.

• The exceptions handling does not add any value. If the code can’t add value, it should let the exception bubble up. I like the idea of application exceptions if this class is going to handle any exceptions.

• It is confusing that 2 of the 3 methods hard code “PNG” as the file name extension while the setImage method does not.

Some specific comments:
• Line 46, 49: should correct the miss-spelling of outputStream. outpuStream is created twice. Obviously this is wasteful, but could cause the program to fail on some platforms if the file is not closed before trying to re-open on line 49.
• Line 46, 49, 55: an exception could leave an empty or incomplete file. Presumably, the caller would not know where this file is located and would not be able to delete it.

Well, there is not much that has not yet been touched on, but here are my thoughts on this. I very much agree with most of the things said, but these are the pieces that I saw.

• First off, there are no tests. I am not necessarily saying that TDD needs to be used (although I think there is a VERY strong case for it and I would highly recommend it) but there is nothing to allow a safe refactoring at all
• Rewrite the methods so that the names do not require a comment. If you can not create class names/variable names/method names that are meaningful and explain what the class is doing, there is probably a problem with the design (too big of class, just bad naming, etc)
• This code could really use being DRY'd up. I think everyone agrees on this (see all 432578 comments above)
• Remove the catch statements and add in finally statements to close the streams properly. If you aren't going to handle exceptions, don't bother catching them
• The only thing of value I have to add here is that the methods are written as static, but there is no privatized constructor. Do we really want the user of this class to be able to instantiate the class, hold it in memory, but only access it statically?