Skip to content
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

[CS2113T-F14-1] CCA Manager #64

Open
wants to merge 581 commits into
base: master
Choose a base branch
from

Conversation

JohnNub
Copy link

@JohnNub JohnNub commented Oct 2, 2020

CCA Manager
CCA Manager is a revolutionary tool that changes the way you can manage interest groups with unrivaled efficiency and simplicity. Its lightweight Command Line Interface (CLI) allows administrators to breeze through tasks quickly and easily while offering powerful features to advanced users.

Copy link

@thngyuxuan thngyuxuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this DG was well written, with simple diagrams that are easy to understand. Although the explanations are a bit lengthy, it was broken down into steps that made it easy to understand as well. Also, there are some minor formatting issues, such as section 3.5.4:
image


{Give steps to get started quickly}
### 2. About this User Guide

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if the problem is on my computer, but on GitHub pages, the 'Setting Up' section comprises of only one sentence, 'Refer to the guide here.' with no hyperlink or any other prompts when I mouse over the words.


![CommandEventDelete](EventDiagram/SequenceDiagram/CommandEventDelete.png)

**3.5.2. Listing Events**

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please correct your numbering when revising the Developer Guide


The sequence diagram for searching for an event is as shown below:

![](EventDiagram/SequenceDiagram/CommandSearchEvent.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the arrow to the start of the activation bar?

image


The sequence diagram for deleting a particular event or all events is as shown below:

![CommandEventDelete](EventDiagram/SequenceDiagram/CommandEventDelete.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the condition to avoid it overlapping with the activation bar?

image


The sequence diagram for listing events is as shown below:

![](EventDiagram/SequenceDiagram/CommandEventList.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the dotted line to the middle of the activation bar?

image


The sequence diagram for listing events is as shown below:

![](EventDiagram/SequenceDiagram/CommandEventList.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can you format the conditions to avoid overlapping with the activation bar?


The sequence diagram for deleting a particular event or all events is as shown below:

![CommandEventDelete](EventDiagram/SequenceDiagram/CommandEventDelete.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you deleting the Event object when calling the clear() method?


The sequence diagram for deleting a particular event or all events is as shown below:

![CommandEventDelete](EventDiagram/SequenceDiagram/CommandEventDelete.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the activation bar too long in this case?

Copy link

@kaiwen98 kaiwen98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, it is a detailed and well-substantiated Developer's Guide. Some formatting errors are indeed present, but I did get a good idea on the architecture and design decisions behind the application. Good Job!

@@ -0,0 +1,21 @@
@startuml

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Not too sure if a class diagram is the correct format for the architecture of the application. I would recommend making of the diagram using Powerpoint!

The use of arrows is rather unclear here as to its purpose; I am not sure what the arrowhead means. Perhaps some annotation towards the arrow can be helpful in explaining the diagram better!


The rest of the app consists of the below:

* [**`UI`**] : The UI of the App.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a newcomer to the application, I may not be familiar with the different components of the architecture. Perhaps a more detailed description with regards to each component can be helpful in explaining more clearly the function of each particular component!

For instance,

  • [UI] : The UI of the App. Handles all console outputs onto the CLI interface.
  • [Logic] : The command executor.


The sequence diagram for listing events is as shown below:

![](EventDiagram/SequenceDiagram/CommandEventList.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Pertaining to eventList, the activation bar should start at the point of printEventList(), since class is active when the method is called. Similarly, the activation bar for the eventList should end at the return call, not extend beyond along the lifeline.

For the condition of the loop, I would choose a better phrasing, perhaps: "For each item in the arrayList".


The sequence diagram for searching for an event is as shown below:

![](EventDiagram/SequenceDiagram/CommandSearchEvent.png)
Copy link

@kaiwen98 kaiwen98 Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Pertaining to the condition "checkIfEventNameMatch()|checkIfEventDateMatch()", I would find it slightly confusing both in terms of format and the equivalent meaning conveyed. I would recommend implementing a separate method to handle checking of event altogether, which can then replace the expression above. Otherwise, I would recommend adding comments to further explain the expression.

yeyutong811 and others added 30 commits November 9, 2020 21:58
Correct UserGuide Formatting
Updated UG to be more user centric
Yutong UG diagram with red boxes and arrows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants