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-T14-1] SmartHomeBot #40

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

Conversation

zongxian-ctrl
Copy link

SmartHomeBot helps disabled working adults to consolidate all of the home appliance’s control into a centralised system. Users can also review and monitor electricity usage.

yeapcl added a commit to yeapcl/tp that referenced this pull request Oct 13, 2020
Copy link

@kiathwe97 kiathwe97 left a comment

Choose a reason for hiding this comment

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

In general, insufficient sample input/outputs, but the rest of the DG is rather well done! Really neat overall presentation and appropriate levels of design details.


public class Main {

private TextUi ui = new TextUi();

Choose a reason for hiding this comment

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

In your class diagram, there is no need to put ui as an attribute and also use a line to denote composition. Just the line would do
Uploading Screenshot 2020-10-29 at 16.13.30.png…

@@ -0,0 +1,241 @@
package seedu.smarthomebot.logic.parser;

Choose a reason for hiding this comment

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

DG mentions class diagram. Should be Sequence Diagram instead?
Uploading Screenshot 2020-10-29 at 16.15.37.png…

Copy link

@ychong032 ychong032 left a comment

Choose a reason for hiding this comment

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

An area to consider for improvement is the consistency of the visuals used. For example, some images have colours while others do not. Some images are also a bit repetitive. On the other hand, the developer guide is ultimately very comprehensive and detailed.


The class diagram of `Data Component` is shown below:

![Data Component](images/diagrams/ClassDiagram_DataOverview.png) <br>

Choose a reason for hiding this comment

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

Should the arrowheads of the arrows pointing from the various appliance classes (Light, AirConditioner etc) to the Appliance class be unfilled (i.e. filled with white)?


The sequence diagram for `OnCommand` is shown below:

![Sequence of On Command](images/diagrams/Sequence_OnCommand.png) <br>

Choose a reason for hiding this comment

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

Should the sequence diagrams be separated into different images for easier viewing? Would it be better to use colours?


The class diagram of `Data Component` is shown below:

![Data Component](images/diagrams/ClassDiagram_DataOverview.png) <br>

Choose a reason for hiding this comment

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

In the composition relationships, is the multiplicity for the "whole" object necessary?

The rest of the App consists of four components.
* `Ui`: The user interface where user can enter instructions and view output.
* `Logic` The command executor which consists of,
* `Paser`: Extract the keyword from user input

Choose a reason for hiding this comment

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

Is this a typo?

Comment on lines 217 to 250
#### Sequence Diagram for `on`

![Parser Model Component](images/diagrams/Sequence_Parser_On.png)

#### Sequence Diagram for `off`

![Parser Model Component](images/diagrams/Sequence_Parser_Off.png)

#### Sequence Diagram for `list`

![Parser Model Component](images/diagrams/Sequence_Parser_List.png)<br><br>
When the user enters the `list` command, the
`prepareListCommand(arguments)` is called. It will check if the argument contains “appliance’ or ‘location”.
1. If the arguments contains “location”, a new `ListCommand(LOCATION_TYPE, ““)` will be returned.

2. If the arguments contains “appliance”, it will check if it contains a “l/” parameter. If it exist, it
means there is a filteredLocation, thus a new `ListCommand(APPLIANCE_TYPE, filteredLocation)` will be returned.
Else, a new `ListCommand(APPLIANCE_TYPE, ““)` will be returned.

3. Any argument that does not contains “location” and “appliance” or contains “appliance” with a wrong
format will return a `InvalidCommand` class with their respective error messages.

#### Sequence Diagram for `commandword`

![Parser Model Component](images/diagrams/Sequence_Parser_Commandword.png)

`commandword` refers to the following commands `help`, `usage`, `p_reset`, `exit`.
As these 4 commands does not require any additional parsing. The sequence diagram referred above will return their
respective CommandObject to execute the command.


#### Sequence Diagram for `default`

![Parser Model Component](images/diagrams/Sequence_Parser_Default.png)

Choose a reason for hiding this comment

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

Are these sequence diagrams a bit repetitive?

@xX-Conan-Xx
Copy link

image
maybe it will be better to change the br1 to something else to make it clearer. I am a little confused when seeing br1

@xX-Conan-Xx
Copy link

image
maybe it will be better to delete the plus icon

Comment on lines +165 to +175
* Help: `help`
* Create a location: `create`
* Remove a location: `remove`
* Add an appliance: `add`
* Delete an appliance: `delete`
* Switch ON an appliance: `on`
* Switch OFF an appliance: `off`
* Listing appliance/location: `list`
* Displaying usage of appliance: `usage`
* Resetting usage of appliance: `p_reset`
* Exiting the application: `exit`

Choose a reason for hiding this comment

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

You may indicate the corresponding input format here for clearity

Comment on lines +217 to +219
#### Sequence Diagram for `on`

![Parser Model Component](images/diagrams/Sequence_Parser_On.png)

Choose a reason for hiding this comment

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

You may add a very short description for this part even though it is quite clear what this method does

Comment on lines +221 to +223
#### Sequence Diagram for `off`

![Parser Model Component](images/diagrams/Sequence_Parser_Off.png)

Choose a reason for hiding this comment

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

You may add a very short description for this part even though it is quite clear what this method does

## Design & implementation
## Table of Contents

Choose a reason for hiding this comment

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

Overall very clear DG for me to understand what your product about

@xX-Conan-Xx
Copy link

Overall good DG

@xX-Conan-Xx
Copy link

image
typo?

zongxian-ctrl and others added 30 commits November 9, 2020 19:26
Update to UserGuide and Developer Guide
Updates for Printing Style
Minor adjustments for printing
Minor edit on the PPP and DG
Minor amendment on personal PPP
Final DeveloperGuide.md update
Update of PPP for V2.1
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