Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[T2A2][T16-A1] Alwinson Au-yong #307

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[T2A2][T16-A1] Alwinson Au-yong #307

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c29b1b8
Refactoring and improve code quality
alwinsonauyong Feb 7, 2017
5f98df0
Ready for Review
alwinsonauyong Feb 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 52 additions & 44 deletions src/seedu/addressbook/AddressBook.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public class AddressBook {
/**
* A platform independent line separator.
*/
private static final String LS = System.lineSeparator() + LINE_PREFIX;
private static final String LINE_SEPARATOR = System.lineSeparator() + LINE_PREFIX;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!


/*
* NOTE : ==================================================================
Expand All @@ -74,11 +74,11 @@ public class AddressBook {
private static final String MESSAGE_DISPLAY_PERSON_DATA = "%1$s Phone Number: %2$s Email: %3$s";
private static final String MESSAGE_DISPLAY_LIST_ELEMENT_INDEX = "%1$d. ";
private static final String MESSAGE_GOODBYE = "Exiting Address Book... Good bye!";
private static final String MESSAGE_INVALID_COMMAND_FORMAT = "Invalid command format: %1$s " + LS + "%2$s";
private static final String MESSAGE_INVALID_COMMAND_FORMAT = "Invalid command format: %1$s " + LINE_SEPARATOR + "%2$s";
private static final String MESSAGE_INVALID_FILE = "The given file name [%1$s] is not a valid file name!";
private static final String MESSAGE_INVALID_PROGRAM_ARGS = "Too many parameters! Correct program argument format:"
+ LS + "\tjava AddressBook"
+ LS + "\tjava AddressBook [custom storage file path]";
+ LINE_SEPARATOR + "\tjava AddressBook"
+ LINE_SEPARATOR + "\tjava AddressBook [custom storage file path]";
private static final String MESSAGE_INVALID_PERSON_DISPLAYED_INDEX = "The person index provided is invalid";
private static final String MESSAGE_INVALID_STORAGE_FILE_CONTENT = "Storage file has invalid content";
private static final String MESSAGE_PERSON_NOT_IN_ADDRESSBOOK = "Person could not be found in address book";
Expand Down Expand Up @@ -154,6 +154,14 @@ public class AddressBook {
* Offset required to convert between 1-indexing and 0-indexing.COMMAND_
*/
private static final int DISPLAYED_INDEX_OFFSET = 1;
/**
* Command Type Index
*/
private static final int COMMAND_TYPE_INDEX = 0;
/**
* Command Argument Index
*/
private static final int COMMAND_ARGS_INDEX = 1;

/**
* If the first non-whitespace character in a user's input line is this, that line will be ignored.
Expand Down Expand Up @@ -212,7 +220,6 @@ public static void main(String[] args) {
loadDataFromStorage();
while (true) {
String userCommand = getUserInput();
echoUserCommand(userCommand);
String feedback = executeCommand(userCommand);
showResultToUser(feedback);
}
Expand Down Expand Up @@ -365,9 +372,10 @@ private static void loadDataFromStorage() {
* @return feedback about how the command was executed
*/
private static String executeCommand(String userInputString) {
echoUserCommand(userInputString);
final String[] commandTypeAndParams = splitCommandWordAndArgs(userInputString);
final String commandType = commandTypeAndParams[0];
final String commandArgs = commandTypeAndParams[1];
final String commandType = commandTypeAndParams[COMMAND_TYPE_INDEX];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to see you have removed the magic numbers here!

final String commandArgs = commandTypeAndParams[COMMAND_ARGS_INDEX];
switch (commandType) {
case COMMAND_ADD_WORD:
return executeAddPerson(commandArgs);
Expand Down Expand Up @@ -645,7 +653,7 @@ private static String getDisplayString(ArrayList<String[]> persons) {
final int displayIndex = i + DISPLAYED_INDEX_OFFSET;
messageAccumulator.append('\t')
.append(getIndexedPersonListElementMessage(displayIndex, person))
.append(LS);
.append(LINE_SEPARATOR);
}
return messageAccumulator.toString();
}
Expand Down Expand Up @@ -794,11 +802,11 @@ private static void addPersonToAddressBook(String[] person) {
* @return true if the given person was found and deleted in the model
*/
private static boolean deletePersonFromAddressBook(String[] exactPerson) {
final boolean changed = ALL_PERSONS.remove(exactPerson);
if (changed) {
final boolean ischanged = ALL_PERSONS.remove(exactPerson);
if (ischanged) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the idea here is correct, naming the boolean by prefixing is however, you need to follow camel casing. appropriate would be isChanged

savePersonsToFile(getAllPersonsInAddressBook(), storageFilePath);
}
return changed;
return ischanged;
}

/**
Expand Down Expand Up @@ -957,10 +965,10 @@ private static Optional<ArrayList<String[]>> decodePersonsFromStrings(ArrayList<
private static boolean isPersonDataExtractableFrom(String personData) {
final String matchAnyPersonDataPrefix = PERSON_DATA_PREFIX_PHONE + '|' + PERSON_DATA_PREFIX_EMAIL;
final String[] splitArgs = personData.trim().split(matchAnyPersonDataPrefix);
return splitArgs.length == 3 // 3 arguments
&& !splitArgs[0].isEmpty() // non-empty arguments
&& !splitArgs[1].isEmpty()
&& !splitArgs[2].isEmpty();
return splitArgs.length == PERSON_DATA_COUNT // 3 arguments
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 eliminating magic numbers.

&& !splitArgs[PERSON_DATA_INDEX_NAME].isEmpty() // non-empty arguments
&& !splitArgs[PERSON_DATA_INDEX_PHONE].isEmpty()
&& !splitArgs[PERSON_DATA_INDEX_EMAIL].isEmpty();
}

/**
Expand Down Expand Up @@ -989,12 +997,12 @@ private static String extractPhoneFromPersonString(String encoded) {

// phone is last arg, target is from prefix to end of string
if (indexOfPhonePrefix > indexOfEmailPrefix) {
return removePrefixSign(encoded.substring(indexOfPhonePrefix, encoded.length()).trim(),
return removePrefix(encoded.substring(indexOfPhonePrefix, encoded.length()).trim(),
PERSON_DATA_PREFIX_PHONE);

// phone is middle arg, target is from own prefix to next prefix
} else {
return removePrefixSign(
return removePrefix(
encoded.substring(indexOfPhonePrefix, indexOfEmailPrefix).trim(),
PERSON_DATA_PREFIX_PHONE);
}
Expand All @@ -1012,12 +1020,12 @@ private static String extractEmailFromPersonString(String encoded) {

// email is last arg, target is from prefix to end of string
if (indexOfEmailPrefix > indexOfPhonePrefix) {
return removePrefixSign(encoded.substring(indexOfEmailPrefix, encoded.length()).trim(),
return removePrefix(encoded.substring(indexOfEmailPrefix, encoded.length()).trim(),
PERSON_DATA_PREFIX_EMAIL);

// email is middle arg, target is from own prefix to next prefix
} else {
return removePrefixSign(
return removePrefix(
encoded.substring(indexOfEmailPrefix, indexOfPhonePrefix).trim(),
PERSON_DATA_PREFIX_EMAIL);
}
Expand Down Expand Up @@ -1082,46 +1090,46 @@ private static boolean isPersonEmailValid(String email) {

/** Returns usage info for all commands */
private static String getUsageInfoForAllCommands() {
return getUsageInfoForAddCommand() + LS
+ getUsageInfoForFindCommand() + LS
+ getUsageInfoForViewCommand() + LS
+ getUsageInfoForDeleteCommand() + LS
+ getUsageInfoForClearCommand() + LS
+ getUsageInfoForExitCommand() + LS
return getUsageInfoForAddCommand() + LINE_SEPARATOR
+ getUsageInfoForFindCommand() + LINE_SEPARATOR
+ getUsageInfoForViewCommand() + LINE_SEPARATOR
+ getUsageInfoForDeleteCommand() + LINE_SEPARATOR
+ getUsageInfoForClearCommand() + LINE_SEPARATOR
+ getUsageInfoForExitCommand() + LINE_SEPARATOR
+ getUsageInfoForHelpCommand();
}

/** Returns the string for showing 'add' command usage instruction */
private static String getUsageInfoForAddCommand() {
return String.format(MESSAGE_COMMAND_HELP, COMMAND_ADD_WORD, COMMAND_ADD_DESC) + LS
+ String.format(MESSAGE_COMMAND_HELP_PARAMETERS, COMMAND_ADD_PARAMETERS) + LS
+ String.format(MESSAGE_COMMAND_HELP_EXAMPLE, COMMAND_ADD_EXAMPLE) + LS;
return String.format(MESSAGE_COMMAND_HELP, COMMAND_ADD_WORD, COMMAND_ADD_DESC) + LINE_SEPARATOR
+ String.format(MESSAGE_COMMAND_HELP_PARAMETERS, COMMAND_ADD_PARAMETERS) + LINE_SEPARATOR
+ String.format(MESSAGE_COMMAND_HELP_EXAMPLE, COMMAND_ADD_EXAMPLE) + LINE_SEPARATOR;
}

/** Returns the string for showing 'find' command usage instruction */
private static String getUsageInfoForFindCommand() {
return String.format(MESSAGE_COMMAND_HELP, COMMAND_FIND_WORD, COMMAND_FIND_DESC) + LS
+ String.format(MESSAGE_COMMAND_HELP_PARAMETERS, COMMAND_FIND_PARAMETERS) + LS
+ String.format(MESSAGE_COMMAND_HELP_EXAMPLE, COMMAND_FIND_EXAMPLE) + LS;
return String.format(MESSAGE_COMMAND_HELP, COMMAND_FIND_WORD, COMMAND_FIND_DESC) + LINE_SEPARATOR
+ String.format(MESSAGE_COMMAND_HELP_PARAMETERS, COMMAND_FIND_PARAMETERS) + LINE_SEPARATOR
+ String.format(MESSAGE_COMMAND_HELP_EXAMPLE, COMMAND_FIND_EXAMPLE) + LINE_SEPARATOR;
}

/** Returns the string for showing 'delete' command usage instruction */
private static String getUsageInfoForDeleteCommand() {
return String.format(MESSAGE_COMMAND_HELP, COMMAND_DELETE_WORD, COMMAND_DELETE_DESC) + LS
+ String.format(MESSAGE_COMMAND_HELP_PARAMETERS, COMMAND_DELETE_PARAMETER) + LS
+ String.format(MESSAGE_COMMAND_HELP_EXAMPLE, COMMAND_DELETE_EXAMPLE) + LS;
return String.format(MESSAGE_COMMAND_HELP, COMMAND_DELETE_WORD, COMMAND_DELETE_DESC) + LINE_SEPARATOR
+ String.format(MESSAGE_COMMAND_HELP_PARAMETERS, COMMAND_DELETE_PARAMETER) + LINE_SEPARATOR
+ String.format(MESSAGE_COMMAND_HELP_EXAMPLE, COMMAND_DELETE_EXAMPLE) + LINE_SEPARATOR;
}

/** Returns string for showing 'clear' command usage instruction */
private static String getUsageInfoForClearCommand() {
return String.format(MESSAGE_COMMAND_HELP, COMMAND_CLEAR_WORD, COMMAND_CLEAR_DESC) + LS
+ String.format(MESSAGE_COMMAND_HELP_EXAMPLE, COMMAND_CLEAR_EXAMPLE) + LS;
return String.format(MESSAGE_COMMAND_HELP, COMMAND_CLEAR_WORD, COMMAND_CLEAR_DESC) + LINE_SEPARATOR
+ String.format(MESSAGE_COMMAND_HELP_EXAMPLE, COMMAND_CLEAR_EXAMPLE) + LINE_SEPARATOR;
}

/** Returns the string for showing 'view' command usage instruction */
private static String getUsageInfoForViewCommand() {
return String.format(MESSAGE_COMMAND_HELP, COMMAND_LIST_WORD, COMMAND_LIST_DESC) + LS
+ String.format(MESSAGE_COMMAND_HELP_EXAMPLE, COMMAND_LIST_EXAMPLE) + LS;
return String.format(MESSAGE_COMMAND_HELP, COMMAND_LIST_WORD, COMMAND_LIST_DESC) + LINE_SEPARATOR
+ String.format(MESSAGE_COMMAND_HELP_EXAMPLE, COMMAND_LIST_EXAMPLE) + LINE_SEPARATOR;
}

/** Returns string for showing 'help' command usage instruction */
Expand All @@ -1144,14 +1152,14 @@ private static String getUsageInfoForExitCommand() {
*/

/**
* Removes sign(p/, d/, etc) from parameter string
* Removes prefix(p/, d/, etc) from parameter string
*
* @param s Parameter as a string
* @param sign Parameter sign to be removed
* @return string without the sign
* @param fullString Parameter as a string
* @param prefix Parameter prefix to be removed
* @return string without the prefix
*/
private static String removePrefixSign(String s, String sign) {
return s.replace(sign, "");
private static String removePrefix(String fullString, String prefix) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the solution to this one is incorrect.
this one is pretty much the same as earlier. you need to change the code so that only prefix is removed.
in the current version all occurrences of prefix in fullString are removed.

return fullString.replace(prefix, "");
}

/**
Expand Down