[CS2113-W10-2] ExchangeCourseMapper#17
[CS2113-W10-2] ExchangeCourseMapper#17chewycj wants to merge 1048 commits intonus-cs2113-AY2425S1:masterfrom
Conversation
okkhoy
left a comment
okkhoy
left a comment
There was a problem hiding this comment.
Overall, the code is clean. Some minor issues are highlighted in the review. Please take a look and fix.
| private static final Logger logger = Logger.getLogger(AddCoursesCommand.class.getName()); | ||
|
|
||
|
|
||
| private static boolean isValidCourseMapping(String nusCourseInput, String puCourseInput, |
There was a problem hiding this comment.
In general, all non-trivial methods need javadoc header comments
| System.out.println(INVALID_UNIVERSITY_INPUT); | ||
| System.out.println(LINE_SEPARATOR); | ||
| System.out.println(LIST_RELEVANT_PU); | ||
| System.out.println(LINE_SEPARATOR); |
There was a problem hiding this comment.
Can you not use UI to do this job, rather than printing directly?
| String command = input.replaceFirst("help", "").trim().toLowerCase(); | ||
|
|
||
| switch (command) { | ||
| case "set": |
RCPilot1604
left a comment
RCPilot1604
left a comment
There was a problem hiding this comment.
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.
docs/DeveloperGuide.md
Outdated
|
|
||
| ### Class Diagrams | ||
| Command Package: | ||
|  |
There was a problem hiding this comment.
Would it better to split this diagram into smaller, more bite-sized chunks for better readability?
There was a problem hiding this comment.
Agreed, thank you for the feedback, we will work on it!
docs/DeveloperGuide.md
Outdated
| * Line Separator is used to ensure readability and ease of use for users. | ||
|
|
||
| #### Sequence Diagram on PlantUML: | ||
|  |
There was a problem hiding this comment.
Should there be a third activation bar for "print PU and PU course code"?
There was a problem hiding this comment.
Also should "throw new IOException()" be depicted as a solid black arrow within Command?
docs/DeveloperGuide.md
Outdated
| * Assertions and logging are used for error handling. | ||
|
|
||
| #### Sequence on PlantUML: | ||
|  |
There was a problem hiding this comment.
Should there be an activation bar and dotted return arrow for "print course details"?
| opt inputStream is null | ||
| Command -> Command: throw new IOException() | ||
| end |
| opt inputStream is null | ||
| Command -> Command: throw new IOException() | ||
| end |
| ListSchoolCommand --> CEG_Student | ||
| deactivate ListSchoolCommand |
There was a problem hiding this comment.
Thank you for the feedback! May we clarify the meaning of "No group"?
| #### 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. | ||
|
|
There was a problem hiding this comment.
I guess this part is redundant and duplicated?
docs/DeveloperGuide.md
Outdated
|
|
||
| ### Class Diagrams | ||
| Command Package: | ||
|  | ||
|
|
||
| {TODO: Object Diagram} |
There was a problem hiding this comment.
Perhaps you can try to omit some unnecessary details to make it more readable.
| 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, |
There was a problem hiding this comment.
maybe you can try to illustrate other components here ?
| * 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. |
There was a problem hiding this comment.
Perhaps adding some diagrams here will make it easier to understand?
limkongkiat
left a comment
limkongkiat
left a comment
There was a problem hiding this comment.
Good job overall, just a few nits with some of the diagrams. Keep it up!
| * Line Separator is used to ensure readability and ease of use for users. | ||
|
|
||
| #### Sequence Diagram on PlantUML | ||
|  |
There was a problem hiding this comment.
Perhaps you could standardize whether you use User or CEG_Student?
There was a problem hiding this comment.
Agreed, our team will work on it!
| * Line Separator is used to ensure readability and ease of use for users. | ||
|
|
||
| #### Sequence Diagram on PlantUML | ||
|  |
There was a problem hiding this comment.
Perhaps it would be better to standardize whether or not there are activation bars?
| * Line Separator is used to ensure readability and ease of use for users. | ||
|
|
||
| #### Sequence Diagram on PlantUML | ||
|  |
There was a problem hiding this comment.
Maybe you could ensure that all arrows are properly connected to the bar?
| readability. | ||
|
|
||
| #### Sequence Diagram on PlantUML: | ||
|  |
There was a problem hiding this comment.
Perhaps you could include an [else] label for the first alt block for greater clarity?
glenda-1506
left a comment
glenda-1506
left a comment
There was a problem hiding this comment.
Great job! The diagrams were well-drawn with minor errors. Perhaps you could consider colour-coding your sequence diagrams like the Architecture Diagram?
| * 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. |
There was a problem hiding this comment.
As these 2 lines are repeated throughout the DG, perhaps you could put it at the top and just mention it once?
There was a problem hiding this comment.
That's true, it is repeated throughout our DG 😮 we will extract it out and mention it once, thank you for the feedback!
| readability. | ||
|
|
||
| #### Sequence Diagram on PlantUML: | ||
|  |
There was a problem hiding this comment.
Should this be CEG_Student, instead of User?
docs/DeveloperGuide.md
Outdated
| * Line Separator is used to ensure readability and ease of use for users. | ||
|
|
||
| #### Sequence Diagram on PlantUML | ||
|  |
There was a problem hiding this comment.
Should there be a dotted return arrow for the deleteCourse(listIndex : int)?
docs/DeveloperGuide.md
Outdated
| |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 | |
There was a problem hiding this comment.
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
…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
11 participants
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.