[CS2113T-T12-4] PAC #14

lowjiayee · 2020-03-03T04:56:56Z

@lowjiayee
@harithadiv
@benchan911
@Anqi-nus

Updates to command parser and addreservationcommand

JensonWee · 2020-03-30T08:19:35Z

docs/DeveloperGuide.md

+`EventParser`.
+1. Then, the relevant class that corresponds to the command is created, with the information extracted 
+from the previous step passed into it. It modifies `Event` or `EventList`.
+1. These commands are then returned to `Duke.run()` to `execute()`. 


The diagram looks neat and very informative. Perhaps more description would aid the user a better understanding of the diagram.

JensonWee · 2020-03-30T08:24:15Z

docs/DeveloperGuide.md

+1. Once determined, the relevant class that corresponds to the type of command is created.
+1. Then, the class will execute base on its function. It modifies `AttendanceList`.
+1. These commands are then returned to `Duke.run()` to `execute()`. 
+


The diagram shows clearly what is been created when the function is executed. However, description of the attendance class is not detailed enough. Users might wonder where is AttendanceList? Perhaps a few sentence regarding the details of the objects will aid the user a better understanding of the class. Furthermore, what does the grey box represents?

Description of the flow could be more detailed. Perhaps you can include the reason why such methods are implemented as such. The Pros and Cons of the methods and alternative methods.

JensonWee · 2020-03-30T08:25:45Z

docs/DeveloperGuide.md

+###Calendar Implementation
+
+*Figure 2: Class diagram of the Calendar component*
+


Class diagram of the Calendar component is missing from DG. Is this going to be added in the future update? Perhaps telling the user that it would be added in the future would let the user know that it is going to be added.

JensonWee · 2020-03-30T08:30:28Z

docs/DeveloperGuide.md

+* Any classes (e.g. `Seminar`) that inherit from `Event` class will have similar control flow. 
+
+### Attendance
+![attendance](images/Attendance.png "Class diagram of Attendance component")        


The diagram does not display AttendanceList. Is this format correct? Perhaps an indication where AttendanceList is from would aid the user better understanding of the diagram.

Perhaps a sequence diagram will help users to understand the flow of the command execution?

Missing the AttendanceList class in the diagram. Agrees with previous comments about possibly adding a sequence diagram. Also, I feel that some arrows can be omitted.

JensonWee · 2020-03-30T08:31:55Z

docs/DeveloperGuide.md

+*Figure 2: Class diagram of the Calendar component*
+
+1. When a user enters a calendar-related command, the command is analysed by `CalendarCommandInterpreter`.
+1. Once determined, the relevant information (eg. semester, academic year) are extracted by `CalendarParser`.


Perhaps clearer description of what's 'relevant information' would aid the user in better understanding. Is semester and academic year the only thing that is extracted by the CalenderParser?

JensonWee · 2020-03-30T08:32:16Z

docs/DeveloperGuide.md

+![attendance](images/Attendance.png "Class diagram of Attendance component")        
+*Class diagram of the Attendance component*
+1. When a user enters an attendance-related command, the command is analysed by `AttendanceCommandInterpreter`. 
+1. Once determined, the relevant class that corresponds to the type of command is created.


Perhaps clearer description of what's 'relevantclass' would aid the user in better understanding.

JensonWee · 2020-03-30T08:42:20Z

docs/DeveloperGuide.md

+1. Then, either AddFirstSemester or AddSecondSemester class that corresponds the semester number is created. 
+1. Subsequently, it separates events by the required month and year in `CalendarList`
+1. These commands are then returned to `Duke.run()` to `execute()`. 
+


Description of the flow could be more detailed. Perhaps you can include the reason why such methods are implemented as such. The Pros and Cons of the methods and alternative methods.

thanhduc2000

Overall, some class diagrams can be improved. All diagrams should not be auto-generated and your team could use drawing tool online instead. Moreover, you could start working on sequential diagrams for easier explanation

thanhduc2000 · 2020-03-31T05:21:53Z

docs/DeveloperGuide.md

+![Command](images/Command.png "Class diagram of Command component")         
+*Class diagram of the Command component*  


The diagram is hard to follow since the image is compressed and not viewable in the website and also not zoomable. Moreover, you are using an auto generated diagram from the system, which is not recommended

thanhduc2000 · 2020-03-31T05:26:17Z

docs/DeveloperGuide.md

+![event](images/event.png "Class diagram of Event component")           
+*Class diagram of the Event component*


