-
Notifications
You must be signed in to change notification settings - Fork 231
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.
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
CS2113-T13-3 Classify #37
base: master
Are you sure you want to change the base?
CS2113-T13-3 Classify #37
Conversation
…yao-defensiveCoding Branch zongyao defensive coding
docs/DeveloperGuide.md
Outdated
|
||
#### Implementation and rationale | ||
|
||
![InputParsingSequenceDigram](./diagrams/src/InputParsing/InputParsingSequenceDiagram.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there suppose to be a X at the end of lifeline?
docs/DeveloperGuide.md
Outdated
|
||
Seen below is an UML diagram of the relevant classes dealing with storage. | ||
|
||
![DataStoringUML](./diagrams/src/DataStoring/DateStoring.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job in adding the UML diagram to help with visualisation
|
||
### Value proposition | ||
|
||
{Describe the value proposition: what problem does it solve?} | ||
Classify serves as an attempt to modernise administrative tasks in education institutes, such as tuition centres or school environments. | ||
|
||
## User Stories |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -35,4 +212,21 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -35,4 +212,21 @@ | |||
|
|||
## Instructions for manual testing | |||
|
|||
{Give instructions on how to do a manual product testing e.g., how to load sample data to be used for testing} | |||
### Adding a student to the student list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the developer guide looks good! It is detailed and sufficient for a developer to understand the main structure of the code. However, please note some of the UML diagram improvements that could be considered. LGTM
|
||
This component ensures that the user parses in commands in a format that makes sense, which will modify the master list. | ||
|
||
![InputParsingUML](./diagrams/src/InputParsing/InputParsing.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the green 'C' be removed from the UML diagram? It seems that the official convention does not use the green 'C'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can refer to the plant UML documentation to remove the circles.
docs/DeveloperGuide.md
Outdated
|
||
#### Implementation and rationale | ||
|
||
![InputParsingSequenceDigram](./diagrams/src/InputParsing/InputParsingSequenceDiagram.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good try on the sequence diagram! However, should the studentInfo.txt be omitted from the sequence diagram? It would be better if there is a method call to the class which handles the modification of studentInfo.txt instead.
docs/DeveloperGuide.md
Outdated
|
||
Please see the diagram below to see how identifying a student works. | ||
|
||
![studentIdentification](./diagrams/src/DataStoring/Student%20Identification.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great attempt! I think some of the method calls to various classes seems to be returning multiple objects in the same activation bar, when there is only the initial input method call. Perhaps what you intend to show is 2 activation bars, each returning one object. Or within the same activation bar, there should be some input before there is a return value?
Also should the arrows which show the returning from the activation bar be dotted?
docs/DeveloperGuide.md
Outdated
|
||
Seen below is an UML diagram of the relevant classes dealing with storage. | ||
|
||
![DataStoringUML](./diagrams/src/DataStoring/DateStoring.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue here, would removing the green 'C' be better?
Expected: `view joe` now shows the Student details of a student with Name: joe, Phone Number: 11111111. Other fields that were left blank will reflect 'Unknown' or for date fields, today's date. | ||
3. Test case: `add` and when prompted for Name, `joe`. `11111111` when prompted for phone number, press enter to skip other fields. | ||
<br /> | ||
Expected: `view joe` shows the same results as when a student was added via `add joe`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a
tag here might help with the indentation issue!
docs/DeveloperGuide.md
Outdated
### Viewing a student's details | ||
1. View a student who has been added to the student list | ||
1. Prerequisites: Add one student named 'joe' to the list with the `add` command | ||
2. Test case: `view joe` | ||
<br /> | ||
Expected: Student's details shown correspond to the details input when `add` was used to add a student. | ||
3. Test case: `view dogman` | ||
<br /> | ||
Expected: No details are displayed, an error message stating 'Student not found!' is shown. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this section can be supported with code blocks showing the expected outputs that corresponds to the inputs given instead to make things clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall great work on the develop guide, but there may some errors with the sequence diagrams.
1. Reference to AB-3 Developer Guide | ||
|
||
- [Source](https://se-education.org/addressbook-level3/DeveloperGuide.html#proposed-undoredo-feature) | ||
- Used as template to structure this Developer Guide | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be a table of contents to give readers easy access to various parts of the developer guide? Maybe putting one can help with the navigability of the guide!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, a table of contents should be added.
docs/DeveloperGuide.md
Outdated
|
||
#### Implementation and rationale | ||
|
||
![InputParsingSequenceDigram](./diagrams/src/InputParsing/InputParsingSequenceDiagram.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the stickman and database image are unnecessary for the sequence diagram. Actually, shouldn't the database image be omitted as its irrelevant to the course?
@@ -35,4 +213,21 @@ | |||
|
|||
## Instructions for manual testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the manual testing section can have more commands to be more comprehensive (e.g delete, list, or edit)? Rather than testing each command in isolation, maybe can group them together for a single test(e.g show add first, then list, then delete, then list, etc......)?
docs/DeveloperGuide.md
Outdated
|
||
Please see the diagram below to see how identifying a student works. | ||
|
||
![studentIdentification](./diagrams/src/DataStoring/Student%20Identification.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the entity names be strictly classes? Having the names as methods like findStudentByName and findStudentByNumber might not follow the sequence diagram standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more extra comment on one of the diagrams
The `StudentSorter` class facilitates sorting of the list of students based on various criterion. | ||
As of the latest iteration, it sorts the master list of students in the order specified by the user. | ||
|
||
![StudentSorterUML](./diagrams/src/StudentSorter/StudentSorter.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to other diagrams in the develop guide, shouldn't the PlantUML notations / formatting be disabled (e.g. access modifier, green 'C')?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, seems like you guys are on the right path, just need to fill in the missing components and some tiny errors in the diagrams.
1. Reference to AB-3 Developer Guide | ||
|
||
- [Source](https://se-education.org/addressbook-level3/DeveloperGuide.html#proposed-undoredo-feature) | ||
- Used as template to structure this Developer Guide | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, a table of contents should be added.
|
||
This component ensures that the user parses in commands in a format that makes sense, which will modify the master list. | ||
|
||
![InputParsingUML](./diagrams/src/InputParsing/InputParsing.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can refer to the plant UML documentation to remove the circles.
docs/DeveloperGuide.md
Outdated
#### Design considerations | ||
- The parser must be effecive in breaking down the user's input into its constitutent commands, with further breakdown if an associated argument is added. | ||
- The parser should be quick and effective in understanding the user's input, with simple prompts given to help the user in correctly parsing the command through the input parser. | ||
- Allowing the user to input optional arguments. For example, the user could type `view <student name>`, which takes in the "student name" as an optional argument. This is to increase the robustness of the program, which accounts for the two types of users, one who just types in `view`, and the other as formerly mentioned above. | ||
- Error handling must be intuitive for the user, so that appropriate error messages are produced if the user does not input a valid command. The error handling should also be robust, to account for the event a user is incapable in following instructions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps consider splitting into user and developer design considerations
docs/DeveloperGuide.md
Outdated
|
||
#### Implementation and rationale | ||
|
||
![InputParsingSequenceDigram](./diagrams/src/InputParsing/InputParsingSequenceDiagram.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User does not need a ":"
|
||
### Value proposition | ||
|
||
{Describe the value proposition: what problem does it solve?} | ||
Classify serves as an attempt to modernise administrative tasks in education institutes, such as tuition centres or school environments. | ||
|
||
## User Stories |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User stories seems incomplete
|
||
This component ensures that the user parses in commands in a format that makes sense, which will modify the master list. | ||
|
||
![InputParsingUML](./diagrams/src/InputParsing/InputParsing.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please disable the symbols "C"
|
||
This component ensures that the user parses in commands in a format that makes sense, which will modify the master list. | ||
|
||
![InputParsingUML](./diagrams/src/InputParsing/InputParsing.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, you are using a package diagram! Maybe, the use of a Package Diagram might be confusing?
docs/DeveloperGuide.md
Outdated
|
||
#### Implementation and rationale | ||
|
||
![InputParsingSequenceDigram](./diagrams/src/InputParsing/InputParsingSequenceDiagram.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the activation bar is incomplete
docs/DeveloperGuide.md
Outdated
|
||
#### Implementation and rationale | ||
|
||
![InputParsingSequenceDigram](./diagrams/src/InputParsing/InputParsingSequenceDiagram.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the format of the sequence diagram hehe and maybe the images in the middle might not be needed?
…udentList and SubjectLists
…into ParseTextFile
Update functionality for the process command
…into Bug_Fixing
… other methods. Add option for user to delete file in cases where they are unable to manually rectify corruption by editing the info file(that they messed up)
Add handling for cases of file corruption that are not handled by our…
Add archive component to Dg
Fix typos in DG
Update PPP
Update DeveloperGuide.md
…into V2.1 # Conflicts: # docs/DeveloperGuide.md
Add User Story
…into bugfixv2.0
Bugfixv2.0
No description provided.