[CS2113-F14-3] VoyaTrip#39
Open
Lyam-T wants to merge 557 commits into
Open
Conversation
Ashertan256
added a commit
to Ashertan256/tp
that referenced
this pull request
Mar 20, 2025
Hamdhan Junit tests and add assert statements
thomasjlalba
left a comment
thomasjlalba
left a comment
There was a problem hiding this comment.
Good work thus far everyone, only have a few really minor suggestions, but I especially like the abstraction to smaller and simpler methods
Comment on lines
+45
to
+47
| } else { | ||
| return "~/" + currentTrip + "/" + currentTarget + " >"; | ||
| } |
There was a problem hiding this comment.
Maybe the else statement is redundant, given it is a return statement.
| } | ||
|
|
||
| private ArrayList<String> extractCommandArguments(String command) throws InvalidCommand { | ||
| String[] doubleHyphenSeparatedTokens = command.strip().split("(^--|\\s+--)(?=\\w+\\s+\\w+)"); |
There was a problem hiding this comment.
Maybe it would be good to refactor the regex to make it more understandable and less of a magic literal
Comment on lines
+110
to
+111
| Ui.printDeleteAccommodationMessage(accommodations.get(index - 1)); | ||
| accommodations.remove(index - 1); |
There was a problem hiding this comment.
I think it might be clearer if you indicate with a comment why it is index - 1
| } | ||
| Transportation newTransportation = new Transportation(transportName, transportMode, transportBudget); | ||
| transportations.add(newTransportation); | ||
| Ui.printAddTransportationMessage(newTransportation); |
There was a problem hiding this comment.
Good work on refactoring the magic strings!
| } | ||
| } | ||
|
|
||
| public String abbrInfo() { |
There was a problem hiding this comment.
Maybe this is a personal preference, but maybe a more meaningful method name could be used like printAbbreviatedInfo (if I understand the method's intention correctly).
| StringBuilder tripInfo = new StringBuilder(); | ||
| tripInfo.append(abbrInfo()).append("\n"); | ||
|
|
||
| tripInfo.append("Itinerary:\n"); |
There was a problem hiding this comment.
Maybe you can look to refactor this magic string
Comment on lines
+95
to
+99
| case "add", "a", "make", "mk" -> CommandAction.ADD; | ||
| case "delete", "d", "remove", "rm" -> CommandAction.DELETE_BY_INDEX; | ||
| case "list", "l" -> CommandAction.LIST; | ||
| case "cd" -> CommandAction.CHANGE_DIRECTORY; | ||
| case "exit", "quit", "bye" -> CommandAction.EXIT; |
There was a problem hiding this comment.
Maybe you can refactor the magic strings to make it clearer, esp for the short forms like l or d
…mmodation Dev/101 Modification of accommodation
Start and end day were not properly set causing program to crash. Verify if start and end day are null to avoid program crashing.
start and end day cannot be null otherwise they will throw InvalidCommand contradicting assertDoesNotThrow
# Conflicts: # src/main/java/voyatrip/VoyaTrip.java
# Conflicts: # src/main/java/voyatrip/Trip.java
Dev/91 add day to transportation
Dev/127 Add purposed storage system sequence diagram
…chiu/tp into fix/153-DevelopmentGuide
Dev/225 ug and dg update
…je-nnyt/tp into dev/242-Finalize-transportation
# Conflicts: # docs/DeveloperGuide.md
Dev/242 finalize transportation
Fix/153 Development Guide
# Conflicts: # docs/UserGuide.md
Dev/225 user guide
Fix/249 indentation
Fix invalid portfolio path
* master: (26 commits) Fix invalid portfolio path Fix: indentation Fix: table of contents not showing properly UserGuide.md: Change error message order Add clarification for table of contents Fix merge conflicts Update user guide Move PlantUML section in DG Apply format changes Update project description in PPP Apply final changes Update RepoSense link Update portfolio link Finalise DG documentation of modifyAccommodation Fix coding style Update DG Add table of cotent Update product profiles DG update Remove empty lines ...
Dev/246 add ppp for Lyam
Dev/246 Add PPP
Fix/253 Restructure the main readme
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.