This diagram is informative and easier to follow. However, since this is auto-generated, there are some aspects that auto generated diagrams cannot express some terms in words like accessibility in methods (public, private, etc), and they refer to icons instead

thanhduc2000 · 2020-03-31T05:34:28Z

docs/DeveloperGuide.md

+### Attendance
+![attendance](images/Attendance.png "Class diagram of Attendance component")        


Should add AttendanceList alo since it is covered in ListAttendance

andy-aw-why · 2020-03-31T05:16:20Z

docs/DeveloperGuide.md

+{To be added in future revisions}
+
+### Command
+![Command](images/Command.png "Class diagram of Command component")         


Is it possible to resize or restructure this diagram such that it is bigger?

Is it possible to reorder the commands in the diagram so that the commands corresponding the same feature can be put together which makes it easier for the readers to understand?

And I think omitting the constructors or some less important methods can simplify the commands diagram. In this way, it will be more easier for you to resize or restructure the diagram.

May be better to redraw the commands separately

andy-aw-why · 2020-03-31T05:24:13Z

docs/DeveloperGuide.md


+## Feature Design and Implementation 
+### Event
+![event](images/event.png "Class diagram of Event component")           


Hi, is the format for this diagram (and other diagrams as well in this guide) correct? I think it is missing the private/public field for variable and method name.

Also, is this generated using SimpleUML plugin for intellij? I don't think it is recommended to auto generate the UML diagrams.

Might be better to redraw with the notations that we are more familiar with. Currently there are many arrows, some may be combined to reduce clutter.

Shannonwje

Overall DG is clear and well-explained. Some minor errors in formatting of the headers and a missing diagram but overall easy to understand.

Shannonwje · 2020-03-31T05:30:44Z

docs/DeveloperGuide.md

+* Any classes (e.g. `Seminar`) that inherit from `Event` class will have similar control flow. 
+
+### Attendance
+![attendance](images/Attendance.png "Class diagram of Attendance component")        


AttendanceList can't be seen inside the diagram, is this format correct? Maybe including AttendanceList would make understanding the diagram clearer, it can be in another diagram if AttendanceList can't fit into the current diagram or make the current diagram less neat.

Also, there is a grey box in some of the classes while it is not in some other classes. Is there some information missing or omitted in the diagram? Perhaps an explanation for the grey boxes would make it less confusing for the readers trying to understand the diagram

Shannonwje · 2020-03-31T05:32:47Z

docs/DeveloperGuide.md

+1. When a user enters an event-related command, the command is analysed by `EventCommandInterpreter`. 
+1. Once determined, the relevant information (e.g. index, name, time, date, venue) are extracted by 
+`EventParser`.
+1. Then, the relevant class that corresponds to the command is created, with the information extracted 


Diagram and explanation of the overall design and implementation looks neat but maybe giving an example of what you mean by relevant classes would make the understanding of the diagram better.

Unsure of what the relevant class refers to. Can be phrased more explicitly maybe.

Shannonwje · 2020-03-31T05:37:17Z

docs/DeveloperGuide.md

+1. Then, the class will execute base on its function. It modifies `AttendanceList`.
+1. These commands are then returned to `Duke.run()` to `execute()`. 
+
+###Calendar Implementation


The header for Calendar Implementation seems to be off, it appears as "###Calendar Implementation" rather than an actual header of the section.

One change would be to make it "### Calendar Implementation" with a space in between instead.

Another suggestion would be to be consistent with the header names, to be "### Calandar" to be consistent with the header names before like "### Event" and "### Attendance"

Shannonwje · 2020-03-31T05:39:14Z

docs/DeveloperGuide.md

+
+###Calendar Implementation
+
+*Figure 2: Class diagram of the Calendar component*


The class diagram for the calendar component seems to be missing, will it be added in the future? If it is currently not ready then maybe can consider not referencing to it now to avoid confusion for the readers.

Shannonwje · 2020-03-31T05:51:20Z

docs/DeveloperGuide.md

+* `acadamic year` is parsed into corresponding to only one year according to the semester in `EventParser` class.
+* Calendar view of the whole year is not available. Only semester 1 or 2 of an academic year can be viewed at a time.
+* Event name size must be less than 10 characters to be displayed neatly (current implementation), however
+it can be implement to truncate longer names to fit nicely


Overall Design and Implementation is neat and relatively easy to understand. Perhaps can consider adding in examples with sample inputs of how each implementation would work rather than simply explaining the flow.

Another suggestion to help the reader understand more about your choice of design and implementation would be stating the different methods of implementation, weighing the pros and cons and stating the reason for choosing a certain implementation over another.

