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

[CS2113-W11-2] WildWatch #5

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

Conversation

woodenclock
Copy link

@woodenclock woodenclock commented Oct 4, 2023

Created Team Repo for tp.
Wildwatch is a program for recording down individual wildlife in a wildlife reserve via the Command Line Interface (CLI).

Brian030601 pushed a commit to Brian030601/tp that referenced this pull request Oct 13, 2023
Copy link

@Musfirahe0556596 Musfirahe0556596 left a comment

Choose a reason for hiding this comment

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

Good but there are a few violation in code quality to consider!

Comment on lines 22 to 30
} else if (inputBuffer.equals("help")) { //User request "help"
Ui.printHorizontalLines();
Ui.helpRequestMessagePrinter();
Ui.printHorizontalLines();
HelpCommand.printHelpMessage();
} else {
Ui.printHorizontalLines();
ErrorHandler.handleError(inputBuffer);
Ui.printHorizontalLines();

Choose a reason for hiding this comment

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

Could be further abstracted to improve code quality.

Comment on lines 122 to 123
* Returns input of the user from the console.
* @return The user's input as a trimmed string.

Choose a reason for hiding this comment

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

Empty line between description and parameter section.

System.out.println("Creating new file...\nFile created successfully.");
}

public static void printEntry(int nthEntry) {

Choose a reason for hiding this comment

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

Do remember to include JavaDoc comments, along with other methods too!

*/
public static void findEntry(String inputBuffer) {
boolean hasMatch = false;
String matchingWord = inputBuffer.substring(inputBuffer.indexOf("find") + 5).trim();

Choose a reason for hiding this comment

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

'Magic number' 5 - Could be placed as a named constant.

Comment on lines 102 to 104
* @param entry
* @return String
* @throws IOException when there is problem with formatting the task

Choose a reason for hiding this comment

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

Punctuation behind each parameter description.

/**
* Checks if file exists. Opens file if the file exists.
*
* @return true if file exists; false if not

Choose a reason for hiding this comment

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

Punctuation behind each parameter description.

For the diagram below, the command `add D/02-03-23 S/Annam Leaf Turtle N/Ariel R/Injured left flipper` has been substituted by `input`.

![](diagrams/AddSequenceDiagram.svg)

Choose a reason for hiding this comment

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

AddCommand should be destroyed after being executed and returns a command result.

@@ -1,38 +1,212 @@
# Developer Guide
# WildWatch Developer Guide 🦏

Choose a reason for hiding this comment

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

Overall good job. The DG has more to be completed so I don't have much comments to make. No issues with structure and flow. Nice that you have a table of contents and an option to take reader back to top. Cute emojis as well :). Don't forget to include class diagrams as well. I am sure you guys have plans to add in individual commands, explaining the design considerations and implementations.

Copy link

@spaceman03 spaceman03 left a comment

Choose a reason for hiding this comment

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

Great job! I believe that you all will add more UML diagrams into the DG such as class diagrams and not just sequence diagrams.


### System Architecture

![System Architecture](https://github.com/AY2324S1-CS2113T-W11-2/tp/assets/69474977/90309a3c-f784-4ffb-8eef-65735c05ec52)

Choose a reason for hiding this comment

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

image Perhaps you could remove the class names below as it is already mentioned above.

Comment on lines 47 to 58
### Legend

| Symbol | Meaning |
| -------------- |------------------------------------------------------------ |
| ❗ IMPORTANT | These are important instructions that you should follow. |
| ✏ Note | These are important details that you should take note of. |
| ⬆ Back to top | Click to scroll back up to the `Table of Contents`. |
| 🐵 🦊 🦁 | Animals indicate you have reached a new section. |

[⬆ Back to top](https://ay2324s1-cs2113t-w11-2.github.io/tp/DeveloperGuide.html#table-of-contents)

--------------------------------------------------------------------------------------------------------------------------------------

Choose a reason for hiding this comment

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

I like this session that shows the different legends used in the DG.

Comment on lines 160 to 162

[⬆ Back to top](https://ay2324s1-cs2113t-w11-2.github.io/tp/DeveloperGuide.html#table-of-contents)

Choose a reason for hiding this comment

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

image

Nice to see that you have a "back to top" button after each section.

Comment on lines 33 to 45
### How to use the Developer Guide
- Are you new here?
No worries, head to the [Quick Start](https://ay2324s1-cs2113t-w11-2.github.io/tp/DeveloperGuide.html#quick-start-) page.
- Lost among the pages?
Head to the [Table of Contents](https://ay2324s1-cs2113t-w11-2.github.io/tp/DeveloperGuide.html#table-of-contents) to look for the right pages.
- Need help with the functionalities?
Head to the [Features](https://ay2324s1-cs2113t-w11-2.github.io/tp/DeveloperGuide.html#features-) page for detailed guidance.
- Do you have a question for us?
Head to the [FAQ](https://ay2324s1-cs2113t-w11-2.github.io/tp/DeveloperGuide.html#faq-) page.
- Do you want a concise summary of all functionalities?
Head to the [Command Summary](https://ay2324s1-cs2113t-w11-2.github.io/tp/DeveloperGuide.html#command-summary-) page for a summary of all commands.
- Not sure what that word meant?
Head to the [Glossary](https://ay2324s1-cs2113t-w11-2.github.io/tp/DeveloperGuide.html#glossary-) page for its meaning.

Choose a reason for hiding this comment

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

This section is great! It navigates the user to the respective section of the DG.


{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well}
![Architecture Sequence Diagram](https://github.com/AY2324S1-CS2113T-W11-2/tp/assets/69474977/6bea5165-aa78-4b3e-baf0-2e9ced109161)
Copy link

Choose a reason for hiding this comment

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

Is there an additional return from command to execute or is command returning twice to execute?


### Command component
![](images/AddSequenceDiagram.png)

Copy link

Choose a reason for hiding this comment

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

yepp^ and what do the 'a's above return arrows represent?



### Parser component
Copy link

Choose a reason for hiding this comment

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

Would be good to provide a sequence diagram for Parser too

|v1.0|new user|see help instructions|refer to them when I forget how to use the application|
|v1.0|zoo clerk|add an animal entry|record the animals in the zoo, and refer to them afterwards|
|v1.0|zoo clerk|delete an animal entry|remove redundant or invalid animal entry|
|v1.0|zoo clerk|list all the entries|see what entries I have entered previously, and refer to them|
|v2.0|user|find a to-do item by name|locate a to-do without having to go through the entire list|
Copy link

Choose a reason for hiding this comment

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

How about other things like saving, editing, and summarising?


[⬆ Back to top](https://ay2324s1-cs2113t-w11-2.github.io/tp/DeveloperGuide.html#table-of-contents)

--------------------------------------------------------------------------------------------------------------------------------------
Copy link

Choose a reason for hiding this comment

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

Overall DG is incomplete, well formatted and easy to understand but insufficient details and diagrams

Copy link

@kaijie0102 kaijie0102 left a comment

Choose a reason for hiding this comment

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

Overall, UI is really neat and clear.

However, general sequence diagram has some common notation errors, but could be fix really easily

# Developer Guide
# WildWatch Developer Guide 🦣

## Table of Contents

Choose a reason for hiding this comment

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

I really like your table of content, it is something that I would like to have in my own DG


### System Architecture

![System Architecture](https://github.com/AY2324S1-CS2113T-W11-2/tp/assets/69474977/90309a3c-f784-4ffb-8eef-65735c05ec52)

Choose a reason for hiding this comment

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

I like that how clean and clear your architecture diagram is. Not only are the fonts just right, the choice of colours of arrows make the interaction really obvious


## Design & implementation
The **_Generic Sequence Diagram_** above shows a shows how the components in the architecture interact with each other for a generic command input in WildWatch.

Choose a reason for hiding this comment

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

image
Is activation bar too long?

See Common Notation Error in week 10 Sequence Diagram
image


## Design & implementation
The **_Generic Sequence Diagram_** above shows a shows how the components in the architecture interact with each other for a generic command input in WildWatch.

Choose a reason for hiding this comment

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

Could you be missing an entry point to the sequence diagram? It seems like the entry point comes from the Execute class
image
but in the docs its said that main was the entry point
image


## Design & implementation
The **_Generic Sequence Diagram_** above shows a shows how the components in the architecture interact with each other for a generic command input in WildWatch.
Copy link

@kaijie0102 kaijie0102 Nov 3, 2023

Choose a reason for hiding this comment

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

image
Perhaps, It might be better to seperate error handling using checkError() method from the construction of a Parser object?


## Design & implementation
The **_Generic Sequence Diagram_** above shows a shows how the components in the architecture interact with each other for a generic command input in WildWatch.

Choose a reason for hiding this comment

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

Is the activation bar broken?
image

image
See Common Notation Error in week 10 Sequence Diagram



### Parser component

Choose a reason for hiding this comment

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

perhaps "a" could be replaced with addCommand object or something that reflects "a" being a Command

image

| ⬆ Back to top | Click to scroll back up to the `Table of Contents`. |
| 🐵 🦊 🦁 | Animals indicate you have reached a new section. |

[⬆ Back to top](https://ay2324s1-cs2113t-w11-2.github.io/tp/DeveloperGuide.html#table-of-contents)

Choose a reason for hiding this comment

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

I really like your back to top buttons! Will incorporate this

Brian030601 pushed a commit to Brian030601/tp that referenced this pull request Nov 6, 2023
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.

10 participants