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

[CS2113T-T12-3] OrgaNice! #9

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

Conversation

terrytay
Copy link

@terrytay terrytay commented Mar 3, 2020

@@ -0,0 +1,47 @@
Hello from
Copy link
Contributor

Choose a reason for hiding this comment

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

ACTUAL.TXT file should not be in git :-p

Choose a reason for hiding this comment

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

Thank you for the information Prof. We have stopped tracking ACTUAL.TXT in git.

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

@jichngan jichngan left a comment

Choose a reason for hiding this comment

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

Please look at the review as given in the DeveloperGuide.md document.

[comment]: # (@@author GanapathySanathBalaji)
### 2.1. Architecture

![Architecture](images/Architecture.png)

Choose a reason for hiding this comment

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

Good use of Architecture diagram to get an overview of the Software Architecture.

### 3.1. Scheduling Tasks


[comment]: # (@@author GanapathySanathBalaji)

Choose a reason for hiding this comment

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

You might have to explain what the "ref" frame as it is not included in our syllabus at the moment.


![Overall Sequence Diagram](images/Schedule_Overall.png)

The three reference frames used are as follows:

Choose a reason for hiding this comment

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

You might want to check why is there another instance of an object at end of the lifeline of an object.


Therefore, the first alternative is chosen, as it is easier to implement and lesser memory is used while
conducting the search.

Choose a reason for hiding this comment

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

Overall good usage of sequence diagrams to aid in the understanding of implementations. In the implementations section, it could be better if the steps during execution is listed out instead of writing the implementations in a long paragraph. Most of the sequence diagram has an object, which i assume is your main class, named as Duke. It could be better to either name it as main or your program name .

Therefore, the first alternative is chosen, as it is easier to implement and lesser memory is used while
conducting the search.

[comment]: # (@@author )

Choose a reason for hiding this comment

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

Most of the sequence diagrams do not follow the standard notation of having just one instance of the class on the top. There is no need to mention the class again at the bottom. Please see the attached screenshot.

Screenshot 2020-03-30 at 4 42 01 PM

Copy link

@Bencotti Bencotti left a comment

Choose a reason for hiding this comment

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

Great job detailing the workings of your project!

Comment on lines 78 to 94
1. Duke - Main component which controls the flow of execution.

1. Ui - Component used to get input from the user and display results on the monitor.

1. Parser - Component used to abstract out the command based on user's input, so that the command can be executed later.

1. Command - Component contains information and implementation on how to execute various types of commands.

1. Task - Component contains details about handling the task list and related operations.

1. StudyArea - Component contains details about handling queries for study area search.

1. Notes - Component contains details about Notes related operations.

1. Exception - Component contains the various types of exceptions encountered when OrgaNice! is run.

1. ResourceLoader - Component handles loading and saving of the task list and study area details to local storage.

Choose a reason for hiding this comment

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

I really appreciate this short portion, at a quick glance I'm able to understand what the core classes are purposed for. This will help new developers get up to speed fast, coupled together with the object diagram.

[comment]: # (@@author GanapathySanathBalaji)
### 2.1. Architecture

![Architecture](images/Architecture.png)

Choose a reason for hiding this comment

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

Would it make it clearer if different classes used unique colours or arrows to make it easier to differentiate the associations?

[comment]: # (@@author terrytay)

### 2.4. Notes Component
![Notes Component](images/NotesComponent.png)

Choose a reason for hiding this comment

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

I felt the arrow leading from ModuleManager was quite confusing as the branch suddenly became curved, and almost overlaps the top arrow. This might make some look for a separate origin rather than thinking of a split.

It might be helpful to describe what CommandStack is for as it's the only floating unlinked object.

* `User enters search key`

![Study Area Sequence_Diagram_Main](images/user_sL_interaction.png)
<div>Figure 9. Interaction between User and Study Area Search Interface</div>

Choose a reason for hiding this comment

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

Is the format for replying back to user correct? Since it is a response, it might be easier to standardise and use a dotted arrow instead as with the rest of the diagram, to avoid confusion.

sc-1


### 3.3. Operation of Notes

#### 3.3.1 Implementation

Choose a reason for hiding this comment

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

This sections was a little too wordy, it might be better to have simple diagrams to help envision the flow as with the diagrams at the start.

{Give non-functional requirements}

[comment]: # (@@author NizarMohd)
## Appendix D: Glossary

Choose a reason for hiding this comment

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

Very detailed glossary, it might be easier to reference if it was used in a table instead, what do you think?

[comment]: # (@@author terrytay)

### 2.4. Notes Component
![Notes Component](images/NotesComponent.png)

Choose a reason for hiding this comment

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

sc-2

The wrong UML notation might have been used for this object diagram, such as the naming convention of each object


{Give instructions on how to do a manual product testing e.g., how to load sample data to be used for testing}

Choose a reason for hiding this comment

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

This part helps a lot in understanding the architecture of the program and the different classes and their functions efficiently. Good job!

[comment]: # (@@author GanapathySanathBalaji)
### 2.1. Architecture

![Architecture](images/Architecture.png)

Choose a reason for hiding this comment

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

Maybe could consider making the arrows overlaps each other lesser so that it will be much neater or like bencotti suggested using unique colors or arrows. but good job anyway!


[comment]: # (@@author terrytay)

### 3.3. Operation of Notes

Choose a reason for hiding this comment

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

Perhaps can consider adding some diagram or tables to illustrate the points so that readers can follow and flow easily.

* *indoor_flag* - refers to "-i" flag
* *outdoor_flag* - refers to "-o" flag
* *size_flag* - refers to "-s" flag

Choose a reason for hiding this comment

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

Perhaps can consider putting this into a table or make the same space indentation so that it is easier to refer to.

* To test for accuracy of flags, test either "-p", "-i", "-o" or "-s {integer}"
#### Search with both, (1) location, name or address , and , (2) flags
* To test for accuracy, test "{location/name/address} {flags}".
* Since flags must come as a second argument in this case, test for "{flags} {location/name/address}"

Choose a reason for hiding this comment

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

DG is very detailed and explains the classes well. Can consider doing a full rundown of the sequence of events with a diagram or numbered too. Goodjob:)

[comment]: # (@@author GanapathySanathBalaji)
### 2.1. Architecture

![Architecture](images/Architecture.png)

Choose a reason for hiding this comment

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

Clear general flow of program using Architecture diagram. Perhaps the arrow from StudyArea to Ui should be separated from the arrow from Command to StudyArea to avoid confusion?

[comment]: # (@@author terrytay)

### 2.4. Notes Component
![Notes Component](images/NotesComponent.png)

Choose a reason for hiding this comment

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

Should the arrows be separated to clearly show the flow of the diagram?

4. Command : Package containing Command interface, Add command, Command Stack classes.

5. Parser : Class to parse commands for command-based operations.

Choose a reason for hiding this comment

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

Perhaps there should be an explanation for the role of the ModuleManager?


The following sequence diagrams explain how tasks are scheduled.

![Overall Sequence Diagram](images/Schedule_Overall.png)

Choose a reason for hiding this comment

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

Should the word size of the diagram be increased or maybe the long commands could be split into 2 lines? It's quite hard to read the diagram without going closer to the screen.

the method will iterate through the flags array and check if the Study Area's attribute matches each flag stated in
flags. If isAvailStudyArea returns as true for all flags, the Study Area is then added to the output list,
availStudyArea.

Choose a reason for hiding this comment

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

Maybe some of these descriptions should be placed between the diagrams for diagram explanation.


![Study_Area_Sequence_Diagram_subModules2](images/isAvailStudyArea.png)
<div>Figure 11. Interaction when isAvailStudyArea is invoked</div>
<br>

Choose a reason for hiding this comment

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

Clear diagram to showcase how the method works. Good job!

NizarMohd and others added 24 commits April 8, 2020 00:04
* 'master' of https://github.com/AY1920S2-CS2113T-T12-3/tp:
  Update regarding Github pages
  Make clear command request for confirmation before clearing list
  Introduce 3 new changes: 1) Both 'y' and 'Y' can be used for remove 2) Changed messages to be more meaningful 3) Trim whitespace for notes command
* master:
  Update regarding Github pages
  Make clear command request for confirmation before clearing list
  Introduce 3 new changes: 1) Both 'y' and 'Y' can be used for remove 2) Changed messages to be more meaningful 3) Trim whitespace for notes command

# Conflicts:
#	src/main/java/notes/ModulesList.java
#	src/main/java/notes/NotesInvoker.java
#	src/main/java/ui/Constants.java
…nto branch-study-area

* 'branch-study-area' of https://github.com/NizarMohd/tp:

# Conflicts:
#	src/main/java/ui/Constants.java
* 'master' of https://github.com/AY1920S2-CS2113T-T12-3/tp:
  edit constants
  edit constants
  edit changes based on module manager
  Update regarding Github pages
  edit to pass check style
  edit to pass checkstyle
  integrated Ui with Notes
  Make clear command request for confirmation before clearing list
  Introduce 3 new changes: 1) Both 'y' and 'Y' can be used for remove 2) Changed messages to be more meaningful 3) Trim whitespace for notes command
  add exceptions to calendar
  edit UG and add exception to Calendar
  edit UG
  edit DG and add Calendar View to UG
terrytay and others added 30 commits April 11, 2020 14:26
* 'master' of https://github.com/AY1920S2-CS2113T-T12-3/tp: (26 commits)
  Update terrytay.md
  Update nizarmohd.md
  Update hongquan448.md
  Update ganapathysanathbalaji.md
  Update AboutUs.md
  Update README.md
  Update DeveloperGuide.md
  Update UserGuide.md
  Update README.md
  Update terrytay.md
  Update hongquan448.md
  Update ganapathysanathbalaji.md
  Update nizarmohd.md
  Update nizarmohd.md
  Update hongquan448.md
  Update terrytay.md
  Update ganapathysanathbalaji.md
  Update nizarmohd.md
  Update UserGuide.md
  Update DeveloperGuide.md
  ...
* 'master' of https://github.com/AY1920S2-CS2113T-T12-3/tp:
  Set theme jekyll-theme-cayman
  Delete Gemfile.lock
  Delete _config.yml
  Delete Gemfile
  Theme
  Theme
  Theme
  Test
  Delete Gemfile
  Test theme
  Update _config.yml
  Update _config.yml
  Update _config.yml
  Update DeveloperGuide.md
  Update UserGuide.md
  Update _config.yml
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