SibingWu

Structures are generally good and clear. It would be better if you can add more sequence diagrams and replace the IDE-generated diagram

SibingWu · 2020-03-31T06:15:26Z

docs/DeveloperGuide.md

+### Command
+![Command](images/Command.png "Class diagram of Command component")         
+*Class diagram of the Command component*  


I suppose it would be better if the diagram is not so clustered? Also it seems that the IDE 0generated diagram is not encouraged for project submission

SibingWu · 2020-03-31T06:22:50Z

docs/DeveloperGuide.md

+1. Then, the class will execute base on its function. It modifies `AttendanceList`.
+1. These commands are then returned to `Duke.run()` to `execute()`. 
+
+###Calendar Implementation


It seems that the title here is not working well. Maybe consider to add one more space before "Calender Implementation"? i.e. ### Calendar Implementation instead of "###Calendar Implementation"

SibingWu · 2020-03-31T06:37:10Z

docs/DeveloperGuide.md

+### Attendance
+![attendance](images/Attendance.png "Class diagram of Attendance component")        


It seems that AttendanceList and EventList are missing in this diagram?

Consulted the prof: "It's OK to omit less important classes from the diagram"

SibingWu · 2020-03-31T06:39:52Z

docs/DeveloperGuide.md

+1. When a user enters an attendance-related command, the command is analysed by `AttendanceCommandInterpreter`. 
+1. Once determined, the relevant class that corresponds to the type of command is created.
+1. Then, the class will execute base on its function. It modifies `AttendanceList`.
+1. These commands are then returned to `Duke.run()` to `execute()`. 
+


Maybe you can add a sequence diagram here to illustrate the execution?

SibingWu · 2020-03-31T06:45:11Z

docs/DeveloperGuide.md

