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-1] Meeting Organizer #3

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

Conversation

MeLoveCarbs
Copy link

No description provided.

damithc pushed a commit to damithc/tp-draft that referenced this pull request Mar 14, 2020
Construct the basic structure for reservation-related commands
damithc pushed a commit to damithc/tp-draft that referenced this pull request Mar 14, 2020
damithc pushed a commit to damithc/tp-draft that referenced this pull request Mar 14, 2020
damithc pushed a commit to damithc/tp-draft that referenced this pull request Mar 14, 2020
Copy link

@btricec btricec left a comment

Choose a reason for hiding this comment

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

Very cool app! Implementation of the module logic component was well described.

## 3. Implementation
This section describes some noteworthy details of how our application works in the backend.
### 3.1. Detailed implementation of modulelogic component
![modulelogic Component](images/TimetableParser.png)<br>
Copy link

Choose a reason for hiding this comment

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

Diagram is a little blurry and hard to see


## 3. Implementation
This section describes some noteworthy details of how our application works in the backend.
### 3.1. Detailed implementation of modulelogic component
Copy link

Choose a reason for hiding this comment

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

I think the description of the modulelogic component was very well done. However, would be good if some context was given in the DG as I had to refer to the UG to understand the implementation.

Copy link

@btricec btricec left a comment

Choose a reason for hiding this comment

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

Very cool app, functionalities were pretty well explained although DG is still a little sparse.


{Describe the value proposition: what problem does it solve?}

## User Stories
## Appendix B: User Stories

|Version| As a ... | I want to ... | So that I can ...|
|--------|----------|---------------|------------------|
Copy link

Choose a reason for hiding this comment

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

Should there be more user stories?

## 3. Implementation
This section describes some noteworthy details of how our application works in the backend.
### 3.1. Detailed implementation of logic.modulelogic component
![logic.modulelogic Component](images/TimetableParser.png)<br>
Copy link

Choose a reason for hiding this comment

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

dg_review

Should there be an activation bar for the TimetableParser constructor?

### 2.2. UI component

### 2.3. Module parsing logic component
The Module parsing logic component retrieves module information from NUSMODS API.
Copy link

Choose a reason for hiding this comment

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

Could a UML diagram be used here to describe the structure of the components?

From the figure above, ```ModuleApiParser``` instantiates a HTTP GET request object to fetch a Json object from the open-sourced NUSMOD API server, and is called by ```ModuleHandler``` every time a particular module information is requested.
<br>
```ModuleHandler``` cleans the data provided by ```ModuleApiParser``` and stores an easy to use data structure to be used by ```LessonsGenerator```.
![logic.modulelogic Component](images/LessonsGenerator.jpg)<br>
Copy link

Choose a reason for hiding this comment

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

dg_review2

Should the creation of a new LessonsGenerator point directly to the LessonsGenerator class since it is creating a new LessonsGenerator object?

Copy link

@JeremiasLiew JeremiasLiew left a comment

Choose a reason for hiding this comment

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

Overall, good job! I see that the DG isn't complete yet so all the best with that!

Comment on lines 115 to 118
|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|

Choose a reason for hiding this comment

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

Consider adding more user stories instead of just 1 each per version

From the figure above, ```ModuleApiParser``` instantiates a HTTP GET request object to fetch a Json object from the open-sourced NUSMOD API server, and is called by ```ModuleHandler``` every time a particular module information is requested.
<br>
```ModuleHandler``` cleans the data provided by ```ModuleApiParser``` and stores an easy to use data structure to be used by ```LessonsGenerator```.
![logic.modulelogic Component](images/LessonsGenerator.jpg)<br>

Choose a reason for hiding this comment

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

image

Shouldn't there be another activation box for the lessonsChecker() method of the LessonsGenerator?

Integer semester = Integer.parseInt(myTimetableParser.getSemester()) - 1;
for (String module : userModules) {
ModuleHandler myModuleHandler = new ModuleHandler(module);
message = myModuleHandler.generateModule();

Choose a reason for hiding this comment

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

image

Should this be generateModule() and not generate(Module) in the sequence diagram

Comment on lines 78 to 81
![logic.modulelogic Component](images/TimetableParser.png)<br>
The above figure shows```TimetableParser```, a private class called exclusively by ```LessonsGenerator```. It makes use of regex to sift through timetable link provided by user in the form of ```String``` object and stores
the semester and the user's module information according to the timetable link provided. It depends on the ```common.Messages``` class to provide the exception message when an incorrect link is being parsed.<br>
![logic.modulelogic Component](images/ModuleHandler.jpg)<br>

Choose a reason for hiding this comment

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

As the TimetableParser and ModuleHandler parts are already in the LessonsGenerator sequence diagram, consider using another type of diagram instead for these components.

Comment on lines +51 to +53
2. ```LessonsGenerator``` also uses ```Modulehandler``` to retrieve a set of information related to a specific module.
3. With both information, ```LessonsGenerator``` is able to dynamically generate the user's time-slots stored in ```ArrayList<String[]>``` via a series of Key-Value pair hashing.
4. ```Arraylist<String[]> ``` contains the start/end time, days and weeks of all modules the user is taking.

Choose a reason for hiding this comment

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

Consider giving the ArrayList<String[]> a name, or make it a separate class of its own, because referring to it as "ArrayList<String[]>" in the DG could be a bit confusing.

1. ```TeamMember``` consists of information of a member's schedule.
2. ```TeamMemberList``` is a ```Arraylist<TeamMember> ``` which new ```TeamMember``` can be added to.
3. ```ScheduleHandler``` retrieves the schedule of selected ```TeamMember``` in the ```TeamMemberList```, to generate a combined schedule.
### 2.5. Meeting component

Choose a reason for hiding this comment

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

image

Consider making sure that the subheaders are aligned, although I'm not too sure what is causing this issue in this case.

2. ```TeamMemberList``` is a ```Arraylist<TeamMember> ``` which new ```TeamMember``` can be added to.
3. ```ScheduleHandler``` retrieves the schedule of selected ```TeamMember``` in the ```TeamMemberList```, to generate a combined schedule.
### 2.5. Meeting component
The Meeting component of our application consists of 2 classes: ```Meeting MeetingList```

Choose a reason for hiding this comment

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

image

Consider making the 2 classes more obvious, maybe separate them with a comma or more spaces or something.

4. ```Arraylist<String[]> ``` contains the start/end time, days and weeks of all modules the user is taking.
<br>

### 2.4. Schedule component

Choose a reason for hiding this comment

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

Consider adding the detailed implementation of the schedule component as well.

* This class contains essential methods to filter out the modules and the timeslots the user
* is taking from the NUSMOD link.
*/
public class TimetableParser {

Choose a reason for hiding this comment

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

image
Is there a need for the new keyword?

Other than that, the diagrams are rather concise

@@ -0,0 +1,94 @@
package logic.modulelogic;

Choose a reason for hiding this comment

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

Should the explanation for the sequence diagram for TimetableParser in the DG have more elaboration and links to the diagram? I felt that there was not enough explanation for it.

On the other hand, the explanations for the other 2 diagrams were rather well linked to the sequence diagram.

@@ -0,0 +1,172 @@
import com.google.gson.JsonArray;

Choose a reason for hiding this comment

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

image
Should there be return lines added?

@@ -0,0 +1,172 @@
import com.google.gson.JsonArray;
import com.google.gson.JsonElement;
import com.google.gson.JsonObject;

Choose a reason for hiding this comment

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

I felt that section 3.2, Design Considerations was very useful. It takes into account the profile of an experienced developer, but is easy enough to understand for a new developer.

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.

7 participants