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

Jump to bottom

[T2A2][F14-A3]LiuYulin #294

Open

LiuYulin0629 wants to merge 2 commits into nus-cs2103-AY1617S2:master from LiuYulin0629:T2A2

LiuYulin0629 commented Feb 2, 2017

Ready for review


          MethodAbstraction

6deb24a

nus-se-pr-bot added the FormatCheckRequested label

nus-se-pr-bot commented Feb 2, 2017

Hi @LiuYulin0629, your pull request title is invalid. It should be in the format of [Activity ID][Team ID] Your name, where [Activity Id] has no dashes or spaces (e.g. [T2A3] stands for Tutorial 2 Activity 3) and [Team ID] has one dash only and no spaces (e.g. [W14-A2] means Wednesday 2pm (14 hrs), Phase A, Team 2). Please follow the instructions given strictly and edit your title for reprocessing.

Submit only one activity per pull request (unless otherwise stated in instructions) and do remember to create your branches from the commit where the master branch is pointing at so that each PR you submit only consist of commits meant for the activity.

Note: this comment is posted by a bot. If you believe this is done in error, please create an issue at cs2103-pr-bot and add a link to this PR.

LiuYulin0629 changed the title ~~[T2A2][T05-03]LiuYulin~~ [T2A2][F14-A3]LiuYulin

nus-se-pr-bot removed the FormatCheckRequested label

nus-se-pr-bot assigned chanjunweimy


          Update AddressBook.java

64c83aa

chanjunweimy reviewed

View reviewed changes

chanjunweimy left a comment

@LiuYulin0629 nice work done! Keep up your good job!

src/seedu/addressbook/AddressBook.java

-                      }
-                      if (args.length == 1) {
+                      }else if (args.length == 1) {

chanjunweimy Feb 4, 2017

Coding standard violation here: check if-else :D

src/seedu/addressbook/AddressBook.java

@@ @@ -135,7 +135,8 @@ @@
                   private static final String DIVIDER = "===================================================";
+                  private static final int COMMAND_TYPE_INDEX = 0;
+                  private static final int COMMAND_ARG_INDEX = 1;

chanjunweimy Feb 4, 2017

good job in eliminating magic numbers

src/seedu/addressbook/AddressBook.java

-                  private static boolean deletePersonFromAddressBook(String[] exactPerson) {
-                      final boolean changed = ALL_PERSONS.remove(exactPerson);
-                      if (changed) {
+                  private static boolean hasDeletedPersonFromAddressBook(String[] exactPerson) {

chanjunweimy Feb 4, 2017

good job in following the boolean naming convention

src/seedu/addressbook/AddressBook.java

    
                      return splitArgs.length == PERSON_DATA_COUNT // 3 arguments

                              && !splitArgs[PERSON_DATA_INDEX_NAME].isEmpty() // non-empty arguments

                              && !splitArgs[PERSON_DATA_INDEX_PHONE].isEmpty()

                              && !splitArgs[PERSON_DATA_INDEX_EMAIL].isEmpty();

chanjunweimy Feb 4, 2017

good job here ^^

src/seedu/addressbook/AddressBook.java

    
                  private static String removePrefixSign(String s, String sign) {

                      return s.replace(sign, "");

                  private static String removePrefixSign(String fullString, String prefix) {

                      return fullString.replace(prefix, "");

chanjunweimy Feb 4, 2017

good job in trying to rename to make it sounds better ^^

chanjunweimy added the Reviewed label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels