[T2A2][T11-A3]Loh Yao Jun Alvin #238
Conversation
| @@ -98,40 +98,40 @@ | |||
| private static final String PERSON_STRING_REPRESENTATION = "%1$s " // name | |||
| + PERSON_DATA_PREFIX_PHONE + "%2$s " // phone | |||
| + PERSON_DATA_PREFIX_EMAIL + "%3$s"; // email | |||
| private static final String COMMAND_ADD_WORD = "add"; | |||
| private static final String ADD = "add"; | |||
There was a problem hiding this comment.
The precious name would be preferred. It provides more details about the variable.
| String userCommand = getUserInput(); | ||
| String feedback = executeCommand(userCommand); | ||
| showResultToUser(feedback); | ||
| } |
| @@ -257,20 +264,32 @@ private static void echoUserCommand(String userCommand) { | |||
| * @param args full program arguments passed to application main method | |||
| */ | |||
| private static void processProgramArgs(String[] args) { | |||
| if (args.length >= 2) { | |||
| if (isProgramValid(args)) { | |||
There was a problem hiding this comment.
The name here is misleading. The function will return true when the number of args is invalid.
| setupGivenFileForStorage(args[0]); | ||
| } | ||
|
|
||
| if(args.length == 0) { | ||
| if(isDefaultFile(args)) { |
There was a problem hiding this comment.
You could consider turning these if statements into a switch case.
| @@ -312,7 +331,7 @@ private static void setupDefaultFileForStorage() { | |||
| * and a valid file name as determined by {@link #hasValidFileName}. | |||
| */ | |||
| private static boolean isValidFilePath(String filePath) { | |||
| if (filePath == null) { | |||
| if (noValidFilePath(filePath)) { | |||
There was a problem hiding this comment.
Boolean functions should generally have a name starting with 'is'. Have a closer look at the java coding standard document.
| @@ -364,24 +387,27 @@ private static void loadDataFromStorage() { | |||
| * @param userInputString raw input from user | |||
| * @return feedback about how the command was executed | |||
| */ | |||
| public enum commandType { | |||
| ADD, FIND, LIST, DELETE, CLEAR, HELP, EXIT | |||
| } | |||
There was a problem hiding this comment.
Using enum for commandtype is a good idea! However, the implementation can be improved. Also do look at the java coding standard doc on how enum variables should be named.
| @@ -364,24 +387,27 @@ private static void loadDataFromStorage() { | |||
| * @param userInputString raw input from user | |||
| * @return feedback about how the command was executed | |||
| */ | |||
| public enum commandType { | |||
| ADD, FIND, LIST, DELETE, CLEAR, HELP, EXIT | |||
| } | |||
| private static String executeCommand(String userInputString) { | |||
| final String[] commandTypeAndParams = splitCommandWordAndArgs(userInputString); | |||
| final String commandType = commandTypeAndParams[0]; | |||
| final String commandArgs = commandTypeAndParams[1]; | |||
| switch (commandType) { | |||
There was a problem hiding this comment.
Note that comandType here refers to the String variable declared at line 395 and not the enum type. This is a good example of why the coding standards are important. It helps avoids having name clashes.
| private static String removePrefixSign(String s, String sign) { | ||
| return s.replace(sign, ""); | ||
| private static String removePrefix(String fullstring, String prefix) { | ||
| return fullstring.replace(prefix, ""); |
There was a problem hiding this comment.
Do not forget to update the documentation as well.
Ready for review