[CS2103T-F10-1] ResuMe #21

chrisjwelly · 2020-02-18T02:34:27Z

Team Members:
@chrisjwelly : Christian James Welly
@helloImHai : Nguyen Chi Hai
@wardetu : Nguyen Minh Hoang
@nhamhung : Nham Quoc Hung
@duongphammmm : Pham Thuy Duong

foochifa

Good job! May need to check for carelessness in naming.

foochifa · 2020-03-06T02:41:20Z

docs/DeveloperGuide.adoc

@@ -316,30 +316,73 @@ _{More to be added}_
 (For all use cases below, the *System* is the `AddressBook` and the *Actor* is the `user`, unless specified otherwise)


The system name needs to be changed.

foochifa · 2020-03-06T02:43:17Z

docs/DeveloperGuide.adoc

+[none]
+** 3a1. ResuMe shows an error message
+
+


Maybe need to include a use case ends or resumes

foochifa · 2020-03-06T02:43:34Z

docs/DeveloperGuide.adoc

+[none]
+** 1a1. ResuMe shows an error message.
+
+


Maybe need a use case ends or resumes

foochifa · 2020-03-06T02:44:29Z

docs/DeveloperGuide.adoc

+
+1.  User enters `find KEYWORD` without specifying a `TYPE`
+2.  ResuMe displays all items whose names contain the `KEYWORD`
+3.  If user enters `find -TYPE KEYWORD`
+4.  ResuMe displays only items of the `TYPE` whose names contain the `KEYWORD`


I think it may be a little too specific.
Maybe it can be:
User finds item using keyword etc...

Also,
It seems like these could be separated:
1-2 is Use case: Find item without specifying type
3-4 is Use case: Find item with specified type

foochifa · 2020-03-06T02:48:26Z

docs/DeveloperGuide.adoc

+1.  User requests to list all items or only items of a specific `TYPE`
+2.  ResuMe shows a list of corresponding items
+3.  User checks for the `ID` of a specific item in the list to delete
+4.  User requests to edit a specific item in the list


I think this may be 'delete' instead of 'edit'

foochifa · 2020-03-06T02:49:25Z

docs/DeveloperGuide.adoc

+
+1.  User requests to list all items or only items of a specific `TYPE`
+2.  ResuMe shows a list of corresponding items
+3.  User checks for the `ID` of a specific item in the list to delete


This may be borderline specific? Maybe can combine with line 4?
Maybe can change to User requests to delete a specific item based on ID?
Then extension can still follow

foochifa · 2020-03-06T02:50:19Z

docs/DeveloperGuide.adoc

+3.  User checks for the `ID` of a specific item in the list to edit
+4.  User requests to edit a specific item in the list


Maybe can combine these two lines? Borderline specific.
Maybe can change to User requests to edit a specific item based on ID?
Then extension can still follow

zhuhanming

Excellent documentation! Really very, very impressed by all the UML diagrams and screenshots. I myself can't wait to use your app.

zhuhanming · 2020-03-26T01:33:07Z

docs/DeveloperGuide.adoc

@@ -64,13 +64,24 @@ image::LogicClassDiagram.png[]
 [discrete]


The Parser is still named AddressBookParser, need to change!

I agree with what @zhuhanming is pointing out.

zhuhanming · 2020-03-26T01:38:43Z

docs/DeveloperGuide.adoc

+
+.Component interactions for initialisation
+
+image::ArchitectureInitSequenceDiagram.png[]


Excellent diagram!

zhuhanming · 2020-03-26T01:39:17Z

docs/DeveloperGuide.adoc


-.Component interactions for `delete 1` command
 image::ArchitectureSequenceDiagram.png[]


The Save to file action should be changed to its method name. Furthermore, the arrow is detached from the activation bar, should be touching instead.

Out of curiosity, why are there UML notes for your sequence diagram? Are these notes really helpful in this case, or can the reader get the same information from your descriptive method names?

zhuhanming · 2020-03-26T01:40:10Z

docs/DeveloperGuide.adoc

@@ -79,14 +90,15 @@ image::UiClassDiagram.png[]

 *API* : link:{repoURL}/src/main/java/seedu/address/ui/Ui.java[`Ui.java`]

-The UI consists of a `MainWindow` that is made up of parts e.g.`CommandBox`, `ResultDisplay`, `PersonListPanel`, `StatusBarFooter` etc. All these, including the `MainWindow`, inherit from the abstract `UiPart` class.
+The UI consists of a `MainWindow` that is made up of parts e.g.`CommandBox`, `ResultDisplay`, `ItemDisplay`, `PersonListPanel`, `StatusBarFooter` etc. All these, including the `MainWindow`, inherit from the abstract `UiPart` class.