+### Performance
+![Performance](images/Performance.png "Class diagram of Performance component")     
+*Class diagram of the Performance component*  
+1. When a user enters a performance-related command, the command is analysed by `PerformanceCommandInterpreter`. 
+1. Once determined, the relevant class that corresponds to the command is created (e.g. AddPerformance, 
+DeletePerformance...), and ask for relevant information (e.g. event name, student name, student result) from the user. 
+1. Then, with the information extracted from the previous step passed into it. It modifies PerformanceList` under
+the event class correspond to the input event name.
+1. These commands are then returned to `Duke.run()` to `execute()`. 
+
+Note that:
+* All PerformanceList class should be created under an Event class. A PerformanceList class cannot exist 
+by its own. 
+* All Performance commands are line-by-line commands. This aims to assist the user with correct command format and
+prevent time wasted on key in wrong commands. 
+
+### Student List Collection
+![Student](images/Student.png "Class diagram of Student component")     
+*Class diagram of the Student component*  
+1. When a user enters an studentList-related command, the command is analysed by `StudentCommandInterpreter`. 
+1. Once determined, the relevant class that corresponds to the type of command is created.
+1. Then, the class will execute base on its function. It modifies `AttendanceList`.
+1. These commands are then returned to `Duke.run()` to `execute()`. 

-{Describe the value proposition: what problem does it solve?}
+Note that:
+* studentList-related commands can be executed without the existence of events.


I am not sure if you format this on purpose, but it seems that those parts look similar to Event, Attendance, and Calendar Implementation. Maybe these two parts also belong to Feature Design and Implementation section?

SibingWu · 2020-03-31T06:48:27Z

docs/DeveloperGuide.md

+
+1. When a user enters a calendar-related command, the command is analysed by `CalendarCommandInterpreter`.
+1. Once determined, the relevant information (eg. semester, academic year) are extracted by `CalendarParser`.
+1. Then, either AddFirstSemester or AddSecondSemester class that corresponds the semester number is created. 


Just minor format thing: maybe better to emphasize the two classes? AddFirstSemester or AddSecondSemester

sliu107

All the diagrams are class diagrams, I think adding some sequence diagrams is helpful for readers to understand how each part work and how they are connected to each other.

sliu107 · 2020-03-31T06:36:17Z

docs/DeveloperGuide.md

+{To be added in future revisions}
+
+### Command
+![Command](images/Command.png "Class diagram of Command component")         


Is it possible to reorder the commands in the diagram so that the commands corresponding the same feature can be put together which makes it easier for the readers to understand?

sliu107 · 2020-03-31T06:40:07Z

docs/DeveloperGuide.md

+{To be added in future revisions}
+
+### Command
+![Command](images/Command.png "Class diagram of Command component")         


And I think omitting the constructors or some less important methods can simplify the commands diagram. In this way, it will be more easier for you to resize or restructure the diagram.

sliu107 · 2020-03-31T06:46:05Z

docs/DeveloperGuide.md

+
+### Parser
+*Class diagram of the Parser component*  
+There are total of four Parser classes as shown below. Each Parser class correspond to a feature 


It seems the diagram of parser compont is missing here.If you are going to add it in the feature, I think you can draw a class diagram here and add a sequence diagram to give an example at the end of this part.

sliu107 · 2020-03-31T06:54:18Z

docs/DeveloperGuide.md

-## Design & Implementation
+### UI
+![Ui](images/Ui.png "Class diagram of Ui component")                
+*Class diagram of the UI component*   


Is it possible to show the high level association between UI and command class in the diagram? Or just add some description in words so that the reader can more easily understand how the ui is used in other classes and how ui be implemented.

ananda-lye

Maybe the generated diagrams can be used as reference only. As other reviewers have pointed out, the notation may not be entirely correct. Also, consider adding some sequence diagrams which will greatly aid in understanding how the classes interact with one another. Cheers ! :)

ananda-lye · 2020-03-31T07:33:01Z

docs/DeveloperGuide.md

+{To be added in future revisions}
+
+### Command
+![Command](images/Command.png "Class diagram of Command component")         


May be better to redraw the commands separately

ananda-lye · 2020-03-31T07:37:27Z

docs/DeveloperGuide.md


+## Feature Design and Implementation 
+### Event
+![event](images/event.png "Class diagram of Event component")           


Might be better to redraw with the notations that we are more familiar with. Currently there are many arrows, some may be combined to reduce clutter.

ananda-lye · 2020-03-31T07:54:48Z

docs/DeveloperGuide.md

+* Any classes (e.g. `Seminar`) that inherit from `Event` class will have similar control flow. 
+
+### Attendance
+![attendance](images/Attendance.png "Class diagram of Attendance component")        


Missing the AttendanceList class in the diagram. Agrees with previous comments about possibly adding a sequence diagram. Also, I feel that some arrows can be omitted.

ananda-lye · 2020-03-31T07:58:01Z

docs/DeveloperGuide.md

+1. When a user enters an event-related command, the command is analysed by `EventCommandInterpreter`. 
+1. Once determined, the relevant information (e.g. index, name, time, date, venue) are extracted by 
+`EventParser`.
+1. Then, the relevant class that corresponds to the command is created, with the information extracted 


Unsure of what the relevant class refers to. Can be phrased more explicitly maybe.

ananda-lye · 2020-03-31T07:59:15Z

docs/DeveloperGuide.md

+1. Then, the class will execute base on its function. It modifies `AttendanceList`.
+1. These commands are then returned to `Duke.run()` to `execute()`. 
+
+###Calendar Implementation


Seems to have some formatting error

Reformat the Developer Guide to improve Readability: Added Content Page

Also deleted ui.displayMessage()

* Replace System.out.println to display * Add list and delete to Event help * Use StringBuilder in printGetHelp() * Explain 'relevant class' better * Add blank lines for better visual * Change "Duke" to "PAC" * Add Architecture in DG * Change 'PAC' to 'Pac' * Delete StudentListStorageParser (implemented in StudentList) * Add Storage to DG * Force change filename from uppercase to lowercase * Force PACException.java to PacException.java * Remove 'exam' and 'tutorial' from help command * Now print error messages * Change sout to UI.display * Change ui.displayMessage() to UI.display() Also deleted ui.displayMessage() * Fix UI.display() not printing

Update DG

…into Anqi-Documentation

Edit DG manual testing

DG documentation

Allows app to continue running despite corrupted data

* adding studentCommandInterpreter SequenceDiagram * edit abit of PPP

Resize image.

Manual testing instruction

Jia yee PPP

Update docs

update font size.

* documentation bug for sorting fixed * fix a sentence * final update * adding the "." formatting * remove "by the user"

* documentation bug for sorting fixed * fix a sentence * final update * adding the "." formatting * remove "by the user" * grammar

damithc pushed a commit to damithc/tp-draft that referenced this pull request Mar 14, 2020

Merge pull request nus-cs2113-AY1920S2#14 from nguan1/master

558a6a1

Updates to command parser and addreservationcommand

JensonWee reviewed Mar 30, 2020

View reviewed changes

thanhduc2000 reviewed Mar 31, 2020

View reviewed changes

andy-aw-why reviewed Mar 31, 2020

View reviewed changes

Shannonwje reviewed Mar 31, 2020

View reviewed changes

SibingWu reviewed Mar 31, 2020

View reviewed changes

sliu107 reviewed Mar 31, 2020

View reviewed changes

ananda-lye reviewed Mar 31, 2020

View reviewed changes

benchan911 and others added 22 commits March 31, 2020 21:47

Fix Appendix Format & Edited Non-functional Requirements.

e168367

Add Storage to DG

841ef8c

Merge pull request #123 from benchan911/Benjamin-DeveloperGuideUpdate

abdfc08

Reformat the Developer Guide to improve Readability: Added Content Page

Force change filename from uppercase to lowercase

6ab955c

Merge branch 'master' into JiaYee-Guide

00479be

Force PACException.java to PacException.java

ed27874

Remove 'exam' and 'tutorial' from help command

fdccce4

Now print error messages

65d46fa

Change sout to UI.display

79da070

Change ui.displayMessage() to UI.display()

98e8cbf

Also deleted ui.displayMessage()

Fix UI.display() not printing

05021ba

Merge branch 'master' into JiaYee-Storage

e875a61

Fix closing an uninitialised FileWriter

050f0c0

Modify message printed when checking if a file exists

8bc57d8

Add status message to show that data is loaded

3ba91e0

Show status message that data is saved

04995df

Change expected output in text-ui-test

9156847

Merge branch 'master' into JiaYee-Event

b400fad

Add storage files to .gitignore

675d7ab

Use more user-friendly exception messages in EventParser

3b143fb

EventCI and SeminarCI now show more user-friendly exception messages

aa74795

Anqi-nus and others added 30 commits April 11, 2020 22:41

Merge branch 'master' into Anqi-Documentation

68ecfa0

Merge pull request #240 from Anqi-nus/Anqi-Documentation

9c60e82

Update DG

Edit DG manual testing

cb9cf21

Increase font size of feature heading.

21bfe24

Merge branch 'master' of https://github.com/AY1920S2-CS2113T-T12-4/tp …

7fa236a

…into Anqi-Documentation

Merge pull request #241 from harithadiv/branch-Calendar

f780d0f

Edit DG manual testing

Merge branch 'master' into JiaYee-Storage

261e987

Merge pull request #242 from Anqi-nus/Anqi-Documentation

b6e099c

DG documentation

Allows app to continue running despite corrupted data

e01936e

Merge pull request #243 from lowjiayee/JiaYee-Storage

733456b

Allows app to continue running despite corrupted data

adding studentCommandInterpreter SequenceDiagram (#244)

3649c6f

* adding studentCommandInterpreter SequenceDiagram * edit abit of PPP

Merge branch 'master' into JiaYee-Guide

6a6b86f

Add manual testing instruction for storage

9c4ed3b

Resize image.

8ba2978

Update.

8bfaf0d

Add manual testing instruction for deleting event

9cd8bab

Update diagram.

118beba

Merge pull request #245 from Anqi-nus/master

1629a53

Resize image.

Add manual testing instruction for adding event

ce1568e

Merge branch 'master' into JiaYee-PPP

9a20d92

Add my face inside

4f66216

Update to add manual testing

cf5466a

Merge pull request #246 from lowjiayee/JiaYee-Guide

a6322ac

Manual testing instruction

Merge pull request #247 from lowjiayee/JiaYee-PPP

3a03240

Jia yee PPP

Update docs

0d75006

Merge pull request #248 from Anqi-nus/master

64b8d0d

Update docs

update font size.

09e882d

Merge pull request #249 from Anqi-nus/master

1a36f25

update font size.

Changes for CS2101 (#250)

e1bf162

* documentation bug for sorting fixed * fix a sentence * final update * adding the "." formatting * remove "by the user"

UG bug (#251)

fa06f43

* documentation bug for sorting fixed * fix a sentence * final update * adding the "." formatting * remove "by the user" * grammar

		###Calendar Implementation

		Figure 2: Class diagram of the Calendar component

		![Command](images/Command.png "Class diagram of Command component")
		Class diagram of the Command component

		![event](images/event.png "Class diagram of Event component")
		Class diagram of the Event component

		### Attendance
		![attendance](images/Attendance.png "Class diagram of Attendance component")

[CS2113T-T12-4] PAC #14

Are you sure you want to change the base?

[CS2113T-T12-4] PAC #14

Conversation

lowjiayee commented Mar 3, 2020

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

thanhduc2000 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

Shannonwje 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

SibingWu 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

sliu107 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

ananda-lye 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