[CS2113-W14-4] Internity#8
Conversation
NigelYeoTW
left a comment
NigelYeoTW
left a comment
There was a problem hiding this comment.
Overall lgtm, just some minor issues with magic numbers, etc, keep it up! 🔥
| try { | ||
| String[] parts = args.split("\\s+(?=company/|role/|deadline/|pay/)"); | ||
|
|
||
| if (parts.length != 4 || |
There was a problem hiding this comment.
Consider extracting out long logic statements for readability
|
Is there missing activation bars here? |
|
If it returns a new delete command, shouldn't it point to the side of the box? (Refer to week 10.1 CS2113 ) |
|
If it returns a new list command, shouldn't it point to the box?(refer to w10.1 cs2113) |
|
If it's a new findcommand(), should it be a constructor instead of activation of an already instantialized object? |
|
For AddCommand and exit command, if it returns a new object, shouldnt it have a constructor for its sequence diagram instead of calling an activation bar ? |
sanjay-shiva-kumar
left a comment
sanjay-shiva-kumar
left a comment
There was a problem hiding this comment.
Overall, this is a very thorough and polished developer guide. There are just a few details that are not working/incomplete, and some of the diagrams can be simplified.
|
|
||
| ### Delete feature | ||
|
|
||
| **API**: [`DeleteCommand.java`](https://github.com/AY2526S1-CS2113-W14-4/tp/blob/master/src/main/java/internity/commands/DeleteCommand.java) |
There was a problem hiding this comment.
This link for DeleteCommand is broken.
|
|
||
| ### Storage Component | ||
|
|
||
| **API**: [`Storage.java`](https://github.com/AY2526S1-CS2113-W14-4/tp/blob/master/src/main/java/internity/core/Storage.java) |
There was a problem hiding this comment.
The link for Storage is broken.
|
|
||
| ## Instructions for manual testing | ||
|
|
||
| {Give instructions on how to do a manual product testing e.g., how to load sample data to be used for testing} |
There was a problem hiding this comment.
Should this line be deleted?
| ### Adding an internship | ||
|
|
||
| ### Updating an internship | ||
|
|
||
| ### Deleting an internship | ||
|
|
||
| ### Listing and sorting all internships |
There was a problem hiding this comment.
Will these sections be written in the future?
| * **Alternative 1 (current choice):** Pipe-delimited text format. | ||
| * Pros: Human-readable, easy to debug and manually edit if needed. | ||
| * Pros: Simple parsing logic without external dependencies. | ||
| * Pros: Works across all platforms without binary compatibility issues. | ||
| * Cons: Slightly larger file size compared to binary formats. | ||
| * Cons: No built-in schema validation. | ||
| * Cons: Performance degrades with very large files (not an issue for target use case of ~1000 entries). | ||
|
|
||
| * **Alternative 2:** JSON format using a library. | ||
| * Pros: Structured format with built-in validation. | ||
| * Pros: Easier to extend with new fields. | ||
| * Pros: Better for complex nested data structures. | ||
| * Cons: Less human-readable due to verbose syntax. | ||
| * Cons: Overkill for simple tabular data. | ||
|
|
||
| * **Alternative 3:** Binary serialization using Java's `ObjectOutputStream`. | ||
| * Pros: Compact file size. | ||
| * Pros: Fast serialization/deserialization. | ||
| * Cons: Not human-readable or editable. | ||
| * Cons: Platform-dependent binary format could cause issues. | ||
|
|
||
| **Aspect: Error handling strategy** | ||
|
|
||
| * **Alternative 1 (current choice):** Skip corrupted lines with warnings, continue loading valid entries. | ||
| * Pros: Maximizes data recovery - users don't lose all data due to one corrupted line. | ||
| * Pros: Application remains usable even with partial data corruption. | ||
| * Pros: Warnings inform users of data issues without blocking usage. | ||
| * Cons: Silent data loss is possible if users don't notice warnings. | ||
| * Cons: Corrupted data is permanently lost on next save. | ||
| * Cons: Code complexity increases as edge cases need to be accounted for. | ||
|
|
||
| * **Alternative 2:** Fail fast - throw exception and abort load on any corrupted line. | ||
| * Pros: No silent data loss - users are immediately aware of corruption. | ||
| * Pros: Forces users to fix or remove corrupted data. | ||
| * Cons: Users cannot access any data if even one line is corrupted. | ||
| * Cons: Poor user experience for a minor data issue. | ||
|
|
||
| * **Alternative 3:** Load all lines (including corrupted ones) into a separate "invalid entries" list for user review. | ||
| * Pros: No data loss - even corrupted entries are preserved. | ||
| * Pros: Users can manually review and fix corrupted entries. | ||
| * Cons: Significantly more complex implementation. | ||
| * Cons: Requires additional UI for managing invalid entries. | ||
| * Cons: Invalid entries could accumulate over time. | ||
|
|
||
| **Aspect: When to save** | ||
|
|
||
| * **Alternative 1 (current choice):** Auto-save after every command that modifies data. | ||
| * Pros: Minimizes data loss risk - data is never more than one command out of sync. | ||
| * Pros: Simple mental model for users - changes are always saved. | ||
| * Pros: No need for users to remember to save manually. | ||
| * Cons: Higher I/O overhead (mitigated by small file sizes). | ||
| * Cons: Performance impact if storage is slow (e.g., network drive). | ||
|
|
||
| * **Alternative 2:** Save only on explicit "save" command or application exit. | ||
| * Pros: Better performance with fewer write operations. | ||
| * Pros: Users have control over when data is persisted. | ||
| * Cons: Risk of data loss if application crashes or user forgets to save. | ||
| * Cons: Inconsistent with modern application expectations. | ||
| * Cons: More complex implementation (need to track dirty state). | ||
|
|
||
| * **Alternative 3:** Periodic auto-save every N seconds or N operations. | ||
| * Pros: Balances performance and data safety. | ||
| * Pros: Reduces risk of data loss compared to manual save. | ||
| * Cons: More complex implementation (requires background thread or operation counter). | ||
| * Cons: Users could still lose up to N operations of data. | ||
| * Cons: Unpredictable save timing could confuse users. | ||
|
|
||
| **Aspect: File location and structure** | ||
|
|
||
| * **Alternative 1 (current choice):** Single file at `./data/internships.txt` with both username and internships. | ||
| * Pros: Simple file structure, easy to locate and backup. | ||
| * Pros: All data in one place for easy migration. | ||
| * Pros: Atomic writes ensure consistency (file is completely written or not at all). | ||
| * Cons: Single point of failure - if file is corrupted, all data is affected. | ||
|
|
||
| * **Alternative 2:** Separate files for username and internships. | ||
| * Pros: Better separation of concerns. | ||
| * Pros: Reduces risk of username corruption affecting internship data. | ||
| * Cons: More complex file management. | ||
| * Cons: Two files must be kept in sync. | ||
|
|
||
| * **Alternative 3:** User-home directory with platform-specific application data folder. | ||
| * Pros: Follows OS conventions for application data storage. | ||
| * Pros: Better for multi-user systems. | ||
| * Cons: Harder for users to locate and backup data. | ||
| * Cons: More complex path resolution logic. |
There was a problem hiding this comment.
Would this section benefit from being more concise/less comprehensive?
|
|
||
| The following sequence diagram illustrates the load operation: | ||
|
|
||
|  |
There was a problem hiding this comment.
Is this sequence diagram more comprehensive than comprehensible?
|
|
||
| The following class diagram shows the Storage component and its relationships: | ||
|
|
||
|  |
There was a problem hiding this comment.
Can this class diagram be made more readable?
|
|
||
| ### Add feature | ||
|
|
||
|  |
There was a problem hiding this comment.
One of the activation bars in this diagram is too long
| The list mechanism is implemented by the `ListCommand` class, which allows users to view all internships in their list. | ||
|
|
||
| Below is the sequence diagram for a common usage of the list feature: | ||
|  |
There was a problem hiding this comment.
Is it intentional for some of the participants (e.g. InternshipList) to call themselves even though they are not currently executing?
There was a problem hiding this comment.
The activation bars also have to be consistent (if you show an activation bar, you must show it in all cases).
There was a problem hiding this comment.
The activation bar also extends too far past both the top and bottom arrow
darshhs
left a comment
darshhs
left a comment
There was a problem hiding this comment.
Overall good job! It's very comprehensive and detailed. Most of the images were useful in understanding how the application works
|
|
||
| The following class diagram shows the Storage component and its relationships: | ||
|
|
||
|  |
There was a problem hiding this comment.
I think this can be simplified to only show how the components interact with each other.
| The list mechanism is implemented by the `ListCommand` class, which allows users to view all internships in their list. | ||
|
|
||
| Below is the sequence diagram for a common usage of the list feature: | ||
|  |
There was a problem hiding this comment.
Same question as the above commenter, I don't understand how InternshipList and ListCommand are related, prehaps you can give a description of how the important methods work?
|
|
||
| **Step 2.** `InternshipList.loadFromStorage()` calls `Storage.load()`, which returns an `ArrayList<Internship>`. | ||
|
|
||
| **Step 3.** Inside `Storage.load()`: |
There was a problem hiding this comment.
The description for step 3 could be more concise, only explain the important aspects.
|
|
||
| **Aspect: Index base convention** | ||
|
|
||
| * **Alternative 1 (current choice):** Users provide 1-based indices, converted to 0-based internally in `ArgumentParser`. |
There was a problem hiding this comment.
This section could have a separate section? like all possible alternatives for each feature
Pavithra6-Srinivasan
left a comment
Pavithra6-Srinivasan
left a comment
There was a problem hiding this comment.
Maybe you can consider removing some parts that have too many details and include only the important points. Other than that good job!
| \ | ||
|
|
||
| The diagram below shows a simplified **Class Diagram** of all of Internity's classes and their relationships. | ||
|  |
There was a problem hiding this comment.
Do you think the icons beside each class should be there?
| Below is the sequence diagram for a common usage of the list feature: | ||
|  | ||
|
|
||
| #### Design considerations |
There was a problem hiding this comment.
This section is abit confusing, maybe you can split it up and add them under their respective features instead?
| Given below is an example usage scenario and how the delete mechanism behaves at each step. | ||
|
|
||
| **Step 1.** The user launches the application and the `InternshipList` contains 3 internships. The user executes `delete 2` to delete the 2nd internship in the list. | ||
|
|
||
| **Step 2.** The `CommandParser` validates the input and splits it into command word `"delete"` and arguments `"2"`. | ||
|
|
||
| **Step 3.** The `CommandFactory` delegates to `ArgumentParser.parseDeleteCommandArgs("2")`, which: | ||
| * Parses `"2"` as an integer | ||
| * Converts the 1-based index (2) to 0-based index (1) | ||
| * Creates and returns a new `DeleteCommand(1)` | ||
|
|
||
| **Step 4.** `InternityManager` calls `DeleteCommand.execute()`, which: | ||
| * Calls `InternshipList.get(1)` to retrieve the internship details (for displaying to the user) | ||
| * Calls `InternshipList.delete(1)` to remove the internship from the list | ||
| * This method validates that the index is within bounds before removal | ||
| * Calls `InternshipList.size()` to get the updated list size | ||
| * Calls `Ui.printRemoveInternship()` to display a confirmation message | ||
|
|
||
| **Step 5.** After the command completes, `InternityManager` automatically calls `InternshipList.saveToStorage()`, which in turn calls `Storage.save()` to persist the changes to disk. |
There was a problem hiding this comment.
Can the example be shorter, like just describe briefly what component does what
|
|
||
| The following sequence diagram illustrates the complete delete operation flow: | ||
|
|
||
|  |
There was a problem hiding this comment.
can the diagram be more specific about the "delete" part and abstract out all the logic before (like the "add" diagram above)
|
|
||
| ### Find feature | ||
|
|
||
|  |
There was a problem hiding this comment.
Can the steps of the feature be listed out briefly?
muadzyamani
left a comment
muadzyamani
left a comment
There was a problem hiding this comment.
Overall good job on the Developer Guide. Just a few polishing steps needed in certain areas and resolving a few critical inconsistencies.
| - Uses the `ArgumentParser` to interpret argument strings. | ||
| - Returns a fully constructed `Command` object. | ||
| 4. The `Command` object executes its logic (e.g. adds a new internship to `InternshipList`). | ||
| 5. Finally, the result of the execution is printed ot the console via the `Ui`. |
| ``` | ||
| 2. The `UsernameCommand` constructor validates that the argument is non-null and non-blank. | ||
| 3. When `execute()` is called: | ||
| - The provided username is stored via `InternshipList.setUSername(username)`. |
There was a problem hiding this comment.
Should this method be .setUsername(username)?
|
|
||
| --- | ||
|
|
||
| ### Find feature |
There was a problem hiding this comment.
This section seems more detailed than other sections which is alright but slightly inconsistent with other sections like 'Delete feature' which is focused more on the flow. Consider shortening to match the style of other sections.
|  | ||
|
|
||
| The class diagram illustrates: | ||
| * **Storage** manages file I/O operations and uses helper methods for parsing and formatting |
There was a problem hiding this comment.
Perhaps you could be more consistent here by using backticks (...) to format class names, method names and commands instead of bolding.
fix UI bugs
add JavaDoc to some methods
Update UML for storage
update Vito's PPP
Edit Benjamin's PPP
Final v2.1 touchup
Update documentation - Ben's PPP, UG, DG
Edit username validation
fix UG spacing and add dashboard
update UG to fix weird bug again
final fix pls
Removed duplicate description of the Nearest Deadline feature and added an example.
Refactor Nearest Deadline description in UserGuide
Last min fix
13 participants
Internity is a CLI based app for managing internship applications. Internity can help you manage and keep track of your internship applications more efficiently, perfect for Computer Science students who need to manage hundreds of applications.