zhuhanming · 2020-03-26T01:40:52Z

docs/DeveloperGuide.adoc

@@ -98,13 +110,13 @@ image::LogicClassDiagram.png[]
 *API* :
 link:{repoURL}/src/main/java/seedu/address/logic/Logic.java[`Logic.java`]


This looks like the same diagram as before, not sure if it'll be good to replace one of the two.

zhuhanming · 2020-03-26T01:42:27Z

docs/DeveloperGuide.adoc

 .  The result of the command execution is encapsulated as a `CommandResult` object which is passed back to the `Ui`.
 .  In addition, the `CommandResult` object can also instruct the `Ui` to perform certain actions, such as displaying help to the user.

-Given below is the Sequence Diagram for interactions within the `Logic` component for the `execute("delete 1")` API call.
+Given below is the Sequence Diagram for interactions within the `Logic` component for the `execute("delete 1 i/ res")` API call.

 .Interactions Inside the Logic Component for the `delete 1` Command
 image::DeleteSequenceDiagram.png[]


zhuhanming · 2020-03-26T01:43:18Z

docs/DeveloperGuide.adoc

@@ -114,6 +126,7 @@ NOTE: The lifeline for `DeleteCommandParser` should end at the destroy marker (X
 [[Design-Model]]
 === Model component

+// TODO: Fix diagram layout
 .Structure of the Model Component
 image::ModelClassDiagram.png[]


Perhaps one area of improvement is to make the numbers on the arrows easier to read. Some of them are overlapping and hard to read.

Another point: Believe that ReadOnlyUserPrefs should have an <<Interface>> header.

Some things to consider:

For the arrow pointing to the Model interface (not from any class), where should it start from?

Does the arrow pointing to the Model interface from ModelManager accurately reflect the relationship between the two?

Is Item an interface? If not, why are classes like Internship, Resume etc implementing Item?

zhuhanming · 2020-03-26T01:44:51Z

docs/DeveloperGuide.adoc

+===== Adding into the resume
+The following screenshot illustrates what happens when we use `redit` to add item into the resume. The numbers in the command represent the index of the item in the list. After the end of the command, the number of item in the resume would have increased.
+
+image::ReditAddIntoResume.png[][AddIntoResume,442,337]


Excellent screenshots!

zhuhanming · 2020-03-26T01:45:40Z

docs/DeveloperGuide.adoc

+image::ReditTags.png[][ReditWithTags,442,337]
+
+==== Design Considerations
+===== Aspect: Whether `ResumeEditCommand` should extend `EditCommand`


Excellent considerations here!

ChesterSim

Overall, still a great DG effort, if possible, make the Sequence Diagrams smaller so as to have a clearer view on the relevant parts!

ChesterSim · 2020-03-27T01:47:14Z

docs/DeveloperGuide.adoc

@@ -114,6 +126,7 @@ NOTE: The lifeline for `DeleteCommandParser` should end at the destroy marker (X
 [[Design-Model]]
 === Model component

+// TODO: Fix diagram layout
 .Structure of the Model Component
 image::ModelClassDiagram.png[]


Another point: Believe that ReadOnlyUserPrefs should have an <<Interface>> header.

ChesterSim · 2020-03-27T01:48:24Z

docs/DeveloperGuide.adoc


 == Implementation

 This section describes some noteworthy details on how certain features are implemented.

+=== [Proposed] Resume Edit feature


This section's screenshots feels more suited for a User Guide

I agree with @ChesterSim 's comment. Are there UML diagrams which can effectively capture the implementation details behind this feature instead?

ChesterSim · 2020-03-27T01:52:05Z

docs/DeveloperGuide.adoc

+ie. `AddItem` can add `Internship` to the `Internship` list, or add `Skill` to `Skill` list.
+
+****
+*Conclusion:* We went with our current design because it allows for each command type to only have one distinct job which


Good initiative for a conclusion!

ChesterSim · 2020-03-27T04:13:28Z

docs/DeveloperGuide.adoc

+
+The following sequence diagram shows how the `me` feature allows user to edit the user profile:
+
+image::MeSequenceDiagram.png[]


Me Feature Implementation Sequence Diagram is wrong → control at EditUserCommand has two concurrent calls → getUser() and new Person("Quoc Hung") called before either returns anything

I agree with @ChesterSim 's comment.

ChesterSim · 2020-03-27T04:15:00Z

docs/DeveloperGuide.adoc

@@ -218,21 +516,27 @@ image::CommitActivityDiagram.png[]



Commit Activity Diagram is unchanged!

I agree with @ChesterSim 's comment.

joanneong

I took a look at your DG, and focused on the Design and Implementation sections. Overall, the team has done a good job updating the DG :)

However, there are some small bugs that can be fixed for the DG to be even better. Keep it up!

joanneong · 2020-04-03T17:11:33Z

docs/DeveloperGuide.adoc

@@ -64,13 +64,24 @@ image::LogicClassDiagram.png[]
 [discrete]


I agree with what @zhuhanming is pointing out.

joanneong · 2020-04-03T17:27:15Z

docs/DeveloperGuide.adoc


-.Component interactions for `delete 1` command
 image::ArchitectureSequenceDiagram.png[]


Out of curiosity, why are there UML notes for your sequence diagram? Are these notes really helpful in this case, or can the reader get the same information from your descriptive method names?

joanneong · 2020-04-03T17:37:03Z

docs/DeveloperGuide.adoc

@@ -114,6 +126,7 @@ NOTE: The lifeline for `DeleteCommandParser` should end at the destroy marker (X
 [[Design-Model]]
 === Model component

+// TODO: Fix diagram layout
 .Structure of the Model Component
 image::ModelClassDiagram.png[]


Some things to consider:

For the arrow pointing to the Model interface (not from any class), where should it start from?

Does the arrow pointing to the Model interface from ModelManager accurately reflect the relationship between the two?

Is Item an interface? If not, why are classes like Internship, Resume etc implementing Item?

joanneong · 2020-04-03T17:39:31Z

docs/DeveloperGuide.adoc


 == Implementation

 This section describes some noteworthy details on how certain features are implemented.

+=== [Proposed] Resume Edit feature


I agree with @ChesterSim 's comment. Are there UML diagrams which can effectively capture the implementation details behind this feature instead?

joanneong · 2020-04-03T17:40:48Z

docs/DeveloperGuide.adoc

+
+* ** Alternative 2:** `ResumeEditCommand` extends `EditCommand`
+
+** Pros: Some methods in `EditCommand` may be able to inherited by `ResumeEditCommand`, reducing code duplication.


Can you spot a slight grammatical error in this sentence?

joanneong · 2020-04-03T17:44:02Z

docs/DeveloperGuide.adoc

+* ** Alternative 1 (current choice):** `ABCCommand` is separated into many `ABCItemCommand`.
+
+** Pros: More OOP. Flexible behaviour of `ABCItemCommand` and can be easily changed as required. No
+casing required. Each `ABCItemCommand` has it's own and distinct functionality.


Can you spot a small grammatical error here?

Also, what does "casing" mean?

joanneong · 2020-04-03T17:47:40Z

docs/DeveloperGuide.adoc

+
+Command sequence:
+
+1. User type `me` command in the command box.


Can you see the very slight grammatical mistake?

joanneong · 2020-04-03T17:49:15Z

docs/DeveloperGuide.adoc

+
+The following sequence diagram shows how the `me` feature allows user to edit the user profile:
+
+image::MeSequenceDiagram.png[]


I agree with @ChesterSim 's comment.

joanneong · 2020-04-03T17:52:17Z

docs/DeveloperGuide.adoc

+with the app.
+
+==== Change Background and Font Color
+This allows the user to change the background and font color using rgb or hex color codes.


Do you think listing these sections contribute to a reader's understanding of the implementation for these features?

joanneong · 2020-04-03T17:57:50Z

docs/DeveloperGuide.adoc

@@ -218,21 +516,27 @@ image::CommitActivityDiagram.png[]



I agree with @ChesterSim 's comment.

… into resume-book-test

… into Update-DG * 'master' of https://github.com/AY1920S2-CS2103T-F10-1/main: Remove Company A Modify SortCommandTest as suggested Add tests for `SortResumesCommand` and `SortSkillsCommand` Add tests for `SortNotesCommand` and `SortProjectsCommand` Add tests for `SortInternshipsCommand` Fix some initialization Add `SortCommandParserTest` Amends `equals` for sort commands a Initial commit for sort tests Add `equals` for the sort command Update UG: fix typo and format consistency Remove unused imports Refactor Sort commands

Fix diagrams size

Modify edit so that cannot edit to same item

… into model-manager-test

Update DG: introduction + model class diagram

Fix for my picture in UG to show

Model Manager Test

Add `UserPrefsTest` and `VersionedResumeBookTest`

Refactor rgen and help

Update UG's

update ug

Fix Christian typos in DG

DG changes

Documentation changes

Finalize UG

Hai

UG

New branch

DG

foochifa reviewed Mar 6, 2020

View reviewed changes

zhuhanming reviewed Mar 26, 2020

View reviewed changes

ChesterSim reviewed Mar 27, 2020

View reviewed changes

joanneong reviewed Apr 3, 2020

View reviewed changes

helloImHai and others added 26 commits April 13, 2020 00:57

Smaller diagrams

c9c939a

Further refine Christian's PPP

234ab16

Merge branch 'master' of https://github.com/AY1920S2-CS2103T-F10-1/main…

9b7d6f2

… into resume-book-test

Shorten Christian's PPP to fit 2 page limit

61e2859

Merge pull request #346 from helloImHai/dg

c5a0e1c

Fix diagrams size

Add TagPullSequenceDiagram to DG

618c85c

Merge

8cbf28b

Merge pull request #340 from helloImHai/master

b22ab75

Modify edit so that cannot edit to same item

Please show

384c8cf

Initial stages of setting up ModelManagerTest

3b73aac

Merge branch 'master' of https://github.com/AY1920S2-CS2103T-F10-1/main…

5dc640a

… into model-manager-test

Update DG: rgen diagrams

3e2f84d

Add notes to MMT

15d420b

Add notes, projects and resume

09d8d68

Refactor private methods in pdf builder

6f47224

Add the rest of ModelManagerTest

ba81b64

Merge pull request #325 from duongphammmm/dg

9aef69f

Update DG: introduction + model class diagram

Fuck me checkstyle

bac53c1

Merge pull request #347 from helloImHai/dg

0cfbdda

Fix for my picture in UG to show

Make ObservablePerson check for null for setPerson

46a55f6

Add tests for ResumeBook

5eb1db7

DG updates

dca458a

Merge pull request #348 from helloImHai/model-manager-test

519fa6d

Model Manager Test

Merge pull request #345 from wardetu/version-resume-book-test

4e62c98

Add `UserPrefsTest` and `VersionedResumeBookTest`

Merge pull request #337 from duongphammmm/refactor

23f00b1

Refactor rgen and help

wardetu and others added 30 commits April 13, 2020 22:01

Merge pull request #394 from wardetu/master

376d545

Update UG's

Fix typos

6a19c3a

update ug

432d499

Commit

4a58044

Merge pull request #395 from duongphammmm/documentation

62bdd8d

update ug

DG changes

44089e3

Merge pull request #397 from chrisjwelly/update-dg

0d9a0ad

Fix Christian typos in DG

Some more documentation changes

bdd401a

Merge pull request #398 from helloImHai/master

57a1b30

DG changes

Merge remote-tracking branch 'origin/master'

ca1fa9a

no message

8d0de43

Merge branch 'master' into master

66ce088

Merge pull request #399 from wardetu/master

60087aa

Documentation changes

Update docs/UserGuide.adoc

4689400

Hai

ee69764

Hai

509d951

Merge branch 'master' of https://github.com/AY1920S2-CS2103T-F10-1/main

268f112

Merge branch 'master' of https://github.com/helloImHai/main

28324fd

rm pdf

17a20ea

Merge pull request #396 from nhamhung/Update-UG

9e2add3

Finalize UG

Merge pull request #400 from helloImHai/master

d19cdd3

Hai

Finish

34c27db

UG

224cd3d

Merge pull request #401 from helloImHai/master

5f6eeb3

UG

Final

2207c6a

Merge branch 'master' into New-branch

941ca0b

Merge pull request #402 from nhamhung/New-branch

ff506ec

New branch

Fix DG

c84c380

Merge branch 'master' of https://github.com/AY1920S2-CS2103T-F10-1/main

cf6c638

Merge pull request #403 from helloImHai/master

98ca981

DG

		@@ -316,30 +316,73 @@ _{More to be added}_
		(For all use cases below, the System is the `AddressBook` and the Actor is the `user`, unless specified otherwise)

		3. User checks for the `ID` of a specific item in the list to edit
		4. User requests to edit a specific item in the list

		@@ -64,13 +64,24 @@ image::LogicClassDiagram.png[]
		[discrete]


		.Component interactions for initialisation

		image::ArchitectureInitSequenceDiagram.png[]


		.Component interactions for `delete 1` command
		image::ArchitectureSequenceDiagram.png[]

		@@ -98,13 +110,13 @@ image::LogicClassDiagram.png[]
		API :
		link:{repoURL}/src/main/java/seedu/address/logic/Logic.java[`Logic.java`]


		The following sequence diagram shows how the `me` feature allows user to edit the user profile:

		image::MeSequenceDiagram.png[]


		* Alternative 2: `ResumeEditCommand` extends `EditCommand`

		** Pros: Some methods in `EditCommand` may be able to inherited by `ResumeEditCommand`, reducing code duplication.


		Command sequence:

		1. User type `me` command in the command box.

[CS2103T-F10-1] ResuMe #21

Are you sure you want to change the base?

[CS2103T-F10-1] ResuMe #21

Conversation

chrisjwelly commented Feb 18, 2020

foochifa 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

zhuhanming 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChesterSim 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

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