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-T17-4] NUSDegs #29

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

Conversation

SebasFok
Copy link

@SebasFok SebasFok commented Oct 6, 2023

NUSDegs streamlines computing degree planning by offering personalized module schedules, tracking progress, and ensuring on-time graduation. It eliminates guesswork, reduces stress, and saves time and money for students while providing data-driven insights for academic institutions. It's a comprehensive tool for efficient and successful degree completion.

Brian030601 pushed a commit to Brian030601/tp that referenced this pull request Oct 19, 2023
Copy link

@YongbinWang YongbinWang 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. Tackling prerequisites and an external API is difficult but the implementation looks good so far. There needs to be more standardization throughout the project such as using a common Ui class, naming convention. There can also be better abstraction of your implementations by breaking down the logic into methods and using different command classes to handle different user inputs.

Comment on lines 36 to 38
if (words.length < 2) {
return "currentMajor";
}

Choose a reason for hiding this comment

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

Consider abstracting this into a clearer method to illustrate what you are trying to achieve.

}

public String updateMajor(String userInput) {
String[] words = userInput.split(" ");

Choose a reason for hiding this comment

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

Consider avoiding magic literals by using a constant for " ".

Comment on lines 70 to 74
case "hi": {
view.displayMessage("can put the commands here");
break;
}
case "hello": {

Choose a reason for hiding this comment

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

I guess this was used for initial local testing, please remember to remove it.


String[] words = userInput.split(" ");

String initialWord = words[0].toLowerCase();

Choose a reason for hiding this comment

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

Consider using a better variable name for initialWord, maybe command.

Comment on lines 113 to 122
case "major": {
String printMessageCommand = student.updateMajor(userInput);
switch (printMessageCommand) {
case "currentMajor":
view.displayMessage("Current major is " + student.getMajor() + ".");
break;
case "newMajor":
view.displayMessage("Major " + student.getMajor() + " selected!");
break;
case "invalidMajor":

Choose a reason for hiding this comment

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

Consider abstracting out into different Command classes. Extensive use of magic literals here, try to avoid them.

Comment on lines 97 to 99
System.out.println("Invalid Module Name");
} catch (IOException | InterruptedException e) {
System.out.println("Invalid Module Name");

Choose a reason for hiding this comment

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

Consider using a standardized Ui object to print output onto command line. The ModulePlannerController class is using CommandLineView.

Comment on lines 38 to 41
public void getDifference (ModuleList a, ModuleList b) throws InvalidObjectException {
//A - B
if (a == null || b == null) {
throw new InvalidObjectException("Null Inputs");

Choose a reason for hiding this comment

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

Consider using clearer variable names for your ModuleLists.

* @return true if the module exists in the list; false otherwise.
* @throws InvalidObjectException If moduleA is null.
*/
public boolean exists(String moduleA) throws InvalidObjectException {

Choose a reason for hiding this comment

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

Boolean method should have prefix such as is, has, was.

Comment on lines 55 to 57
System.out.println(moduleCompleted +
" cannot be marked as completed because of uncompleted prerequisites: "
+ unmetPrerequisites);

Choose a reason for hiding this comment

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

Consider using a standardized Ui object to execute your outputs. Avoid long strings, convert it to constants.

for (String key : modulesWithPreqs.keySet()) {
//If new unlocked mod isn't marked as complete or unlocked already
if (!unlockedModulesSet.contains(key) && !addToModulesCompleted.contains(key)) {
boolean allPrerequisitesMet = true;

Choose a reason for hiding this comment

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

Boolean variable should also have prefix such as is, has etc.

@@ -4,9 +4,123 @@

{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well}

## Design & implementation
## Design & implementation, Architecture

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

@ICubE- ICubE- Nov 1, 2023

Choose a reason for hiding this comment

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

It would be better to understand if there is a diagram for what you implemented.

- Required

# Implementation

Copy link

Choose a reason for hiding this comment

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

Maybe some developers would want to know how the user input is handled.


Response:
`CS2030S CS2040S CS2100 CS2101 CS2106 CS2109S CS3230`

Copy link

Choose a reason for hiding this comment

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

What data structure do you use to record the modules that user has taken?
How do you collect the modules data that user has taken?

Copy link

@Jonoans Jonoans left a comment

Choose a reason for hiding this comment

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

I think the DG could have more diagrams :D

@@ -4,9 +4,123 @@

{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well}

## Design & implementation
## Design & implementation, Architecture
Copy link

Choose a reason for hiding this comment

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

Maybe it would be helpful to include diagrams to illustrate the program flow?


#### Example 1: Calculate Remaining MCs

Command: `pace Y2/S1` (assuming that the user has completed 60 MCs from Y1S1 to Y2S1)
Copy link

Choose a reason for hiding this comment

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

Would it be helpful to have information about the parameter's use?

Command: `required`

Response:
Module requirements for major selected by user


## Product scope
Copy link

Choose a reason for hiding this comment

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

Would it help to include more user stories?

ryanlohyr and others added 24 commits November 9, 2023 14:29
…o-timetable-modify

Rohit/feat/add clear command to timetable modify
Add integration testing for add and delete features
…ts-info-and-search

Update integration test for info and search features
Refactor while loop for timetable function
…q, recommended and delete function (#180)

* Fix issue where application crashes when wifi is off(add, shift,prereq, recommended and delete function

* Fix test bug

* fix checkstyletest

* Fix checkstyle

* fix unit test

* erm

* fix unit test
…pdate

User Input format for TimetableMode
rohitcube and others added 30 commits November 14, 2023 02:08
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