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-F14-4] NUSchedule #131

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

Conversation

Lee-Juntong
Copy link

NUSchedule helps students/staffs in NUS manage tasks of their daily routine, as well as help them to find the shortest route to their destination. It is optimized for CLI users so that frequent tasks can be done faster by typing in commands.

Comment on lines 105 to 114
![Location class diagram](diagrams/Location.jpeg)
*Figure 3.6.1 Class diagram for location component*

__API:__ `LocationList.java` <br>

The `LocationList` is made up of an ArrayList of Locations, which is a type of variable that stores different information
for different types of location (eg. `Building`, `Hostel`, etc.). However, do note that any place that is
not within the list of saved locations will be saved as type `OutOfNUS`, and would not be saved into the `location.txt`
file. The location will still be part of the location list before the app closes, and it will also be
saved as part of the Event information.
Copy link

Choose a reason for hiding this comment

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

Is the Location class suppose to be an abstract class? Based on the enumeration, each location is of a certain type (building, hostel, LT or OutOfNUS) and there are classes for each of them, it looks like Location is an abstract class. Also, I think there should be an association between the Location and the enumeration

image

Comment on lines 120 to 123
<br>The `Location` component
* stores information about various types of locations
* prints the list of locations that is saved in the data file
* checks if a location is saved in the list and returns the location when asked
Copy link

Choose a reason for hiding this comment

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

Does the Location component stores the information about the various types of locations? Sounds like it this is the LocationList component.

Comment on lines 61 to 73
![architecture](../diagrams/architecture.png)

The Architecture Diagram above provides a high-level view of the design of NUSchedule. The app can be broken down into
5 different components:
1. Main: Initializes the other components and connects them with each other.
2. UI: Manages the User Interface that the user interacts with.
3. Logic: Interprets user commands.
4. Storage: Reads data from and writes data to the hard disk.
5. Model: Stores the data the app uses in memory.

__How the architecture components interact with each other__
The sequence diagram below shows how each individual component interacts with each other when the user inputs a command.
![sequence](../diagrams/ArchitectureSequence.png)
Copy link

Choose a reason for hiding this comment

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

Unable to load architecture and sequence images.

image

Comment on lines 202 to 209
### 4.5 Reminders

### 4.6 Sort events

### 4.7 View events

## 5. Documentation

Copy link

Choose a reason for hiding this comment

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

Content are missing

Comment on lines 225 to 234
|Version| As a ... | I want to ... | So that I can ...|
|--------|----------|---------------|------------------|
|v1.0|new user|see usage instructions|refer to them when I forget how to use the application|
|v2.0|user|find a to-do item by name|locate a to-do without having to go through the entire list|

|1.0|busy individual|keep track of both personal and school activities|avoid clashing events
|1.0|freshman|know module details and lesson venues|be on time for my activities
|1.0|forgetful person|be reminded of the deadlines for my assignments|submit on time
|1.0|tutor|know the estimated time for my students to travel to their next class|pace my lesson suitably
|1.0|hardworking student|track how much time I have spent studying\allocate my time efficiently
|2.0|exchange student|know the optimal path to reach my next destination|avoid getting lost
|2.0|user|find a to-do item by name|locate a to-do without having to go through the entire list|
|2.0|professor|Know whether my students have another lesson after mine and the expected time of travelling|Pace my lesson appropriately
Copy link

Choose a reason for hiding this comment

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

Table format is broken

image

Comment on lines 100 to 103
### 3.5 Model
__API__:`EventList.java`
The `Model` component stores an ArrayList, events, that represents the current list of events.

Copy link

Choose a reason for hiding this comment

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

Lack of description and diagrams.

image


### 3.6 Location
![Location class diagram](diagrams/Location.jpeg)
*Figure 3.6.1 Class diagram for location component*
Copy link

@zongxian-ctrl zongxian-ctrl Oct 29, 2020

Choose a reason for hiding this comment

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

Some formatting error might consider to review it before the final version update.

Comment on lines 198 to 200
The sequence diagram below shows exactly which methods, from which classes, are called to obtain the required location.
![](diagrams/Locate.jpg)
*Figure 4.4.1 Sequence diagram for locate function*

Choose a reason for hiding this comment

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

Should the lifeline of EventList and LocationList be extended longer?

Comment on lines +212 to +217
* NUS student or staff
* able to type quickly
* has quite a number of events to keep track of
* prefers to use desktop apps
* prefers using Command Line Interface (CLI) apps
* prefers typing instead of mouse interactions

Choose a reason for hiding this comment

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

Consider making it one sentence instead? For easier readability

Copy link

@chocomango chocomango left a comment

Choose a reason for hiding this comment

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

Invalid review
Sorry I submitted multiple reviews instead of comments, could the team kindly remove this?
Thank you.

Copy link

@chocomango chocomango left a comment

Choose a reason for hiding this comment

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

Invalid review
Sorry I submitted multiple reviews instead of comments, could the team kindly remove this?
Thank you.

Copy link

@chocomango chocomango left a comment

Choose a reason for hiding this comment

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

Just a few comments on diagrams.

whether there is any event has overlap timing with the newly added event.

The sequence diagram below shows the process of adding a new event.
![AddCommand Sequence Diagram](diagrams/AddCommand.png)<br>

Choose a reason for hiding this comment

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

image
Maybe for the objects in sequence diagram, you could use the instance:class format.
E.g. ui:Ui ,command:AddCommand and :NusSchedule

whether there is any event has overlap timing with the newly added event.

The sequence diagram below shows the process of adding a new event.
![AddCommand Sequence Diagram](diagrams/AddCommand.png)<br>

Choose a reason for hiding this comment

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

image
Should the invocation arrow in the sequence diagram be solid lines instead?

each other to run the program.

### 3.1 Architecture
![architecture](diagrams/architecture.png)<br>

Choose a reason for hiding this comment

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

image
After reading Ui.java, I believe the Ui component did not call parser or command. Perhaps, there should not be a directional arrow from Ui component to Logic component ?


## Table of Contents
1. [Introduction](#1-introduction)

Choose a reason for hiding this comment

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

The hyperlinks are not working on GitHub pages as there will changes when GitHub convert markdown into HTML files. In this case, changing it to (#introduce) will work on pages but not markdown. Maybe the team could review this and find the best way to make it work?

Copy link
Author

Choose a reason for hiding this comment

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

entering `edit 1 class <new event description>` to edit the one existing event.

Step 3. `editCommand()` function replaces the original event with the edited one.
![EditCommand Sequence Diagram](diagrams/EditCommand.png)<br>

Choose a reason for hiding this comment

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

image
Since the components are already colour coded in the architecture diagram, perhaps the team could use the same colour for the background for the packages too?

xuche123 and others added 25 commits November 1, 2020 22:24
Move new information parsing to edit command
Fix bug in editing of an event into a personal event
consider academics related personal event in next commit
consider academics related personal event in the next commit
# Conflicts:
#	src/main/java/event/SelfStudy.java
#	src/main/java/eventlist/EventList.java
#	src/main/java/parser/Parser.java
update Storage of self study. use equalsIgnorecase instead of equals …
Fixed locate and storage
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