Skip to content
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

[CS2103-W15-2] HireLah! #32

Open
wants to merge 466 commits into
base: master
Choose a base branch
from

Conversation

agnesnatasya
Copy link

Copy link

@chishanw chishanw left a comment

Choose a reason for hiding this comment

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

Overall, quite good! You can consider adding more diagrams especially for your implementation. Or more sequence diagrams to explain how interviews and other features work.


NOTE: The lifeline for `DeleteCommandParser` should end at the destroy marker (X) but due to a limitation of PlantUML, the lifeline reaches the end of diagram.
NOTE: The lifeline for `AddCommandParser` should end at the destroy marker (X) but due to a limitation of PlantUML, the lifeline reaches the end of diagram.

[[Design-Model]]
=== Model component

Choose a reason for hiding this comment

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

This diagram might be too complicated. You may want to limit the scope of the diagram by breaking it down into smaller fragments. Perhaps you can put the ModelManager stuff as one diagram, and the Session + things extending from Session as another diagram? Or you can split Interviewee, Transcript etc into their own diagrams.

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

Choose a reason for hiding this comment

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

AddCommandParser should parse("attribute leadership") instead, since NormalParser has already parsed it into an Add command.

Choose a reason for hiding this comment

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

Additionally, the Model.getAttributeList().add("leadership") violates the Law of Demeter. The Model is peeking into the methods of attributes, but LoD aims to prevent objects navigating internal structures of other objects.

Copy link

@kevinputera kevinputera left a comment

Choose a reason for hiding this comment

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

Your team's DG looks great!

If I may suggest, you may try to break down your class diagrams into multiple levels of abstraction (especially since the number of classes involved is growing in size). Take for example the class diagram for Model. Perhaps, if you could first have an "architecture diagram" that presents an overall architecture of the Model component (how each constituent parts of Model interact with each other) before jumping deep into the complete class diagram of each of Model's constituent parts, things can be better explained.

In addition, it might be of interest to add in your Implementation section soon! It's pretty difficult currently (being newly introduced to the HireLah! project) to understand what's the purpose of certain parts of the design. For example, I may ask: Why do we need the ...PhaseParser classes? You can use the Implementation section to explain more about these!

@HemanshuGandhi
Copy link

Hi, some general comments:

  1. The lines in class diagram 2.2 are a little messy, as far as possible find an arrangement where most of the lines are straight, for better readability.
  2. Agreed with the previous comment on the model diagram, which can be divided into diagrams of different levels of abstraction.
  3. For presenting pros and cons of each implementation approach, you can consider using a table, whereby the pros and cons are listed in a more readable format horizontally within different columns

mario7lorenzo and others added 27 commits April 3, 2020 01:21
* List more than N intervieewees if tie

* Add BestIntervieweeListPanel class

* fix checkstyle

* Change the UI of BestInterviewee

* fix checkstyle

* limit the score from 0 to 10

* Update User Guide for Best Interviewees UI

* fix bug

* allow 0 for scoring
* updating from team main

* Add esstential UI components

* Add ToggleView

* remove binary files to be refetched from upstream

* Correct build error

* Correct build error in Transcript

* resolving build issue

* resolving build issue with AddCommandTest

* Minor updates

* Add list functionality and debug UI

* Update UML diagrams with upstream

* Delete redundant fxml file

* Rename Transcript to Remark in UI

* Change HireLah logo

* Update uml diagram for UI
* updating from team main

* Add esstential UI components

* Add ToggleView

* remove binary files to be refetched from upstream

* Correct build error

* Correct build error in Transcript

* resolving build issue

* resolving build issue with AddCommandTest

* Minor updates

* Add list functionality and debug UI

* Update UML diagrams with upstream

* Delete redundant fxml file

* Rename Transcript to Remark in UI

* Change HireLah logo

* update interviewee card images

* Fix UI bugs for V1.3

* change image

* Fix checkstyle violations

* Fix opening resume and help

* Fix NullPointerException in BarChart

* Refactor view resume button
* Add useful error message for Add Comands

* Add useful message for Delete Command

* Add useful message for BestCommand

* Add useful error messages for Edit Commands and Parser

* Add useful error message for Finalise Command and Parser

* Add useful error message for List Commands

* Add useful error message for Navigation Command

* Add useful error message for OpenReportCommand

* Modify tests to comply with the new error messages

* Fix minor bug at message format in NavigationTimeCommand

Co-authored-by: agnesnatasya <[email protected]>
…freshed

Update event handlers on IntervieweeListPanel
* Included various classes to support to support the function of saving the Metric, Interviewee, Attributes and question class.

* Merge branch 'master' of https://github.com/AY1920S2-CS2103-W15-2/main into Storage

* Clean and and fix the bugs for AttributeStorage, IntervieweeStorage, MetricStorage and QuestionStorage
Currently able to support the feature of saving metrics, attributes, questions and interviewee at the end of each interview session.

* to be finished

* Implement JsonSerializableTranscript and provide a validating method to
restore IntervieweeList from a stored session

* tidying up the code

* tidying and cleaning of the code

* Fix checkstyle and some bugs

* tidying and cleaning of the code

* updated to the User guide

* Merge branch 'master' of https://github.com/AY1920S2-CS2103-W15-2/main into Storage

# Conflicts:
#	docs/DeveloperGuide.adoc

* Update the pictures of the user guide.

Co-authored-by: Joash Chin <[email protected]>
Update ModelManager and event handlers
* Delete Close Command from UG

* Update User Guide

* Allows user to open resume during interview

* fix typo
* Verifies Edit attribute validates the name

* finalize v1.3.1
…Update-Photo

Revert "Update mario7lorenzo.png"
CornCobs and others added 30 commits April 13, 2020 22:14
Update UG to be more accurate
* updating from team main

* Add esstential UI components

* Add ToggleView

* remove binary files to be refetched from upstream

* Correct build error

* Correct build error in Transcript

* resolving build issue

* resolving build issue with AddCommandTest

* Minor updates

* Add list functionality and debug UI

* Update UML diagrams with upstream

* Delete redundant fxml file

* Rename Transcript to Remark in UI

* Change HireLah logo

* Add DG intro, update broken links, update Ui.png

* Update readme

* revert Ui.png link change

* Update UI component diagram and dg

* Update UI component diagram and dg

* Add TestFX and UI tests

* Update travis config

* Update travis config

* Update travis config

* Update build.gradle

* Update travis config

* Update travis config

* Update travis config

* Update travis config

* Update PPP and DG
Update app internal UG and corncobs ppp
Add some command tests and fix export to display question number
corrected the name on the Adoc file
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.

8 participants