-
Notifications
You must be signed in to change notification settings - Fork 5
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
Update DG (JY) #505
Update DG (JY) #505
Conversation
Updated implementation for register account in terms of: * Sequence diagram * Activity diagram * Their descriptive text
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #505 +/- ##
=========================================
Coverage 84.35% 84.35%
Complexity 1029 1029
=========================================
Files 121 121
Lines 3158 3158
Branches 370 370
=========================================
Hits 2664 2664
Misses 379 379
Partials 115 115 ☔ View full report in Codecov by Sentry. |
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.
Some comments
1. The user specifies the `USERNAME`{.swift}, `PASSWORD`{.swift}, `CONFIRM_PASSWORD`{.swift}, `SECRET_QUESTION`{.swift} | ||
and `ANSWER`{.swift} in the `register`{.swift} command. | ||
1. If any of the fields is not provided, an error message with the correct command format will be shown. | ||
1. If command parameters provided are not within the acceptable value range, an error message with the correct command format will be 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.
Would value range sound abit wrong, since its not really a range?
1. If not all fields are present, a `ParseException` is thrown saying incorrect command format. | ||
1. If all fields are present, it checks if `PASSWORD`{.swift} and `CONFIRM_PASSWORD`{.swift} match. |
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.
Would a check for password format be missing here?
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.
LGTM
35a4088
into
AY2324S1-CS2103T-T13-3:master
Updated implementation for register account in terms of:
Close #503