Skip to content

Conversation

@gerhean
Copy link

@gerhean gerhean commented Feb 20, 2020

Copy link

@chrisjwelly chrisjwelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General comments for user stories:
Most stories seem to have the format of "I want to store this for future use".

|=======================================================================
|Priority |As a ... |I want to ... |So that I can...
|`* * *` |new user |see usage instructions |refer to instructions when I forget how to use the App
|`* * *` |user |trace all my internship application's contact | easily follow up on the application

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this user story be of the highest priority?

|`* * *` |user |tag each application with a status | track my internship application phase

|`* * *` |user |delete a person |remove entries that I no longer need
|`* * *` |self-reflecting user |mark what positions of internship I have been applying to | see which positions I have the best chance of getting and easily look up past internship application when applying to similar positions

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By marking positions of internship you have been applying to, would that help with seeing which positions you have the best chance of getting?

|`* * *` |self-reflecting user |mark what positions of internship I have been applying to | see which positions I have the best chance of getting and easily look up past internship application when applying to similar positions

|`* * *` |user |find a person by name |locate details of persons without having to go through the entire list
|`* * *` |user |set reminders for internship deadlines/appointments| make sure I do not miss any internship opportunities

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be more accurate to describe this as "so that I can promptly complete the internship tasks", instead of "missing any internship opportunities", as missing internship opportunities imply not applying to it in the first place?


|`*` |frequent interviewee |maintain a checklist of questions to ask the interviewer |

|`*` |first-time internship seeker |use the program as a guide to internship applications |learn how to start applying for an internship

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this have been more specific to describe how this program can act as a guide?

2. User requests to prioritise the Internship Application.
3. InternDiary updates the priority level of the Internship Application.
+
Use case ends

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same extension for UC2 might be applicable to UC3 too?

2. User requests to prioritise the Internship Application.
3. InternDiary updates the priority level of the Internship Application.
+
Use case ends

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding extension for UC3 when the application already has the specified priority level

Copy link

@junhaotan junhaotan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, try to add features and implementation in DG. Good effort to update the class diagrams.


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

The `Model`,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good effort on updating the Model class diagram

Copy link
Contributor

@joanneong joanneong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked through your DG, and focused on the Design and Implementation sections. In general, good job for updating the DG and removing references to the AddressBook! :)

However, I have left some comments and questions you might want to consider to improve your DG further. Let me know if you have any questions.

Also, @gerhean , can you amend the PR description and add your team members there? Here's an example. This is so that everyone in your team will receive notifications if there are comments made on the PR!

`EnteredCommandsHistory` allows the user to get the sort command template back
in just one press of the up arrow key so there is little hassle. +
Users do not have to remember the order to place the arguments to get the sort they want.
** Cons: Feels a little restrictive.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this con be phrased more clearly? In what way is this restrictive? Is it really restrictive or is it just how you 'feel' about it?


In particular, `Interview` will rely on the `ApplicationDate` and `Address` classes in the Model to implement `interviewDate` and `interviewAddress`

image::InterviewClassDiagram.png[]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's stick to the notations which we learnt in class! This means that:

  • the letter 'C' should not be shown in the class diagram
  • to indicate access level for each method (private/public etc), notations such as - or + should be used instead of colored circles
  • the composition association should have arrow heads pointing towards the parts in this whole-part relationship

These comments also apply to other class diagrams in your DG!

which is the primary logic parser for user input. The following sequence diagram will illustrate the process of invocation for
`InterviewAddCommand`. All other sub-commands will follow the same invocation format.

image::InterviewCommandSequenceDiagram.png[]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments/questions:

  • Why is there a strange : InterviewCommand in the sequence diagram?
  • Why do the lifelines lead back to instances at the bottom of the diagram?
  • Are the instances named correctly? (i.e. are you perhaps missing something like a :?)

* `EmailContainsKeywordsPredicate` -- Predicate to check if an internship application's `Email` field contains any
substring matching any words in the list supplied by its constructor `EmailContainsKeywordsPredicate(List<String>
keywords)`.
* `PriorityContainsNumbersPredicate` -- Predicate to check if an internship application's `Phone` field contains any
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...to check if an internship application's Phone field...

Is there a typo here?

`ApplicationDateIsDatePredicate`, `PriorityContainsNumbersPredicate` and `StatusContainsKeywordsPredicate` to get a
single predicate and passing that into the method `updateFilteredInternshipApplicationList()` of the `ModelManager`
instance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there appropriate UML diagram(s) that you can insert to complement your description?


3) The `unarchive` command moves an internship application from the archival list to the main list.

image::UnarchiveSequenceDiagram.png[]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GIven that the sequence diagram for unarchive is essentially the same as for archive, do you think that having this diagram is necessary?

`StatisticsWindow` serves as an additional graphical statistics interface for users to get a visual breakdown of their internship
application(s) in the form of a bar chart or pie chart.

Users will be able to bring up the `StatisticsWindow` by executing the command `stats`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide more details on the implementation for this feature? You can also consider using appropriate UML diagrams to illustrate your ideas!

** Pros: Users will be choose which list they want to view the relevant statistics for.
Works well with `archival`, `list`, and `find` commands that dynamically changes the list.
** Cons: Often re-computation upon changes in the filtered list may cause some performance bottleneck.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you spot the grammatical errors in this section?

wxwxwxwx9 and others added 30 commits April 13, 2020 14:20
* 'master' of https://github.com/AY1920S2-CS2103T-F10-2/main:
  Update find class diagram
  Update description
  Fix priority predicate description
  fix styling of interview in DG
* 'master' of https://github.com/AY1920S2-CS2103T-F10-2/main:
  get rid of some stuff
  Update PPP PRs
  remove floating PR header
  Add markdown syntax to DG
  Fix acsiidoctor error
  Remove extra fullstop
Update project title in PPP
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants