[v1.5][W11-B4] StardyTogether #82

yeggasd · 2018-03-11T05:39:16Z

No description provided.

joanneong

Good attempt at the user stories, although there could have been more - you can also include stories which you don’t intend to implement in your final product.

important Do remember to give credit to code sources if your code is inspired from elsewhere. Just changing the variable names will not help - we will still be able to tell that it has been adapted. Please follow the guidelines as given in the learning resources!

Lastly, good job as well for updating tests for the features. Keep it up for the next few versions, and try to submit your PR earlier next time (just in case).

joanneong · 2018-03-11T06:30:31Z

README.adoc

@@ -1,9 +1,9 @@
 = Address Book (Level 4)


Remember to update the product name here as well!

joanneong · 2018-03-11T06:31:33Z

docs/DeveloperGuide.adoc

@@ -787,8 +787,10 @@ See this https://github.com/se-edu/addressbook-level4/pull/599[PR] for the step-
 * can type fast


Remember to update the application title for the developer guide as well!

joanneong · 2018-03-11T06:39:57Z

docs/DeveloperGuide.adoc

+** 4a3. If problem persists, AddressBook directs User to troubleshooting
+
+Use case ends
+


Good to see a new use case added :)

joanneong · 2018-03-11T06:44:25Z

docs/UserGuide.adoc

@@ -221,6 +231,7 @@ The `redo` command fails as there are no `undo` commands executed previously.
 === Clearing all entries : `clear`

 Clears all entries from the address book. +
+Alias: `c` +
 Format: `clear`

 === Exiting the program : `exit`


Nice to think about updating user guide (although it seems like only one person remembered...)

joanneong · 2018-03-11T06:45:42Z

src/main/java/seedu/address/model/AddressBook.java

+        tags.setTags(tagsInPersons);
+    }
+
+


Redundant line here

joanneong · 2018-03-11T06:47:54Z

src/main/java/seedu/address/ui/PersonCard.java

+    }
+
+    /**
+     *


Just delete this empty line if there is nothing you want to add here

yeggasd · 2018-03-16T17:46:12Z

@jingyinno @AzuraAiR @caijun7

joanneong

Good job!

The most common comment this time is probably "Redundant line here", but overall the code quality is high and whatever has been merged so far looks good. I have taken a brief look at everything that is submitted so far, and you guys are on the right track :)

@yeggasd @jingyinno @AzuraAiR @caijun7

IMPORTANT
I would want you to mention the following in your Developer guide

For each team member what are the features (major and minor) you are proposing?
Within the scope of the project, how does it fit in (just a one or 2 line summary)
Please do this by end of day Monday 19 March.

joanneong · 2018-03-17T12:14:26Z

README.adoc

@@ -15,19 +15,15 @@ ifndef::env-github[]
 image::images/Ui.png[width="600"]


Just noticed that your mock UI has the name "Address App" :P

joanneong · 2018-03-17T12:14:53Z

README.adoc


 == Site Map

 * <<UserGuide#, User Guide>>
 * <<DeveloperGuide#, Developer Guide>>
-* <<LearningOutcomes#, Learning Outcomes>>


Good job removing irrelevant sections

joanneong · 2018-03-17T12:28:04Z

docs/AboutUs.adoc

-{empty}[http://github.com/yl-coder[github]] [<<johndoe#, portfolio>>]
+=== Lee Yong Ler
+image::yongler.jpg[width="150", align="left"]
+{empty}[http://github.com/yeggasd[github]] [<<johndoe#, portfolio>>]



As I mentioned in class, the photos are very nice and everything, so 👍

But if you want to be even stricter, the textbook says: "The photo of a team member should be doc/images/githbub_username_in_lower_case.png e.g. docs/images/damithc.png."

joanneong · 2018-03-17T12:59:09Z

docs/UserGuide.adoc

@@ -17,7 +17,7 @@ By: `Team SE-EDU`      Since: `Jun 2016`      Licence: `MIT`

 == Introduction

-AddressBook Level 4 (AB4) is for those who *prefer to use a desktop app for managing contacts*. More importantly, AB4 is *optimized for those who prefer to work with a Command Line Interface* (CLI) while still having the benefits of a Graphical User Interface (GUI). If you can type fast, AB4 can get your contact management tasks done faster than traditional GUI apps. Interested? Jump to the <<Quick Start>> to get started. Enjoy!
+Stardy Together (ST) is mainly for NUS Students who *prefer to use a desktop app for managing contacts*. More importantly, ST is *optimized for those who prefer to work with a Command Line Interface* (CLI) while still having the benefits of a Graphical User Interface (GUI). If you can type fast, ST can get your contact management tasks done faster than traditional GUI apps. Interested? Jump to the <<Quick Start>> to get started. Enjoy!


Good job for updating this section as well

joanneong · 2018-03-17T13:05:31Z

src/main/java/seedu/address/logic/commands/AliasCommand.java

+    }
+}
+
+


Redundant line here

joanneong · 2018-03-17T13:23:36Z

src/main/java/seedu/address/model/person/Birthday.java

+ */
+public class Birthday {
+
+


Redundant line here

joanneong · 2018-03-17T13:29:49Z

src/main/java/seedu/address/storage/SecurityUtil.java

+ */
+public class SecurityUtil {
+    private static final Logger logger = LogsCenter.getLogger(MainApp.class);
+    private static String password = new String("test");


Interesting implementation. So this means that anyone with access to the codebase can decrypt any addressbook data right?

joanneong · 2018-03-17T13:31:51Z

src/main/java/seedu/address/storage/StorageManager.java

+        logger.fine("Attempting to write to backup data file: ");
+        addressBookStorage.backupAddressBook(addressBook);
+    }
+


Redundant line here

joanneong · 2018-03-17T13:32:09Z

src/main/java/seedu/address/storage/XmlAdaptedAlias.java

+
+    /**
+     * Converts this jaxb-friendly adapted alias object into the model's Alias object.
+     *


Redundant line here

joanneong · 2018-03-17T13:34:30Z

src/main/java/seedu/address/storage/XmlAdaptedAlias.java

+        }
+
+        return aliasName.equals(((XmlAdaptedAlias) other).aliasName)
+                && command.equals(((XmlAdaptedAlias) other).command);


This equals implementation looks a bit different from the rest. Is it possible to standardise all the equals implementation?

joanneong

Good job in general, I really like that you guys have included UML diagrams in your documentation 👍

Small suggestion: you might want to label figures to keep the guide consistent throughout.

IMPORTANT
Please make sure to tag your repo with the correct version (e.g. for last tutorial the commits up till v1.2 should be tagged v1.2). If you are not sure how, see the textbook (W3.9c). I will be checking this as well.

joanneong · 2018-03-24T14:38:23Z

docs/DeveloperGuide.adoc

+image::LogicComponentAliasSequenceDiagram.png[width="800"]
+
+image::StorageClassDiagram.png[width="800"]
+


Good attempt at adding UML diagrams! However, the StorageClassDiagram is probably not necessary here since it is already presented under the Architecture section of the Developer Guide :)

joanneong · 2018-03-24T14:41:28Z

docs/DeveloperGuide.adoc

+* **Alternative 2:** Create a HashMap of `Alias` in each command class
+** Pros: Faster to implement as each command class only needs to include a HashMap that stores all the aliases tagged to the command.
+** Cons: High coupling between `Alias` and other commands and the HashMaps of every command needs to be iterated through to find to find the aliased command.
+


Minor detail but the bullet point seems to be showing up differently for the two Alternative headers...

joanneong · 2018-03-24T14:42:31Z

docs/DeveloperGuide.adoc

+** Cons:
+* **Alternative 2:** Store in `UserPrefsStorage`
+** Pros:
+** Cons:


Please complete this section (asap)! I didn't really notice that this was incomplete in class... :(

joanneong · 2018-03-24T14:45:16Z

docs/DeveloperGuide.adoc

+
+* **Alternative 2:**  Encryption and Decryption done where needed in `XmlUtil` and `XmlFileStorage`
+** Pros: `addressbook.xml` is exposed minimally.
+** Cons: Increase coupling of more classes and makes the implementation harder to understand.
 // end::dataencryption[]


The documentation of this feature seems to be okay :)

joanneong · 2018-03-24T14:49:51Z

docs/DeveloperGuide.adoc

+* **Alternative 2:** Adds selected `Person`,`Tag`, and `Alias` from imported AddressBook that are not a duplicate of existing `Person`, `Tag`, and `Alias` to the user's AddressBook.
+** Pros: User is able to select which `Person`, `Tag` or `Alias` to be imported.
+** Cons: User needs to indicate which `Person`, `Tag` or `Alias` to be imported.
+


This section is also alright. However, you can consider adding on to the pros/cons at time (e.g. another con might be in terms of difficulty for implementation) just so it is even more comprehensive than it already is.

joanneong · 2018-03-24T14:53:33Z

docs/DeveloperGuide.adoc

+
+===== Aspect: How BirthdayList parses its Data
+
+* **Alternative 1: Updating Birthday Class to have separate classes Day and Month.


You might want to make Alternative 1 clearer. Compare this with what you wrote for Alternative 2 - which one do you think is better?

Hint: I didn't quite understand what you were getting at here until I read Alternative 2.

joanneong · 2018-03-24T14:55:17Z

docs/DeveloperGuide.adoc

+
+===== Aspect: How BirthdayList presents its data
+* ** Alternative 1: Notification at the start of app if a birthday is occuring today
+** Pros: More intuitive for User. Provides additional functionality for user stories


What is "provides additional functionality for user stories"?

joanneong · 2018-03-24T15:01:23Z

docs/UserGuide.adoc

+
+=== Viewing a collated birthday list: `birthdays` (coming in v1.3)
+
+Displays a list that contains all the birthdays of all contacts ordered in date +


I think it should be "ordered by date"? :P

joanneong · 2018-03-24T15:03:14Z

docs/UserGuide.adoc

+
+****
+* Finds vacant study rooms in the specified `BUILDING`.
+* The building must be in NUS venue format, e.g. `COM1`, `S17`, `E2`


Is there any way for users to find out what the NUS venue formats are?

joanneong · 2018-03-24T15:05:17Z

docs/UserGuide.adoc

+* Displays full name of persons, arranged in the order of the modules they are taking.
+* There may by multiple entries for each person.
+****
+


Personally I find the section on sortCode hard to understand. I cannot quite visualise what you mean here. Maybe adding an example can help...

Edit User Guide

…ixedStorageIssues_28032018 Fixed Storage and Tagging Issues

yeggasd · 2018-04-04T03:34:25Z

@joanneong joanneong

joanneong

My apologies for the 'late' review - usually I'd have it done by Sunday but last week was a little...busy.

In any case, I have looked at the collated code and given some feedback on the coding style and quality. In general, your team is on the right track and the code seems to be of high quality, so keep it up! 👍

joanneong · 2018-04-10T05:25:05Z

collated/functional/AzuraAiR-reused.md

+``` java
+    @Override
+    public void backupAddressBook(ReadOnlyAddressBook addressBook) throws IOException,
+                                                                            WrongPasswordException {


Seems like this can fit on a single line (see line 14 below)

joanneong · 2018-04-10T05:30:42Z

collated/functional/AzuraAiR.md

+
+            if (longUrl == null) {
+                throw new NoInternetConnectionException("No internet connection, starting with empty timetables");
+            } else if (longUrl.equals("http://modsn.us")) {


Curious, but why do you specifically check for http://modsn.us?

joanneong · 2018-04-10T05:33:15Z

collated/functional/AzuraAiR.md

+
+        String semNum = urlParts[4];    // Normally found at the fifth part
+        String[] toParse = urlParts[5].split("\\?");    // Seperate "share" out of last part of url
+        String[] modules = toParse[1].split("&");   // Seperate the modules in last part of url


You may want to consider abstracting these magic numbers (4, 5, 1) and regex into constants.

Also, I am okay with how you are writing the comments here but you may want to follow point 5 in this section of the coding standards...

joanneong · 2018-04-10T05:34:57Z

collated/functional/AzuraAiR.md

+            for (HashMap<String, String> lesson : lessonInfo) {
+                Lesson lessonToAdd = new Lesson(moduleCode, lesson.get("ClassNo"), lesson.get("LessonType"),
+                        lesson.get("WeekText"), lesson.get("DayText"),
+                        lesson.get("StartTime"), lesson.get("EndTime"));


This line can be combined with the previous line

joanneong · 2018-04-10T05:36:33Z

collated/functional/AzuraAiR.md

+        }
+
+        list.sort(new Comparator<Person>() {
+            @Override


Would be nice if you could write the comparator properly elsewhere instead of randomly squeezing it here...

joanneong · 2018-04-10T06:23:28Z

collated/test/AzuraAiR.md

+        //postNow(birthdayListEventStub);
+        //birthdayList.loadList(birthdayListEventStub.getBirthdayList()); // Manual loading
+        //guiRobot.pauseForHuman();
+        //assertEquals(expectedResult, birthdaysListHandle.getText());


What happened to the code here?

joanneong · 2018-04-10T06:27:10Z

collated/test/Caijun7.md

+    /**
+     * Sets the {@code buildingName} into a {@code Building} that we are building.
+     */
+    public BuildingBuilder withBuildingName(String buildingName) {


Can the setter method here (and below) be better named?

joanneong · 2018-04-10T06:31:03Z

collated/test/jingyinno.md

+     */
+    /**
+     * A default model stub that have all of the methods failing.
+     */


@.@ Am I starting to see doubles?

joanneong · 2018-04-10T06:32:13Z

collated/test/jingyinno.md

+    /**
+     * A default model stub that have all of the methods failing.
+     */
+    private class ModelStub implements Model {


Double JavaDoc here again...

In fact, if this stub is being used in multiple places, you can consider the possibility of extracting it into a file of its own

joanneong · 2018-04-10T06:35:31Z

collated/test/yeggasd.md

+ * same path through the RemovePasswordCommand, and therefore we test only one of them.
+ * The path variation for those two cases occur inside the ParserUtil, and
+ * therefore should be covered by the ParserUtilTest.
+ */


If you find yourself explaining the "why" behind something you're doing in the code, it may be more appropriate to shift the explanation to the real documentation documents (e.g. Developer Guide).

Same below where you copied this comment

…essbook-level4 into ExportCommand # Conflicts: # src/test/data/XmlAddressBookStorageTest/encryptedAliceBensonAddressBook # src/test/data/XmlAddressBookStorageTest/encryptedAliceBensonAddressBookBackup

Update DG and PPP

Pull group master

Update developer guide

Update DG

Update UG and DG

Final collate

Update PPP

Fix broken link

pull group master

fix dg indentation

nus-se-pr-bot added team.B4 tutorial.W11 labels Mar 11, 2018

nus-se-pr-bot assigned joanneong and hwkchia Mar 11, 2018

joanneong reviewed Mar 11, 2018

View reviewed changes

joanneong added the reviewed-v1_0 label Mar 11, 2018

yeggasd changed the title ~~[v1.0][W11-B4] Stardy Together~~ [v1.1][W11-B4] StardyTogether Mar 16, 2018

joanneong reviewed Mar 17, 2018

View reviewed changes

joanneong added reviewed-v1_1 and removed reviewed-v1_0 labels Mar 17, 2018

joanneong reviewed Mar 24, 2018

View reviewed changes

joanneong added reviewed-v1_2 and removed reviewed-v1_1 labels Mar 24, 2018

jasmoon added a commit to jasmoon/addressbook-level4 that referenced this pull request Mar 24, 2018

Merge pull request nus-cs2103-AY1718S2#82 from karenfrilya97/UGDGdraft1

f3e39f3

Edit User Guide

wynonaK pushed a commit to wynonaK/addressbook-level4 that referenced this pull request Mar 29, 2018

Merge pull request nus-cs2103-AY1718S2#82 from CS2103JAN2018-F14-B2/F…

db6baf8

…ixedStorageIssues_28032018 Fixed Storage and Tagging Issues

joanneong reviewed Apr 10, 2018

View reviewed changes

joanneong added reviewed-v1_4 and removed reviewed-v1_2 labels Apr 10, 2018

jingyinno and others added 10 commits April 14, 2018 16:49

Fix checkstyle

1a3a4e6

Merge branch 'master' into alias-list-11Apr

306126e

Modify test files to binary format

456957c

Remove encrypted file

56b484f

Remove encrypted file

a767a64

Merge branch 'master' of https://github.com/CS2103JAN2018-W11-B4/addr…

65be8df

…essbook-level4 into ExportCommand # Conflicts: # src/test/data/XmlAddressBookStorageTest/encryptedAliceBensonAddressBook # src/test/data/XmlAddressBookStorageTest/encryptedAliceBensonAddressBookBackup

Modify checkstyle to ignore newline at EOF for XML files

ee15b1d

Change to binary file

ab9338d

Remove files

925dcf2

Update test password

4a3ad84

yeggasd and others added 26 commits April 15, 2018 23:25

Merge pull request #247 from yeggasd/Production

f2798db

Update DG and PPP

Update dg

e80c6a7

Update dg

564a1b0

Merge pull request #32 from CS2103JAN2018-W11-B4/master

0a1defc

Pull group master

Merge pull request #248 from Caijun7/DeveloperGuide-update

24ac62a

Update developer guide

Final collate

24658d4

Merge branch 'master' into master

6a134fb

Add sections

de27134

Update DG

a519b7d

Merge branch 'master' into master

40494cc

Update PPP

6cdbc0b

Remove trailing whitespace

ce03243

Merge branch 'master' into master

a011769

Merge pull request #251 from yeggasd/master

0b0943b

Update DG

Merge branch 'master' into master

8628b5e

Merge branch 'master' into master

dd90c8b

Update UG

a47f0be

Merge branch 'master' of https://github.com/AzuraAiR/addressbook-level4

4b34348

Merge pull request #250 from AzuraAiR/master

32c4094

Update UG and DG

Merge branch 'master' into master

e147ab3

Merge pull request #249 from jingyinno/master

c63d8f7

Final collate

Fix broken link

23451c5

Merge branch 'master' into Production

11358c2

Merge pull request #252 from yeggasd/Production

6bf72e4

Update PPP

Merge branch 'master' of https://github.com/CS2103JAN2018-W11-B4/main

2c0792b

Merge pull request #253 from AzuraAiR/master

a67ac3c

Fix broken link

yeggasd changed the title ~~[v1.1][W11-B4] StardyTogether~~ [v1.5][W11-B4] StardyTogether Apr 15, 2018

jingyinno added 3 commits April 16, 2018 01:04

Merge pull request #33 from CS2103JAN2018-W11-B4/master

692fd0a

pull group master

update dg

8bb43fb

Merge pull request #259 from jingyinno/master

1a86375

fix dg indentation

		@@ -787,8 +787,10 @@ See this https://github.com/se-edu/addressbook-level4/pull/599[PR] for the step-
		* can type fast

+                  }
+                  /**
+                   *

		@@ -15,19 +15,15 @@ ifndef::env-github[]
		image::images/Ui.png[width="600"]

		image::LogicComponentAliasSequenceDiagram.png[width="800"]

		image::StorageClassDiagram.png[width="800"]


		===== Aspect: How BirthdayList parses its Data

		* **Alternative 1: Updating Birthday Class to have separate classes Day and Month.


		=== Viewing a collated birthday list: `birthdays` (coming in v1.3)

		Displays a list that contains all the birthdays of all contacts ordered in date +

[v1.5][W11-B4] StardyTogether #82

Are you sure you want to change the base?

[v1.5][W11-B4] StardyTogether #82

Conversation

yeggasd commented Mar 11, 2018

joanneong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeggasd commented Mar 16, 2018

joanneong left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanneong left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeggasd commented Apr 4, 2018

joanneong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanneong left a comment •

edited

Loading

joanneong left a comment •

edited

Loading