[CS2113-W10-2] ExchangeCourseMapper #17

chewycj · 2024-09-21T13:28:39Z

ExchangeCourseMapper allows users to plan their course mapping by listing the universities of interest, along with the specific courses and subject codes offered by each school. Users can quickly filter by NUS-coded modules or by partner universities (PU) when they want to view the relevant options.

okkhoy

Overall, the code is clean. Some minor issues are highlighted in the review. Please take a look and fix.

okkhoy · 2024-10-26T15:08:12Z

src/main/java/seedu/exchangecoursemapper/command/AddCoursesCommand.java

+    private static final Logger logger = Logger.getLogger(AddCoursesCommand.class.getName());
+
+
+    private static boolean isValidCourseMapping(String nusCourseInput, String puCourseInput,


In general, all non-trivial methods need javadoc header comments

okkhoy · 2024-10-26T15:09:04Z

src/main/java/seedu/exchangecoursemapper/command/AddCoursesCommand.java

+            System.out.println(INVALID_UNIVERSITY_INPUT);
+            System.out.println(LINE_SEPARATOR);
+            System.out.println(LIST_RELEVANT_PU);
+            System.out.println(LINE_SEPARATOR);


Can you not use UI to do this job, rather than printing directly?

okkhoy · 2024-10-26T15:10:36Z

src/main/java/seedu/exchangecoursemapper/command/HelpCommand.java

+        String command = input.replaceFirst("help", "").trim().toLowerCase();
+
+        switch (command) {
+        case "set":


need explicit fallthrough comment

RCPilot1604

Would it be possible extract out repeated sequential diagram segments using reference frames? For instance, ListSchoolCommand, FilterCoursesCommand, ObtainCoursesCommand etc all share createJsonObject which can be extracted out and the differences be depicted in a ref frame.

Overall, the sequence diagrams are a bit too long and complicated. Splitting them up using ref frames would be a good idea.

Also would it be a good idea to show the initial set up seqence as well in a sequence diagram that shows the full run-through of the code? Ref frames can be used here as well to simplify the sequence diagrams.

RCPilot1604 · 2024-10-30T05:13:34Z

docs/DeveloperGuide.md

+
+### Class Diagrams
+Command Package:
+![Class diagram for Commands](images/CommandClass.png)


Would it better to split this diagram into smaller, more bite-sized chunks for better readability?

Agreed, thank you for the feedback, we will work on it!

RCPilot1604 · 2024-10-30T05:17:25Z

docs/DeveloperGuide.md

+* Line Separator is used to ensure readability and ease of use for users.
+
+#### Sequence Diagram on PlantUML:
+![Filter Courses Sequence Diagram](images/FilterCoursesCommand.png)


Should there be a third activation bar for "print PU and PU course code"?

Also should "throw new IOException()" be depicted as a solid black arrow within Command?

RCPilot1604 · 2024-10-30T05:26:13Z

docs/DeveloperGuide.md

+* Assertions and logging are used for error handling.
+
+#### Sequence on PlantUML:
+![ListUniCourseCommand sequence diagram](images/ListUniCoursesCommand.png)


Should there be an activation bar and dotted return arrow for "print course details"?

TrungBui32 · 2024-10-30T05:35:31Z

docs/diagrams/ListSchoolsCommand.puml

No group (i.e ListSchoolCommand)

TrungBui32 · 2024-10-30T05:36:48Z

docs/diagrams/FilterCoursesCommand.puml

+opt inputStream is null
+    Command -> Command: throw new IOException()
+end


Exception arrow should go out

TrungBui32 · 2024-10-30T05:38:24Z

docs/diagrams/ListSchoolsCommand.puml

+opt inputStream is null
+    Command -> Command: throw new IOException()
+end


Exception arrow should go out

TrungBui32 · 2024-10-30T05:52:35Z

docs/diagrams/ListSchoolsCommand.puml

+ListSchoolCommand --> CEG_Student
+deactivate ListSchoolCommand


Should return something

TrungBui32 · 2024-10-30T05:54:19Z

docs/diagrams/FilterCoursesCommand.puml

Thank you for the feedback! May we clarify the meaning of "No group"?

MatchaRRR · 2024-10-30T05:35:29Z

docs/DeveloperGuide.md

+#### Why it is implemented that way:
+* The `execute` method is essential and unique to every command class so inheritance was used.
+* Every method in the class remains maintainable and has one responsibility this allows easy debugging and
+  refactoring.
+* By using inheritance, new command classes can easily extend the functionality of existing ones 
+  which reducing redundancy in the code
+* Logging and assertions helps the team of developers to follow through the command execution.
+


I guess this part is redundant and duplicated?

MatchaRRR · 2024-10-30T05:47:53Z

docs/DeveloperGuide.md

+
+### Class Diagrams
+Command Package:
+![Class diagram for Commands](images/CommandClass.png)
+
+{TODO: Object Diagram}


Perhaps you can try to omit some unnecessary details to make it more readable.

MatchaRRR · 2024-10-30T05:51:03Z

docs/DeveloperGuide.md

+The `UI`, `Parser` and `Storage` components (also shown in the diagram above),
+* defines its API in a class with the same name as the Package
+
+The `Command` component,


maybe you can try to illustrate other components here ?

MatchaRRR · 2024-10-30T05:55:53Z

docs/DeveloperGuide.md

+* The `AddCoursesCommand` class extends `Command` class where it overrides the `execute` method for
+  custom behaviour.
+* The command first reads a JSON file to obtain the names via `createJsonObject()` method from the
+  superclass.
+* The `trimString` method then removes the `add` command and checks whether the user gave any input after the command. 
+  The method would return the user's input without the command.
+* This input is then passed into the `parseAddCommand()` method to obtain the relevant information: NUS course code, 
+  name of partner university and partner university course code.
+* Along with the JSON Object created from the `createJSONObject()` method, the information extracted from the 
+ `parseAddCommand()` method would be passed to the `isValidInput()` method to verify the user's course mapping.
+* In the `isValidInput()` method, the `getPUCourseList()` method is called to verify the user's partner university is 
+  included in the dataset. An exception is thrown if the university is not found in the dataset.
+* Afterward, the `isValidCourseMapping` checks whether the NUS course code and PU course code are compatible for 
+  course mapping. 
+* If both checks above are passed, the course mapping would be added to the `myList.json` file.
+* Throughout the code, exceptions, assertions and logging are in place for better error handling. 
+* Line Separator is used to ensure readability and ease of use for users.


Perhaps adding some diagrams here will make it easier to understand?

limkongkiat

Good job overall, just a few nits with some of the diagrams. Keep it up!

limkongkiat · 2024-10-30T06:26:45Z

docs/DeveloperGuide.md

+* Line Separator is used to ensure readability and ease of use for users.
+
+#### Sequence Diagram on PlantUML
+![Add Courses Sequence Diagram](images/AddCoursesCommand.png)


Perhaps you could standardize whether you use User or CEG_Student?

Agreed, our team will work on it!

limkongkiat · 2024-10-30T06:31:12Z

docs/DeveloperGuide.md

+* Line Separator is used to ensure readability and ease of use for users.
+
+#### Sequence Diagram on PlantUML
+![Delete Courses Sequence Diagram](images/DeleteCoursesCommand.png)


Perhaps it would be better to standardize whether or not there are activation bars?

limkongkiat · 2024-10-30T06:33:29Z

docs/DeveloperGuide.md

+* Line Separator is used to ensure readability and ease of use for users.
+
+#### Sequence Diagram on PlantUML
+![Add Courses Sequence Diagram](images/AddCoursesCommand.png)


Maybe you could ensure that all arrows are properly connected to the bar?

limkongkiat · 2024-10-30T06:42:22Z

docs/DeveloperGuide.md

+  readability.
+
+#### Sequence Diagram on PlantUML:
+![Filter Courses Sequence Diagram](images/ObtainContactsCommand.png)


Perhaps you could include an [else] label for the first alt block for greater clarity?

glenda-1506

Great job! The diagrams were well-drawn with minor errors. Perhaps you could consider colour-coding your sequence diagrams like the Architecture Diagram?

glenda-1506 · 2024-11-01T10:42:10Z

docs/DeveloperGuide.md

+* Throughout the code, exceptions, assertions and logging are in place for better error handling.
+* Line Separator is used to ensure readability and ease of use for users.


As these 2 lines are repeated throughout the DG, perhaps you could put it at the top and just mention it once?

That's true, it is repeated throughout our DG 😮 we will extract it out and mention it once, thank you for the feedback!

glenda-1506 · 2024-11-01T10:45:04Z

docs/DeveloperGuide.md

+  readability.
+
+#### Sequence Diagram on PlantUML:
+![Filter Courses Sequence Diagram](images/ObtainContactsCommand.png)


Should this be CEG_Student, instead of User?

glenda-1506 · 2024-11-01T10:49:16Z

docs/DeveloperGuide.md

+* Line Separator is used to ensure readability and ease of use for users.
+
+#### Sequence Diagram on PlantUML
+![Delete Courses Sequence Diagram](images/DeleteCoursesCommand.png)


Should there be a dotted return arrow for the deleteCourse(listIndex : int)?

glenda-1506 · 2024-11-01T10:55:37Z

docs/DeveloperGuide.md

-|v2.0|user|find a to-do item by name|locate a to-do without having to go through the entire list|
+| Version | As a ...     | I want to ...                                                   | So that I can ...                                              |
+|---------|--------------|-----------------------------------------------------------------|----------------------------------------------------------------|
+| v1.0    | CEG students | see the possible Oceania and South East Asia partner university | see all my possible choices in those regions                   |


I think this is a typo, it should standardise this with the rest?

Add method to parse university abbreviations

…into improvement/parser-test

…into branch-TPD # Conflicts: # src/main/java/seedu/exchangecoursemapper/constants/Messages.java

…ository

…into branch-DocsUpdatev7

[Improvement] Fix minor grammatical errors and update CourseValidator sequence diagram

Update DG manual testing section

…into improvement/List-schools-implementation

…into branch-FixSequenceDiagram

[Improvement] Update Sequence diagrams

…ementation [Improvement] Update List school why it is implemented

…into branch-extractmethod

Extract method to align with AddCoursesCommand seq diagram

[Improvement] Display Image on DG

okkhoy reviewed Oct 26, 2024

View reviewed changes

RCPilot1604 reviewed Oct 30, 2024

View reviewed changes

TrungBui32 reviewed Oct 30, 2024

View reviewed changes

MatchaRRR reviewed Oct 30, 2024

View reviewed changes

limkongkiat reviewed Oct 30, 2024

View reviewed changes

glenda-1506 reviewed Nov 1, 2024

View reviewed changes

louisjoety and others added 24 commits November 10, 2024 16:12

Merge branch 'master' into improvement/parser-test

9928326

Fix checkstyle

996c759

Fix checkstyle

920dcd3

Remove myList.json from data folder

d1e9da1

Merge pull request #315 from hzxnancy/branch-UniShorthand

00d4c4a

Add method to parse university abbreviations

Extract out the validation segment of obtains

14f4854

Merge branch 'master' of https://github.com/AY2425S1-CS2113-W10-2/tp …

725fc90

…into improvement/parser-test

Add test cases when abbreviation is used

78a090e

Restructure obtain contacts file

a223995

Enable shortened school name

e493352

Add new test cases for shortened school names

7eef4f8

Fix grammar bugs in delete command messages

e2cb026

Add named constants

0626e17

Update diagrams to new implementation

38c16af

Merge branch 'master' of https://github.com/AY2425S1-CS2113-W10-2/tp …

b406c85

…into branch-TPD # Conflicts: # src/main/java/seedu/exchangecoursemapper/constants/Messages.java

Make ppp shorter and succint

b2a1775

Shorten length of PPP

a02dfad

Modify filter command to take in special NUS course codes

25fbaa7

Disable loggings for AddCoursesCommand, CourseValidator and CourseRep…

73a2d46

…ository

Modify invalid NUS course code exception message

737d35c

Disable loggings for CourseValidator

99e493d

Remove myList.json file

e077d6c

Update JUnit tests for filter command

c93c950

Update JUnit tests for delete command

30953d3

hzxnancy and others added 30 commits November 12, 2024 02:14

Merge branch 'master' of https://github.com/AY2425S1-CS2113-W10-2/tp …

d69763d

…into branch-DocsUpdatev7

Fix minor grammar mistakes in DG

67b597c

Update UG example description

7c09936

Update test case to follow new message

7909335

Merge pull request #367 from hzxnancy/branch-DocsUpdatev7

4ca950a

[Improvement] Fix minor grammatical errors and update CourseValidator sequence diagram

Update DG manual testing section

9e1df7d

Update DG instructions for data corruption

bc8bde5

Update DG instructions

28b71cc

Update DG instructions

236da8a

Merge pull request #369 from chewycj/branch-dg-bug-fixes

20da7e6

Update DG manual testing section

Add List schools DG implementation causes

a940bda

Update DG to prevent overflow in checkstyle

0007a21

Merge branch 'master' of https://github.com/AY2425S1-CS2113-W10-2/tp …

a8245e0

…into improvement/List-schools-implementation

Match DG to SD

afd9e73

Update the DG with new obtain diagram rectifications

7261e44

Modify sequence diagram

f1f7b8f

Merge branch 'master' of https://github.com/AY2425S1-CS2113-W10-2/tp …

80fd4de

…into branch-FixSequenceDiagram

Fix obtain diagram

61e8c66

Update Sequence diagram for the 3 classes

bbf3f4e

Extract method to align with seq diagram

f7d1a5b

Merge pull request #371 from Chiang-HuiXin/branch-FixSequenceDiagram

6e761a8

[Improvement] Update Sequence diagrams

Merge pull request #370 from louisjoety/improvement/List-schools-impl…

554c42d

…ementation [Improvement] Update List school why it is implemented

Remove unnecessary lines

1c57040

Merge branch 'master' of https://github.com/AY2425S1-CS2113-W10-2/tp …

dbd29b0

…into branch-extractmethod

Switch trivial statement

48f5d08

Update photo on DG

7abf5a2

Merge pull request #372 from hzxnancy/branch-extractmethod

d9002d7

Extract method to align with AddCoursesCommand seq diagram

Merge pull request #373 from louisjoety/improvement/display-imeage

e310ade

[Improvement] Display Image on DG

Refactored ListCommandCommands UG

202b77d

Merge pull request #374 from wahkia/feature/refactor-list-commands-UG

6e07573

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CS2113-W10-2] ExchangeCourseMapper #17

[CS2113-W10-2] ExchangeCourseMapper #17

chewycj commented Sep 21, 2024

okkhoy left a comment

okkhoy Oct 26, 2024

okkhoy Oct 26, 2024

okkhoy Oct 26, 2024

RCPilot1604 left a comment

RCPilot1604 Oct 30, 2024

chewycj Nov 3, 2024

RCPilot1604 Oct 30, 2024

RCPilot1604 Oct 30, 2024

RCPilot1604 Oct 30, 2024

TrungBui32 Oct 30, 2024

TrungBui32 Oct 30, 2024

TrungBui32 Oct 30, 2024

TrungBui32 Oct 30, 2024

TrungBui32 Oct 30, 2024

chewycj Nov 3, 2024

MatchaRRR Oct 30, 2024

MatchaRRR Oct 30, 2024

MatchaRRR Oct 30, 2024

MatchaRRR Oct 30, 2024

limkongkiat left a comment

limkongkiat Oct 30, 2024

chewycj Nov 3, 2024

limkongkiat Oct 30, 2024

limkongkiat Oct 30, 2024

limkongkiat Oct 30, 2024

glenda-1506 left a comment

glenda-1506 Nov 1, 2024

chewycj Nov 3, 2024

glenda-1506 Nov 1, 2024

glenda-1506 Nov 1, 2024

glenda-1506 Nov 1, 2024

		private static final Logger logger = Logger.getLogger(AddCoursesCommand.class.getName());


		private static boolean isValidCourseMapping(String nusCourseInput, String puCourseInput,

		ListSchoolCommand --> CEG_Student
		deactivate ListSchoolCommand

		* Throughout the code, exceptions, assertions and logging are in place for better error handling.
		* Line Separator is used to ensure readability and ease of use for users.

[CS2113-W10-2] ExchangeCourseMapper #17

Are you sure you want to change the base?

[CS2113-W10-2] ExchangeCourseMapper #17

Conversation

chewycj commented Sep 21, 2024

okkhoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RCPilot1604 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

limkongkiat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glenda-1506 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment