Using enumeration to represent error messages legibily - is it good practice?

enums, error-handling, exception, java

Solution

I would suggest using simple string templates instead of enums for building error messages. Something like this:

String EMPTY_FILE_NAME_IN_LIST_TEMPLATE = "One or more null/empty filename(s) found";
String FILE_DOESNT_EXIST_TEMPLATE = "[ %s ] does not exist";
String FILE_LIST_IS_NULL_TEMPLATE = "File list is null/empty";
String FILENAME_NOT_ABSOLUTE_TEMPLATE = "[ %s ] is not absolute";
String MUST_BE_DIR_TEMPLATE = "[ %s ] must be a directory";
String MUST_BE_FILE_TEMPLATE = "[ %s ] must be a file";
String NULL_OR_EMPTY_TEMPLATE = "[ %s ] is null/empty";
String OUTPUT_FILE_ALREADY_EXISTS_TEMPLATE = "[ %s ] already exists";
String OUTPUT_FILENAME_EMPTY_TEMPLATE = "Output filename is null/empty";
String OUTPUT_PATH_EMPTY_TEMPLATE = "Output path is null/empty";

And then, use `String.format(template, sFilename)` for building actual message.

You may also consider throwing an exception right out of `getFileErrorsIfAny()` method:

File file = new File(sFilename);
if (!file.isAbsolute()) {
    throw new ZipInputException(String.format(FILENAME_NOT_ABSOLUTE_TEMPLATE, sFilename));
}

Looks cleaner and more compact to me.

Problem

I'd like to consolidate my error messages and stuff into one file, and make my code more readable if possible. Here's an example of what I have in my enum file: ``` public enum ZipErrorType { // START: define exception messages (alphabetical order) EMPTY_FILE_NAME_IN_LIST { public String toString() { return "One or more null/empty filename(s) found"; } }, FILE_DOESNT_EXIST { public String who(String sThisFile) { return "[" + sThisFile + "] does not exist"; } }, FILE_LIST_IS_NULL { public String toString() { return "File list is null/empty"; } }, FILENAME_NOT_ABSOLUTE { public String who(String sThisFile) { return "[" + sThisFile + "] is not absolute"; } }, MUST_BE_DIR { public String who(String sThisFile) { return "[" + sThisFile + "] must be a directory"; } }, MUST_BE_FILE { public String who(String sThisFile) { return "[" + sThisFile + "] must be a file"; } }, NULL_OR_EMPTY { public String who(String sThisFile) { return "[" + sThisFile + "] is null/empty"; } }, OUTPUT_FILE_ALREADY_EXISTS { public String who(String sThisFile) { return "[" + sThisFile + "] already exists"; } }, OUTPUT_FILENAME_EMPTY { public String toString() { return "Output filename is null/empty"; } }, OUTPUT_PATH_EMPTY { public String toString() { return "Output path is null/empty"; } }, // END: define exception messages NONE {}; public String who(String sThisFile) { return ""; } } ``` Then in my program I have code like: ``` private static ZipErrorType getFileErrorsIfAny(String sFilename, boolean shouldBeFile) { // check if given filename is absolute File file = new File(sFilename); if (!file.isAbsolute()) { return ZipErrorType.FILENAME_NOT_ABSOLUTE; } // check if file exists if (!file.exists()) { return ZipErrorType.FILE_DOESNT_EXIST; } // check if corresponding file is a file when it shouldn't be... if (file.isFile() && !shouldBeFile) { return ZipErrorType.MUST_BE_DIR; } // ...or a directory when it should be a file else if (file.isDirectory() && shouldBeFile) { return ZipErrorType.MUST_BE_FILE; } return ZipErrorType.NONE; } ``` ...and an example of how I make use of my enum: ``` // check input files for (String sFile : files) { if (sFile == null || sFile.trim().length() == 0) { throw new NullPointerException("One or more filename is null/empty"); } errorIfAny = getFileErrorsIfAny(sFile.trim(), true); if (!errorIfAny.equals(ZipErrorType.NONE)) { throw new ZipInputException(errorIfAny.who(sFile.trim())); } } ``` Now I know it's hard to judge just by these code snippets alone, but is this alright, from a general perspective? Is what I'm doing not worth the trouble, and is there a way to improve this?

Original source