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

[CS2113-W13-2] easySEP #6

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

Conversation

joshuan98
Copy link

CEG students are able to efficiently query their local database of past mappings for EE, CG, and CS-coded modules at one go and store their shortlisted modules locally, allowing for ease of reference and follow-up at a later time. This will improve on the shortcomings of EduRec.

redders7 referenced this pull request in AY2223S1-CS2113-T18-1b/tp Oct 18, 2022
Copy link

@brian-vb brian-vb left a comment

Choose a reason for hiding this comment

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

Good work for this DG! It really helps me in understanding your program. Just needs a few more touch ups and you are good to go :)

@@ -8,8 +8,189 @@

{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}

Choose a reason for hiding this comment

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

Hello! There isn't an introduction on what your application is about :/
As well as any acknowledgement is missing as well.

@@ -8,8 +8,189 @@

{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}

Choose a reason for hiding this comment

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

You can actually omit this line :)


The following diagram illustrates the flow of the program, from initialisation to updating of the text file.

![User Storage Sequence Diagram](./images/UserStorage_Sequence.png)

Choose a reason for hiding this comment

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

Nice diagrams!!!


The following sequence diagram illustrates the relationship between the respective classes involved in the creation and execution of an add command.

![Add Command Sequence Diagram](./images/AddCommand_Sequence.png)

Choose a reason for hiding this comment

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

You might want to take separate screenshots as the sequence is huge and is not really readable.

Copy link

@chiewyx chiewyx 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 on the DG


The following sequence diagram illustrates the relationship between the respective classes involved in the creation and execution of an add command.

![Add Command Sequence Diagram](./images/AddCommand_Sequence.png)
Copy link

Choose a reason for hiding this comment

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

consider using reference frames for your sequence diagrams to help you break down your sequence diagrams


The following diagram illustrates the relationships between the two main user storage classes - UserStorage and UserStorageParser.

![User Storage Class Diagram](./images/UserStorage_Class.png)
Copy link

Choose a reason for hiding this comment

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

Consider removing the C next to your class names for class diagrams
image

Copy link

Choose a reason for hiding this comment

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

Class diagrams are nice with additional information on the navigability


The following diagram illustrates the relationships between the three main database classes - DatabaseStorage, DatabaseParser, and Database.

![Database Class Diagram](./images/Database_Class.png)
Copy link

Choose a reason for hiding this comment

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

Consider changing from square bullets to '+' and '-' to show public and private methods
image

Copy link

@chydarren chydarren left a comment

Choose a reason for hiding this comment

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

Pretty well done guide with very informative diagrams and lots of thought put behind the design/planning process. Might want to consider writing more explanation for the command classes, but reduce the detail-ness of the diagrams so that more concise information can be presented to the developers. Also, do take note of some parts that have not been taught in class - e.g. use of bubbles in the class headers to avoid deduction of marks.

Good job and jyjy!! ^^


Upon starting easySEP, the DatabaseStorage will load each line from data.csv, parse the line using DatabaseParser, and store the data into the Database.

Relevant exceptions are thrown when there are unexpected scenarios. For instance, if data.csv cannot be found at the given file path, a FileNotFoundException is thrown.
Copy link

@chydarren chydarren Oct 26, 2022

Choose a reason for hiding this comment

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

You may consider putting data.csv in a code block, e.g. data.csv so that it helps to enhance the readability / highlight the importance of the file?


The following diagram illustrates the relationships between the two main user storage classes - UserStorage and UserStorageParser.

![User Storage Class Diagram](./images/UserStorage_Class.png)

Choose a reason for hiding this comment

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

Great detailed class diagram that gives an overview of the functions in the parser. To aid readability and understanding, maybe you might want to reduce the functions that are shown in the diagram and show the more important ones instead?


The following diagram illustrates the flow of the program, from initialisation to updating of the text file.

![User Storage Sequence Diagram](./images/UserStorage_Sequence.png)

Choose a reason for hiding this comment

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

I like how there are separate dividers in the sequence diagram, i.e. Initialization and Getting User Input to demarcate the purpose very clearly. Nice!


In chronological order, the following diagrams illustrate the flow of the program for adding lessons, deleting lessons and displaying timetables to the user.

![Timetable Add Lesson Sequence Diagram](./images/Timetable_addLesson_Sequence.png)

Choose a reason for hiding this comment

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

Detailed and clear diagram. Perhaps might want to put the isValidDay(), isValidStartTime() etc in a combined ref rather than showing all on the diagram as these are trivial functions.

The Ui class is the cornerstone of the Duke program to facilitate interaction with the user. It is used to scan and collect user input, print error messages to the user upon invalid input commands,
and display the appropriate acknowledgements or required information based on the user's command.

The following diagram illustrates the methods within the Ui class that can be invoked by the other classes in Duke for the purpose of user interaction.

Choose a reason for hiding this comment

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

Nice looking class diagram with clearly written functions. I think you might want to consider adding like how the class is connected to other classes in the application, rather than showing it as a standalone UI class on its own

the specified Command format. To deal with parsing parameters, spaces in University names and Module codes are to be replaced with underscores.

The following class diagram illustrates the relationship between Command class and its subclasses as well as other classes related to Commands.
![Command Class Diagram](./images/Command_class.png)

Choose a reason for hiding this comment

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

Your command class diagram is not rendered on the website view (https://ay2223s1-cs2113-w13-2.github.io/tp/DeveloperGuide.html), might want to look into this


The following sequence diagram illustrates the relationship between the respective classes involved in the creation and execution of an add command.

![Add Command Sequence Diagram](./images/AddCommand_Sequence.png)

Choose a reason for hiding this comment

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

You may wish to consider removing the trivial functions from the sequence diagram so that more emphasis is placed on the important functions, and help improve readability of your diagram.


The following class diagram illustrates the relationship between the respective classes involved in the creation and execution of a list command.

![List Command Class Diagram](./images/ListCommand_Class.png)

Choose a reason for hiding this comment

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

I like that there is an enum class shown together with the other classes to reinforce how the whole feature works all-together. Nice!


The following class diagram illustrates the relationship between the respective classes involved in the creation and execution of a favourite command.

![Favourite Command Class Diagram](./images/FavouriteCommand_Class.png)

Choose a reason for hiding this comment

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

As we are not taught to use the circle bubbles in the diagrams, which I think it really makes the diagram more clear, you do however might want to consider taking the bubbles away?

![Exit Command Sequence Diagram](./images/ExitCommand_Sequence.png)

#### Help Command

Choose a reason for hiding this comment

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

(For all the command classes) Please write more explanation to support the diagrams rather than the short one to three sentences!


## Product scope

Choose a reason for hiding this comment

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

Don't forget to add this in!!

@joshuan98 joshuan98 changed the title [CS2113-T13-2] easySEP [CS2113-W13-2] easySEP Oct 26, 2022
The following sequence diagram illustrates the relationship between the respective classes involved in the creation and execution of a help command.

![Help Command Sequence Diagram](./images/HelpCommand_Sequence.png)

Choose a reason for hiding this comment

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

You are missing columns in your sequence diagram as UI, database, etc. are objects

A list command can be used to display all the module mappings in the database, all the universities in the database, or allow users to filter by NUS module code or partner university name.

The following class diagram illustrates the relationship between the respective classes involved in the creation and execution of a list command.

Choose a reason for hiding this comment

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

Nice graph! Can add more labels to the graph so that the graph is clearer

![UserUniversityListManager Diagram](./images/UserUniversityListManager_Class.png)

The following sequence diagram helps explain the key steps behind the main functions in UserUniversityListManager.

Choose a reason for hiding this comment

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

Since your program won't do anything in the case of input school does not exist, you can use opt instead of alt


## Instructions for manual testing
## 5. Instructions for manual testing

Choose a reason for hiding this comment

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

I am unsure if you missed this section or decided not to do it, as it is empty. Consider deleting it if you don't need it.

Copy link

@kaseykwok kaseykwok left a comment

Choose a reason for hiding this comment

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

Great diagrams drawn in the team. I am impressed by how well constructed are the diagrams.

the specified Command format. To deal with parsing parameters, spaces in University names and Module codes are to be replaced with underscores.

The following class diagram illustrates the relationship between Command class and its subclasses as well as other classes related to Commands.
![Command Class Diagram](./images/Command_Class.png)

Choose a reason for hiding this comment

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

May consider to rearrange the classes position so that the class diagram can be read more easily in the guide. The resolution of the image is quite low for reading.


The following sequence diagram illustrates the relationship between the respective classes involved in the creation and execution of a delete command.

![Delete Command Sequence Diagram](./images/DeleteCommand_Sequence.png)

Choose a reason for hiding this comment

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

You may consider adding \n to very long texts in the diagram to avoid the long text for prolonging the the diagram. It would be more readable this way.


The following sequence diagram helps explain the key steps behind the main functions in UserUniversityListManager.

![UserUniversityListManager Sequence](./images/UserUniversityListManager_Sequence.png)

Choose a reason for hiding this comment

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

Would it be better if you can split each main function into separate sequence diagrams? I think it would be more helpful if the diagrams are split apart with some more explanation added for each of them.


In chronological order, the following diagrams illustrate the flow of the program for adding lessons, deleting lessons and displaying timetables to the user.

![Timetable Add Lesson Sequence Diagram](./images/Timetable_addLesson_Sequence.png)

Choose a reason for hiding this comment

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

Is there a missing return variable for the getUniversity() call for both the addLesson() and deleteLesson() sequence diagrams?
image


The following class diagram illustrates the relationship between the respective classes involved in the creation and execution of a list command.

![List Command Class Diagram](./images/ListCommand_Class.png)

Choose a reason for hiding this comment

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

I noticed that the Command class is an abstract class. Would it be better to add annotations to label it as an abstract class? (e.g. {abstract})

image

Copy link

@deenliong deenliong left a comment

Choose a reason for hiding this comment

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

Very detailed PR with good description. Just note that some diagrams are really hard to see. Consider only displaying the main logic of the command.


The following diagram illustrates the relationships between the three main database classes - DatabaseStorage, DatabaseParser, and Database.

![Database Class Diagram](./images/Database_Class.png)

Choose a reason for hiding this comment

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

image

Can consider removing the circle C symbol as it is not the standard textbook convention taught to us.


The following diagram illustrates the flow of the program, from the initial loadDatabase call to the eventual completion of updating the entire database.

![Database Sequence Diagram](./images/Database_Sequence.png)

Choose a reason for hiding this comment

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

image

Is the activation bar suppose for DatabaseStorage suppose to start?


The following diagram illustrates the relationships between the three main database classes - DatabaseStorage, DatabaseParser, and Database.

![Database Class Diagram](./images/Database_Class.png)

Choose a reason for hiding this comment

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

Good that the multiplicity and directionality are all included!


![Timetable Class Diagram](./images/Timetable_Class.png)

In chronological order, the following diagrams illustrate the flow of the program for adding lessons, deleting lessons and displaying timetables to the user.

Choose a reason for hiding this comment

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

All 3 class diagrams are well explained and focused on the main flow.


The following sequence diagram illustrates the relationship between the respective classes involved in the creation and execution of an add command.

![Add Command Sequence Diagram](./images/AddCommand_Sequence.png)

Choose a reason for hiding this comment

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

Consider abstracting and explaining only the logic of the add command. The interaction with Duke class could be excluded and explain in an overall sequence diagram before all the command class.

Copy link

@richwill28 richwill28 left a comment

Choose a reason for hiding this comment

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

[DG Review]

  1. Nice that TOC was provided. Please make sure that the links are still working when the document is printed (pdf).

  2. Before going to the details, perhaps it's good to dedicate a short intro of the program for developers.

image

  1. The following diagram should be simplified. In addition, we should also elaborate on why such database class design were made.

image

  1. A diagram bug, there should be a colon before each object name.

image

  1. Also just a cosmetic issue, but the bottom boxes (objects) should be removed. It's not a standard syntax.

image

  1. Too much details in the diagram. Only the most important details can be included.

image

  1. This diagram too. The exceptions can probably be omitted.

image

  1. The following diagrams are barely readable.

image

image

  1. Minor cosmetic issue.

image

While it's good that there a lot of diagrams (maybe a bit too much), they are mostly to demonstrate the "what" of the code. A DG is meant to for future developers should they maintain the code. So it's quite important for us to also explain the "why", i.e. design decisions and some implementation details.

Copy link

@richwill28 richwill28 left a comment

Choose a reason for hiding this comment

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

[Code Review] Joshua

Code is quite okay overall. Please add more JUnit tests.

Comment on lines 55 to 57
/**
* Loads data from data.csv into database.
*/

Choose a reason for hiding this comment

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

Good habit for adding comments here and there. But in this case, it's unnecessary if the method name is already self-describing.

In general we only want to add comments when necessary. There are various reasons, but the main one is to prevent outdated comments. If there are too many comments it could become hard to maintain, so keep in mind.

DatabaseStorage.loadDatabase();

assertEquals(
"[COM162 Object Oriented Design and Programming with Java 15MCs in The University of "

Choose a reason for hiding this comment

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

In general we avoid hard-coded values when testing. But if the expected value is not volatile (e.g. not mutating due to changes in database, API, or other various reasons), then it should be fine.

Copy link

@richwill28 richwill28 left a comment

Choose a reason for hiding this comment

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

[Code Review] Ian

Code is fine overall. Since the codebase is not small (which means it's not easy to debug), we might want to reduce the overall coupling.

public Timetable() {
userTimetable = new HashMap<String, ArrayList<Lesson>>();
ArrayList<Lesson> mondayList = new ArrayList<Lesson>();
userTimetable.put("monday", mondayList);

Choose a reason for hiding this comment

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

The value of days could be implemented as en enumeration instead. It could help with reusability and case handling for the rest of the program, also makes it easier to reason about the code.

* @param universityName The name of the university for which the timetable is created.
*/
public void createTimetable(String universityName, boolean isLoadFromFile) {
assert universityName.length() > 0 : "Input university name cannot be empty";

Choose a reason for hiding this comment

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

Probably more appropriate to handle it as an exception?

Copy link

@richwill28 richwill28 left a comment

Choose a reason for hiding this comment

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

[Code Review] Zhi Hong

Please implement more JUnit tests.

Comment on lines 149 to 151
//timetableManager.createTimetable(createCommand.getUniversityName(), false);
//UserStorageParser.storeCreatedLists(userUniversityListManager);
//UserStorageParser.storeTimetable(timetableManager);

Choose a reason for hiding this comment

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

Unused comments should be removed.

Copy link

@richwill28 richwill28 left a comment

Choose a reason for hiding this comment

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

[Code Review] Tingjia & Alfred

Code looks fine overall.

* @param input PU name from user input
*/
public void createList(String input) {
assert input.length() > 0 : "Input school cannot be empty";

Choose a reason for hiding this comment

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

This should probably be handled with an exception instead?

alfred-leong and others added 30 commits November 6, 2022 19:03
Fix bug when user deletes data directory
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.