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-T17-4] parKING #11

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

Conversation

malcolmang
Copy link

A parking CLI that helps drivers choose the best place to park.


## Design & implementation
#### Model Component
![Model Class Diagram](images/ModelClassDiagram.png)
Copy link

@gaoyunfan gaoyunfan Oct 26, 2022

Choose a reason for hiding this comment

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

I think the format for method's parameter-list in class diagram is name: type for carparkList

@@ -1,12 +1,186 @@
# Developer Guide

Choose a reason for hiding this comment

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

Maybe could included an intro about ur project


#### Common Files

## Implementation

Choose a reason for hiding this comment

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

Implementation not done yet, do add it when you can!

- Authenticate user API key. If no user key inputted, default key will be loaded.
- Get API authentication status.

#### Storage Component

Choose a reason for hiding this comment

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

Do add the other components too!!


## Design

### Architecture Level

Choose a reason for hiding this comment

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

Do add the architecture level section!

See [`FileLoader`](#FileLoader) for more information.

#### API Component
![API Class Diagram](images/ApiClassDiagram.png)

Choose a reason for hiding this comment

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

Perhaps the uml visibility symbols such as + or - should be used instead


The following sequence diagrams shows how a favourite / unfavourite command works:

![Favourite Sequence Diagram](images/FavouriteSequenceDiagram.png)

Choose a reason for hiding this comment

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

inconsistent in labelling of return value. There is no label for return findcarpark() call.


### Favourite / Unfavourite feature

![Favourite Class Diagram](images/FavouriteClassDiagram.png)

Choose a reason for hiding this comment

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

perhaps can add in the multiplicity of Favourite

Note: The `Carpark` class contain many getters, setters and annotations to be used with the `jackson` module.
See [`FileLoader`](#FileLoader) for more information.

#### API Component

Choose a reason for hiding this comment

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

Would be good to add labels for the arrows in the uml to make it clear


![Favourite Class Diagram](images/FavouriteClassDiagram.png)

#### Implementation

Choose a reason for hiding this comment

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

The write up of the implementation is pretty clear, with good bullet points and segmentation. Good job!

Copy link

@nshian nshian left a comment

Choose a reason for hiding this comment

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

Overall, an informative guide with some suggestions for your consideration


{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well}
### Component Level
Copy link

Choose a reason for hiding this comment

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

Parameter-list for methods should be of the form [name of variable] : [type of variable] e.g. setNumber(n: Integer)


### Favourite / Unfavourite feature

![Favourite Class Diagram](images/FavouriteClassDiagram.png)
Copy link

Choose a reason for hiding this comment

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

Maybe you want to consider including the multiplicity of the Favourite class as well? Since the other associations you have included have multiplicities on both ends


The following sequence diagrams shows how a favourite / unfavourite command works:

![Favourite Sequence Diagram](images/FavouriteSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

Perhaps you might want to write the [condition] in the alt block in normal prose? e.g. user favourites list, user favourites a specific carpark by ID


The following sequence diagrams shows how a favourite / unfavourite command works:

![Favourite Sequence Diagram](images/FavouriteSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

Consider adding a label for the return value for the findCarpark() method?

1. Land Transport Authority DataMall API Service ([link](https://datamall.lta.gov.sg/content/datamall/en.html)).
2. Jackson JSON Parser ([link](https://fasterxml.github.io/jackson-core/javadoc/2.8/com/fasterxml/jackson/core/JsonParser.html))

## Design
Copy link

Choose a reason for hiding this comment

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

More sequence diagrams for some other commands would be nice


#### Common Files

## Implementation


## Product scope

Choose a reason for hiding this comment

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

referring to line 229, ## User Stories

please consider adding more user stories




#### Design considerations

Choose a reason for hiding this comment

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

can consider resizing the image of the sequence diagram used for increased clarity

{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
#### API Component
![API Class Diagram](images/ApiClassDiagram.png)

Choose a reason for hiding this comment

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

the chunk of text is hard to grasp/understand. perhaps, you can reduce the number of words/simplify the explanation

- changeScanner() - Changes the scanner for the Ui object. To be used for JUnit testing.
- getSeparatorString() - Returns a separator string.

#### Logic Component

Choose a reason for hiding this comment

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

image

unclear what is XYZCommand in the diagram, not described in your description as well.

Copy link

@kohnh kohnh left a comment

Choose a reason for hiding this comment

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

LGTM

#### Storage Component

#### Implementation
![Sequence Diagram](images/LoadFileSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

image
consider adding a "X" at the end of the lifeline of an object to show its deletion

### Component Level

#### Model Component
![Model Class Diagram](images/ModelClassDiagram.png)
Copy link

Choose a reason for hiding this comment

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

image
Should use the symbols taught in class like + or -


The following sequence diagrams shows how a favourite / unfavourite command works:

![Favourite Sequence Diagram](images/FavouriteSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

image

Both alts seems similar except for the command call, maybe can combine the 2


The following sequence diagrams shows how a favourite / unfavourite command works:

![Favourite Sequence Diagram](images/FavouriteSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

Maybe you can make the diagram bigger for easier reading. The diagram is also not sharp.


### Favourite / Unfavourite feature

![Favourite Class Diagram](images/FavouriteClassDiagram.png)
Copy link

Choose a reason for hiding this comment

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

image

Maybe you can remove the circle for class as it is not taught in class.

- Get API authentication status.

The following sequence diagram shows how the API key is loaded.
![Api Loading Sequnce Diagram](images/LoadApiSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

image
The origin of this arrow is unclear. Maybe you should include what the storage class calls in order to return the API_KEY

Copy link

@exetr exetr left a comment

Choose a reason for hiding this comment

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

Good use of the three types of diagrams to augment elaboration of your application. Most of the diagrams are fine, although do be careful of the small details such as the format of header for class diagrams and the visibilty specifer for objects to follow the format stated by the module specifications.

### Component Level

#### Model Component
![Model Class Diagram](images/ModelClassDiagram.png)
Copy link

Choose a reason for hiding this comment

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

Double check your attribute and operations visibility identifiers, they should align with the module's specifications


## Design & implementation
Note: The `Carpark` class contain many getters, setters and annotations to be used with the `jackson` module.
See [`FileLoader`](#FileLoader) for more information.
Copy link

Choose a reason for hiding this comment

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

Minor issue, but currently the fileloader link is broken, do double check it.

* Only reads from the secret.txt file once and stores the API as a variable (will only read from the file
if requested again).

How the API component of data fetching works
Copy link

Choose a reason for hiding this comment

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

For specificity, you may want to consider what type of data you are pulling from the API.

#### Storage Component

#### Implementation
![Sequence Diagram](images/LoadFileSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

This sequence diagram seems overly complicated, do try to re-visit whether it is necessary to depict every aspect of the sequence diagram. If it is requried, you can potentially consider splitting it up into multiple diagrams, or using ref frames.

* All `ABCCommand` classes will have an override `execute` method, which will override the `execute` method in `Command`
and return the respective `CommandResult` result of the Command.

![Logic Class Diagram](images/LogicClassDiagram.png)
Copy link

Choose a reason for hiding this comment

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

Is the XYZCommand class redundant? If so, do consider removing it

* All `ABCCommand` classes will have an override `execute` method, which will override the `execute` method in `Command`
and return the respective `CommandResult` result of the Command.

![Logic Class Diagram](images/LogicClassDiagram.png)
Copy link

Choose a reason for hiding this comment

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

Same case here, do double check that you use the correct visibilty indicators as specified by the module.


The following sequence diagrams shows how a favourite / unfavourite command works:

![Favourite Sequence Diagram](images/FavouriteSequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

Consider enlarging the size of this sequence diagram for greater clarity.

malcolmang and others added 30 commits November 7, 2022 18:31
Change favourite list to throw the correct exception
Add disclaimer about tampering with file permissions
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