[CS2113-W11-3] Travel Diary#52
Open
LeThiHongMinh wants to merge 384 commits into
Open
Conversation
okkhoy
reviewed
Mar 30, 2025
okkhoy
left a comment
okkhoy
left a comment
There was a problem hiding this comment.
The code that is present here is quite good.
however,
1/ much of the codebase needs javadoc comments
2/ i didn't see too many exception handling scenarios, or assert statements (and any logging statements).
please pay attention to these as they are used for grading
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class Album { |
There was a problem hiding this comment.
non-trivial methods need javadoc comments. (applied to all such classes in this codebase, please propagate the comment)
| import trip.TripManager; | ||
| import ui.Ui; | ||
|
|
||
| // TODO: need to find out how to close photo after bye |
There was a problem hiding this comment.
remove such "note-to-self" comments.
you can use these kind of review comments for the purpose
Comment on lines
+1
to
+18
| //package command; | ||
| // | ||
| //import exception.TravelDiaryException; | ||
| //import trip.TripManager; | ||
| //import ui.Ui; | ||
| // | ||
| //public class SelectTripCommand extends Command { | ||
| // private int index; | ||
| // | ||
| // public SelectTripCommand(int index) { | ||
| // this.index = index; | ||
| // } | ||
| // | ||
| // @Override | ||
| // public void execute(TripManager tripManager, Ui ui, int fsmValue) throws TravelDiaryException { | ||
| // ui.showToUser("Alvida! Till we meet next time :)"); | ||
| // } | ||
| //} |
There was a problem hiding this comment.
don't retain commented code - in this case, you may as well remove the file totally
Comment on lines
+43
to
+46
| case "bye": | ||
| case "list": | ||
| case "close": | ||
| case "menu": |
| map.put("name", tagsMap.get("n#")); | ||
| map.put("description", tagsMap.get("d#")); | ||
| map.put("location", tagsMap.get("l#")); | ||
| if (map.get("name") == null || map.get("description") == null || map.get("location") == null) { |
There was a problem hiding this comment.
some parts of the code (like this one) can use some blank lines to give well-readable vertical spacing
Comment on lines
+125
to
+135
| if (part.startsWith(tag)) { | ||
| if (!seenTags.add(tag)) { | ||
| throw new TravelDiaryException("Duplicate tag provided for " + tag); | ||
| } | ||
| String value = part.substring(tag.length()).trim(); | ||
| if (value.isEmpty()) { | ||
| throw new TravelDiaryException("Empty value provided for tag " + tag); | ||
| } | ||
| return new AbstractMap.SimpleEntry<>(tag, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
consider extracting out some parts under a guard clause and eliminate arrow-head code
Comment on lines
+29
to
+47
| try (BufferedWriter writer = new BufferedWriter(new FileWriter(dataFile))) { | ||
| for (Trip trip : trips) { | ||
| writer.write(formatTripLine(trip)); | ||
| writer.newLine(); | ||
|
|
||
| if (trip.album != null) { | ||
| writer.write("A | " + encodeString(trip.name)); | ||
| writer.newLine(); | ||
| System.out.println("Saving album for trip: " + trip.name); // Log album saving | ||
|
|
||
| if (trip.album.photos != null) { | ||
| for (Photo photo : trip.album.photos) { | ||
| writer.write(formatPhotoLine(photo)); | ||
| writer.newLine(); | ||
| System.out.println("Saving photo: " + photo.getPhotoName()); // Log each photo saving | ||
| } | ||
| } | ||
| } | ||
| } |
Comment on lines
+177
to
+179
| decodeString(parts[1]), // file path | ||
| decodeString(parts[2]), // photo name | ||
| decodeString(parts[3]), // caption |
There was a problem hiding this comment.
why not use meaningful variable names rather than use comments to indicate what each part is?
Comment on lines
+70
to
+71
| tripsDetails.append("\t").append(i + 1).append(") ") | ||
| .append(trips.get(i).toString()); |
There was a problem hiding this comment.
would using a loop be good in this case, instead of a sequence of appends?
Comment on lines
+43
to
+92
| // @Test | ||
| // public void AddPhotoParsingTest() { | ||
| // String simulatedInput = "add_photo f#./data/photos/sample1.jpg n#Osaka photo c#Friends l#Osaka street\n"; | ||
| // System.setIn(new ByteArrayInputStream(simulatedInput.getBytes())); | ||
| // | ||
| // Map<String, String> parsedCommand = assertDoesNotThrow(Parser::getCommandDetails); | ||
| // | ||
| // // Verify the parsed result | ||
| // assertEquals("add_photo", parsedCommand.get("command")); | ||
| // assertEquals("./data/photos/sample1.jpg", parsedCommand.get("filepath")); | ||
| // assertEquals("Osaka photo", parsedCommand.get("photoname")); | ||
| // assertEquals("Friends", parsedCommand.get("caption")); | ||
| // assertEquals("Osaka street", parsedCommand.get("location")); | ||
| // } | ||
| // | ||
| // @Test | ||
| // public void SelectParsingTest() { | ||
| // String simulatedInput = "select 1"; | ||
| // System.setIn(new ByteArrayInputStream(simulatedInput.getBytes())); | ||
| // | ||
| // Map<String, String> parsedCommand = assertDoesNotThrow(Parser::getCommandDetails); | ||
| // | ||
| // // Verify the parsed result | ||
| // assertEquals("select", parsedCommand.get("command")); | ||
| // assertEquals("1", parsedCommand.get("index")); | ||
| // } | ||
| // | ||
| // @Test | ||
| // public void DeleteParsingTest() { | ||
| // String simulatedInput = "delete 10"; | ||
| // System.setIn(new ByteArrayInputStream(simulatedInput.getBytes())); | ||
| // | ||
| // Map<String, String> parsedCommand = assertDoesNotThrow(Parser::getCommandDetails); | ||
| // | ||
| // // Verify the parsed result | ||
| // assertEquals("delete", parsedCommand.get("command")); | ||
| // assertEquals("10", parsedCommand.get("index")); | ||
| // } | ||
| // | ||
| // @Test | ||
| // public void ListParsingTest() { | ||
| // String simulatedInput = "list"; | ||
| // System.setIn(new ByteArrayInputStream(simulatedInput.getBytes())); | ||
| // | ||
| // Map<String, String> parsedCommand = assertDoesNotThrow(Parser::getCommandDetails); | ||
| // | ||
| // // Verify the parsed result | ||
| // assertEquals("list", parsedCommand.get("command")); | ||
| // } | ||
| } |
Metadata extractor
…+ fix ui trip list
RESOLVED conflict for DG # Conflicts: # build.gradle
Add Location class
Ashley dg draft2
Remove trip location and added period tracker
Roycecodes
reviewed
Apr 3, 2025
Roycecodes
left a comment
Roycecodes
left a comment
There was a problem hiding this comment.
Overall nice DG
Found some UML diagram flaws
| ##### Saving Data | ||
| The saving process converts in-memory objects into a text representation and writes them to a file. The `saveTasks` method handles this process. | ||
|
|
||
|  |
There was a problem hiding this comment.
Good work clear diagram shown
I think it would be nice to have activtion boxes
:)
| ##### Saving Data | ||
| The saving process converts in-memory objects into a text representation and writes them to a file. The `saveTasks` method handles this process. | ||
|
|
||
|  |
There was a problem hiding this comment.
Nice work
think if you add colons to class name it will be good
| ##### Saving Data | ||
| The saving process converts in-memory objects into a text representation and writes them to a file. The `saveTasks` method handles this process. | ||
|
|
||
|  |
There was a problem hiding this comment.
think it will be nice to include constructor boxes as well
| These specialized exceptions provide detailed information about what went wrong and where, making debugging easier. | ||
|
|
||
| ### PhotoPrinter | ||
|  |
There was a problem hiding this comment.
I think there shouldnt be a C logo next to class name
nice diagram btw
| These specialized exceptions provide detailed information about what went wrong and where, making debugging easier. | ||
|
|
||
| ### PhotoPrinter | ||
|  |
There was a problem hiding this comment.
i think it is the standard to have + - to indicate public/private instead of boxes and squares
nice work.
update aboutus
Add colon to sequence diagram
Uml format bryan
Add coments
make changes to ppp - remove community involvement
Guide update
Uml format bryan
Finished PPP
UML update
Final PPP.
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
10 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.
Travel Diary is a desktop app for managing trips and travel memories, for use via a Command Line Interface (CLI). Travel Diary helps you log and organize trips, photos, and experiences more efficiently than traditional travel journaling apps.