-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CS2113-W13-3] EventManagerCLI #7
base: master
Are you sure you want to change the base?
[CS2113-W13-3] EventManagerCLI #7
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job overall! The code is solid, and the functionality is well-implemented. I only had a few minor recommendations, mainly around 'code readability' and 'SRP' , but nothing that detracts from the quality of the work. Thanks for the effort and attention to detail here!
private Optional<Participant> getParticipantByName(String participantName) { | ||
for (Participant participant : this.participantList) { | ||
if (participant.getName().equalsIgnoreCase(participantName)) { | ||
return Optional.of(participant); | ||
} | ||
} | ||
|
||
return Optional.empty(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider moving the Private
methods together at the end of the class and group them together and not have them in between Public
methods
/** | ||
* Removes a participant from the participant list. | ||
* | ||
* <p> | ||
* This method attempts to remove the specified participant from the list of | ||
* participants associated with the event. It returns {@code true} if the | ||
* participant was successfully removed, and {@code false} if the participant | ||
* was not found in the list. | ||
* </p> | ||
* | ||
* @param participantName the name of the participant to be removed from the list. | ||
* @return {@code true} if the participant was successfully removed; | ||
* {@code false} if the participant was not found in the list. | ||
*/ | ||
public boolean removeParticipant(String participantName) { | ||
return this.participantList.removeIf((participant) -> | ||
(participant.getName().equalsIgnoreCase(participantName))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider simplifying the documentation to be more concise while maintaining clarity. As of now Javadoc is longer than the actual method implementation. Since the method name is self explanatory and the @param
and @return
on the Javadoc explain the things clearly, make the description very concise, and not as long as the method
/** | ||
* Returns true if the participant with the given name can be marked present or absent. | ||
* Returns false otherwise. | ||
* | ||
* @param participantName the name of the participant. | ||
* @param isPresent true if the participant is to be marked present, false if he is to be marked absent. | ||
* @return {@code true} if the participant with participantName has been marked present or absent, | ||
* {@code false} otherwise. | ||
*/ | ||
public boolean markParticipant(String participantName, boolean isPresent) { | ||
Optional<Participant> participant = getParticipantByName(participantName); | ||
|
||
if (participant.isEmpty()) { | ||
return false; | ||
} | ||
|
||
participant.get().setPresent(isPresent); | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using SLAP, and limit one method to only one function. As of now, function contains code for : both participant lookup and status modification
Consider splitting into two methods for better single responsibility.
public ArrayList<Participant> getParticipantList() { | ||
return participantList; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding Javadocs to standardize how the document handles getter code.
//@@author glenn-chew | ||
/** | ||
* Parses the input string to create a {@link Command} based on the provided command parts. | ||
* | ||
* <p> | ||
* This method checks the command flag extracted from the command parts. If the command | ||
* flag is {@code "-e"}, it splits the input string to create a {@link ViewCommand} | ||
* for viewing the participants in the event. | ||
* Otherwise, it throws an {@link InvalidCommandException} with an error message. | ||
* </p> | ||
* | ||
* @param input the input string containing the command details. | ||
* @param commandParts an array of strings representing the parsed command parts, | ||
* where the second element is the command flag. | ||
* @return a {@link Command} object representing the parsed command. | ||
* @throws InvalidCommandException if the flag is not matched. | ||
*/ | ||
private Command parseViewCommand(String input, String[] commandParts) throws InvalidCommandException { | ||
assert commandParts[0].equalsIgnoreCase(ViewCommand.COMMAND_WORD); | ||
try { | ||
String commandFlag = commandParts[1]; | ||
|
||
if (commandFlag.equals("-e")) { | ||
String [] inputParts = input.split("-e"); | ||
return new ViewCommand(inputParts[1].trim()); | ||
} | ||
|
||
logger.log(WARNING,"Invalid command format"); | ||
throw new InvalidCommandException(INVALID_VIEW_MESSAGE); | ||
} catch (IndexOutOfBoundsException exception) { | ||
logger.log(WARNING,"Invalid command format"); | ||
throw new InvalidCommandException(INVALID_VIEW_MESSAGE); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider reducing the Javadocs description. The @params
and @return
make the code's functionality understandable and does not require a huge description. Rephrase the description to make it clearer and try to not make it as long as the method itself.
} catch (IndexOutOfBoundsException exception) { | ||
logger.log(WARNING,"Invalid command format"); | ||
throw new InvalidCommandException(INVALID_REMOVE_MESSAGE); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's duplicate error handling code across parse methods. Consider extracting the common error handling pattern and making that a method
String[] inputParts = input.split("-e|-s"); | ||
return getMarkEventCommand(inputParts[1].trim(), inputParts[2].trim()); | ||
} else if (commandFlag.equalsIgnoreCase("-p")) { | ||
String[] inputParts = input.split("-p|-e|-s"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting command flags into constants. That is, storing -p
, -e
, -s
as constants.
public void saveEvents(EventList events) throws IOException { | ||
try (FileWriter writer = new FileWriter(filePath)) { | ||
for (Event event : events.getList()) { | ||
writer.write(event.getEventName() + "," + event.getEventTime() + "," | ||
+ event.getEventVenue() + "\n"); // Save event details in CSV format | ||
} | ||
} catch (IOException exception) { | ||
throw new IOException("Error saving events to file: " + filePath); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider saving Events with a different De-limiter and not comma ','. As if any name or event happens to have a dummy comma inside, it could break the code.
} | ||
} | ||
} catch (IOException exception) { | ||
throw new IOException("Error loading events from file: " + filePath + "."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding what the exception which has occurred is
public void addParticipant(String participantName) { | ||
Participant participant = new Participant(participantName); | ||
this.participantList.add(participant); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding code to verify if the participant is already in the list
docs/DeveloperGuide.md
Outdated
2. The user enters the command `mark -e Event 1 -s done` to mark `Event 1` as done. `MarkEventCommand` calls `MarkEventCommand#execute`, | ||
in which it gets the event `Event 1` from the event list, and sets its mark status to `true` or done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it would be helpful to use a state or class diagram to illustrate the program state here?
docs/DeveloperGuide.md
Outdated
2. Command Recognition: | ||
The `COMMAND_WORD` is set to "list", enabling the system to recognize the command input and invoke `ListCommand`. | ||
|
||
3. Execution of ListCommand#execute(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should method calls be formatted as a code block (i.e. enclose with ``) as well?
It uses the `String.format` method with `LIST_MESSAGE` to include the total number of events in the message header. | ||
Events are appended to `outputMessage` with numbered formatting for readability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be generalized as pseudocode? I.e. Does the developer need to know exactly how the message is created to understand how the feature wroks?
docs/DeveloperGuide.md
Outdated
|
||
Given below is an example usage scenario for the `add` mechanism, and how it behaves at each step. | ||
|
||
1. The user enters the command `add` followed by `-e` or `-p` to indicate adding an event or participant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the developer need to know exactly what flags are being used to execute this command? Can it be generalized/explained in pseudo terms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall Great Job but certain things are misrepresented in the code. Just needs minor changes. Great Job overall.
docs/DeveloperGuide.md
Outdated
|
||
## Non-Functional Requirements | ||
|
||
{Give non-functional requirements} | ||
* Should work for any **mainstream OS** as long as Java 17 is installed. | ||
|
||
## Glossary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fill out the Glossary Items and Include the functions required for manual testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Event should have a constructor and multiple Objects such as UI, Parser and Event need to be shown as objects. eg. ui:UI
Similar issues with most of the other diagrams. Otherwise very clear and helps with understanding!
docs/DeveloperGuide.md
Outdated
| v2.0 | user | mark events as completed | easily track all past events | | ||
| v2.0 | user | mark participants present | know exactly who signed up but did not attend the event | | ||
| v2.0 | user | save events info | can still access the information if the program terminates | | ||
| v2.0 | user | filter events by keywords | can find relevant information efficiently | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filter and Find Commands seem to be implemented yet not mentioned here. It would be best to at least make a small mention of it above with a general description of how it works
2. The user enters the command `mark -e Event 1 -s done` to mark `Event 1` as done. `MarkEventCommand` calls `MarkEventCommand#execute`, | ||
in which it gets the event `Event 1` from the event list, and sets its mark status to `true` or done. | ||
|
||
3. The user then enters the command `mark -e Event 1 -s undone` to mark `Event 1` as not done. The `MarkEventCommand` again calls `MarkEventCommand#execute`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flag format is inconsistent. All of the Sequence Diagrams have the flags as e/ or p/ instead of -e or -p as stated in the text right above them. All of the above ones could use examples of the full format of the command like it is done for mark/unmark section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, more work needs to be done to better align the diagram to textbook expectations. Other more pressing concerns include description of the Participant
class and consistency of usage flags/options. Do also consider the relevance and positon of the Storage
class if it is being used by the various commands.
docs/DeveloperGuide.md
Outdated
<img src = "images/ArchitectureSequenceDiagram.png"> | ||
|
||
The above **Sequence Diagram** shows how the different components of the system interact with one | ||
another in the scenario when the command `add -e event -t 1200 -v venue` is executed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For your sequence diagram here, to better align yourselves with the notation suggested in the textbook, might it be better to add colons to indicate that these are actual instances? For example, :Parser
. From the textbook as well, I noticed that there was no need to specify the return type of the object that is being returned from a method.
Might you consider adding a User
participant that interacts with Ui
instead of Main
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you can add colons to indicate that these are actual instances/objects instead of classes. This comment applies to all other sequence diagrams throughout the DG too.
|
||
### Storage component | ||
|
||
<img src = "images/StorageClassDiagram.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might you consider adding the solid black triangles to indicate the directionality of the labels? While I am able to gather intuitively the directionality of most of the labels, I am unsure about that of the adopts
label.
|
||
The `Command` component and its component classes are shown in the below **Class Diagram**: | ||
|
||
<img src = "images/CommandClassDiagram.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In describing the abstract Command
class, might it be relevant to include at least the abstract method that all children Command
classes are expected to implement?
docs/DeveloperGuide.md
Outdated
The interactions between `Command` and other commands in the system is shown in the following _Sequence Diagram_: | ||
|
||
<img src = "images/CommandSequenceDiagram.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diagram only describes the interaction between a single XYZCommand
and the other components of the system, yet the text description above does not match the diagram as it says that it is showing interactions between various commands. Consider proofreading content before updating?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this, was an oversight on our part (it was meant to be "components" rather than "commands")
docs/DeveloperGuide.md
Outdated
|
||
The `Event` component and its component classes are shown in the below **Class Diagram**: | ||
|
||
<img src = "images/EventClassDiagram.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The introduction of Participant
seems to be rather abrupt. Is Participant
an actual class/object or is it referring to the user? The relationship here isn't obvious and well-documented and you might consider adding more content to clarify this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Participant
here is an actual class/object. Thanks for clarifying this, we'll add more details on the implementation of the Event component.
Also, we initially believed that it was clear from the class diagram that the Event
class was composed of multiple Participant
objects (with the multiplicity and association labels), and it would have been too much detail to describe it further.
docs/DeveloperGuide.md
Outdated
**Add Participant** | ||
|
||
<img src = "images/AddParticipantSequenceDiagram.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comments regarding the AddEvent
sequence diagram.
docs/DeveloperGuide.md
Outdated
Given below is an example usage scenario for the `remove` mechanism, and how it behaves at each step. | ||
|
||
1. The user enters the command `remove` followed by `-e` or `-p` to specify removing an event or participant. | ||
2. This step is determined by our `Parser` which parses through the user input to determine if it is adding a participant or event | ||
3. Based on the parsed input, `RemoveComamnd` executes one of the following actions: | ||
+ **Remove Event:** Remove the specified event from `EventList` using the provided event name | ||
+ **Remove Participant:** Locates the event in `EventList` and deletes the specified participant | ||
4. If the event or participant is not found, `RemoveCommand` sets a failure message. | ||
|
||
The interactions between components during the execution of the `remove` command are show in the **Sequence Diagram** below: | ||
|
||
**Remove Event** | ||
|
||
<img src = "images/RemoveEventSequenceDiagram.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conflict between the flags employed in the text description and diagram themselves. See above comment regarding the AddEvent
sequence diagram.
docs/DeveloperGuide.md
Outdated
|
||
**Remove Participant** | ||
|
||
<img src = "images/RemoveParticipantSequenceDiagram.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment for AddEvent
sequence diagram.
docs/DeveloperGuide.md
Outdated
|
||
The interactions between components during the execution of the `view` command are show in the **Sequence Diagram** below: | ||
|
||
<img src = "images/ViewEventSequenceDiagram.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to previous comments regarding Participant
, if you have another relevant class like ParticipantList
, it might be worth noting down its details in the DG?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding Participant
, see the above comment on the Event
component.
|
||
The interactions between components during the execution of the `mark` command are show in the **Sequence Diagram** below: | ||
|
||
<img src = "images/MarkEventSequenceDiagram.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! The sequence diagram here appears to be aligned with textbook requirements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, some changes are required to follow textbook conventions and coding standards
|
||
### Interactions between components | ||
|
||
<img src = "images/ArchitectureSequenceDiagram.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the textbook, the object deletion notation for Parser object to indicate the point at which it becomes ready to be garbage-collected should be marked with a cross with no lifeline after that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lifeline after that is an issue with PlantUML. We will note that below the diagram.
docs/DeveloperGuide.md
Outdated
<img src = "images/ArchitectureSequenceDiagram.png"> | ||
|
||
The above **Sequence Diagram** shows how the different components of the system interact with one | ||
another in the scenario when the command `add -e event -t 1200 -v venue` is executed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you can add colons to indicate that these are actual instances/objects instead of classes. This comment applies to all other sequence diagrams throughout the DG too.
docs/DeveloperGuide.md
Outdated
* Load events and participants information from a text file and save it to `EventList` list. | ||
* Save events from `EventList` list to a text file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps can consider adding a sequence diagram to illustrate the method calls here (similar to how you created a sequence diagram for the command component)
Same goes to the event component
docs/DeveloperGuide.md
Outdated
|
||
This section describes some noteworthy details on how certain features are implemented. | ||
|
||
### List feature[TBD] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps can write [Proposed] for proposed implementation of list feature instead of using abbreviations such as TBD which may not be easily understood by everyone
docs/DeveloperGuide.md
Outdated
**Add Event** | ||
|
||
<img src = "images/AddEventSequenceDiagram.png"> | ||
|
||
**Add Participant** | ||
|
||
<img src = "images/AddParticipantSequenceDiagram.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We believe it's more of an issue of the sequence diagram being too large and complicated. We're planning to redo the diagram (other comments pointed out more pressing issues) so we'll take your point into account.
|
||
The interactions between components during the execution of the `mark` command are show in the **Sequence Diagram** below: | ||
|
||
<img src = "images/MarkEventSequenceDiagram.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the boolean variable toMark can be renamed to sound like booleans (such as isDone) so that there is no coding standard violations
docs/DeveloperGuide.md
Outdated
@@ -4,30 +4,274 @@ | |||
|
|||
{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps can consider removing the acknowledgements section if you don't have sources to acknowledge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, thanks for pointing this out!
Update code flow to be consistent throughout
Add Edit feature to UG
Add Edit feature to DG
Update FilterCommand , UG and DG
Update output string method
Update DG manual testing section to reflect actual functionality
Add project management items to PPP
Add page break before "Features" section of UG
Convert command list in UG to screenshot
Resolve storage bugs
Only use english keyboard
Update regex
Update Storage
Update storage loading sequence diagram
Update Storage PUML
EventManagerCLI allows tutors/event managers to organize events and classes and keep track of participants progress. It is optimized for CLI users so that frequent tasks can be done more efficiently.