[CS2113T-T13-2] Nuke #16
Conversation
…branch-Joseph merge fixed command parser
|
|
||
| {Describe the target user profile} | ||
| By: `CS2113T-T13-2` Since: `Feb 2020` | ||
| <small>[Go to Webpage](https://ay1920s2-cs2113t-t13-2.github.io/tp/DeveloperGuide.html)</small> |
There was a problem hiding this comment.
Good use of links to navigate through the link
docs/DeveloperGuide.md
Outdated
|
|
||
| Below are the class-diagram for the involved classes: | ||
|
|
||
|  |
There was a problem hiding this comment.
The team have clear and useful class diagram, you may add an interface class for Command class.
docs/DeveloperGuide.md
Outdated
| 3. There are **multiple** matches -- The list of matches will be shown to the user, and the user chooses which ones to delete. A further prompt will be given to confirm the deletion(s). | ||
|
|
||
| #### Feature Implementation | ||
|  |
There was a problem hiding this comment.
I think in my opinion, there are too many detail in one diagram. I suggest we can keep it simple how it is shown in Add :)
There was a problem hiding this comment.
I agree. maybe it could be cleaned up by removing unimportant attributes in the leaf classes
docs/DeveloperGuide.md
Outdated
| Lastly, the `FilterCommand` class extends the abstract `Command` class that contains the `execute()` method to execute the actual **delete** command. | ||
| <br> | ||
|
|
||
|  |
There was a problem hiding this comment.
YES this is perfect amount of information! Clear and minimalist but still provide enough information for the reader to understand the class diagram
docs/DeveloperGuide.md
Outdated
|
|
||
| Below is a *sequence diagram* to illustrate the above example scenario. | ||
|
|
||
|  |
There was a problem hiding this comment.
There is a sequence diagram. 😄
However are are a few improvement that I can find:
- PrepareDeleteTaskCommand is not needed,
- createFilterTaskList() instead of just createFilterTaskList
- The return arrow if there is no object to return, there is no need for them to label them.
Other then these type of error there is nothing wrong with the sequence diagram !!
Great use of Sequence diagram to explain how delete function works. Provide enough information
not too much and not too less to the reader to understand the flow of the sequence.
Good job 👍
docs/DeveloperGuide.md
Outdated
| 2. James receive the following feedback: | ||
|
|
||
| ``` | ||
| root : |
There was a problem hiding this comment.
I think that a picture would be better as it show what the actual output should look like 🤔
docs/DeveloperGuide.md
Outdated
| 4 | CS3235 | Computer Security | ||
| +--------------------------------------------------------------------------------------------------+ | ||
| Total modules: 4 | ||
| +--------------------------------------------------------------------------------------------------+ |
There was a problem hiding this comment.
The diagram is useful and help the reader understand the output better
docs/DeveloperGuide.md
Outdated
| Below is a *sequence diagram* to illustrate the above example scenario. | ||
|
|
||
| ``` | ||
| to-do: add the sequence diagram |
There was a problem hiding this comment.
Yes please add more sequence diagram to aid the reader understand the feature more 👍
docs/DeveloperGuide.md
Outdated
| Below is a *sequence diagram* to illustrate the above example scenario. | ||
|
|
||
|  | ||
| <span style="color: green"><small><i>Figure <b>Delete Command Sequence Diagram</b></i></small></span> |
There was a problem hiding this comment.
I am not sure but I think line crossing through parser eg in sequence diagram 2 is not allowed ? 🤔
docs/DeveloperGuide.md
Outdated
| 1. `DuplicateModuleException` will be thrown if the module specified by the user is contained in the `ArrayList` named `moduleList` in `ModuleManager` class. | ||
| 2. `ModuleNotProvidedException` will be thrown if the module code specified by the user is not contained in the `HashMap` named `modulesMap` in `ModuleManager` class. | ||
|
|
||
| Below are the class-diagram for the involved classes: |
There was a problem hiding this comment.
Could it make sense to add the interaction with ModuleManager to this class diagram?
docs/DeveloperGuide.md
Outdated
|
|
||
|
|
||
|
|
||
| #### 1.2. Add Category and Task Features |
There was a problem hiding this comment.
Maybe can add some more visuals to this section? It was a bit hard for me to follow just reading along
docs/DeveloperGuide.md
Outdated
| 3. There are **multiple** matches -- The list of matches will be shown to the user, and the user chooses which ones to delete. A further prompt will be given to confirm the deletion(s). | ||
|
|
||
| #### Feature Implementation | ||
|  |
There was a problem hiding this comment.
I agree. maybe it could be cleaned up by removing unimportant attributes in the leaf classes
| Refer to the guide [here](#) to set up. | ||
|
|
||
| <br> | ||
|
|
There was a problem hiding this comment.
Maybe adding a high level architecture view at the beginning would be helpful. I think it would've made it easier for me to read the implementation stuff if I had some more context
There was a problem hiding this comment.
i know the developer guide is unfinished but just to keep in mind
docs/DeveloperGuide.md
Outdated
|
|
||
| <br> | ||
|
|
||
| Below is a *sequence diagram* to illustrate the above example scenario. |
There was a problem hiding this comment.
I think the annotations between the sequence diagram and the above example were really well done. It made a complex sequence diagram easy to follow
| package seedu.nuke.command; | ||
|
|
||
| public abstract class Command { | ||
| // public static String COMMAND_WORD; |
There was a problem hiding this comment.
If it's unnecessary, it may need to be deleted after
| * | ||
| * @see Command | ||
| */ | ||
| public abstract class AddCommand extends Command { |
There was a problem hiding this comment.
Hi there, I see in the figure, there is a
getParentDictionary.
But I haven't seen it in the code..
| * @see Command | ||
| * @see Module | ||
| */ | ||
| public class AddModuleCommand extends AddCommand { |
There was a problem hiding this comment.
There is also without getParentDictionary()
| } | ||
| } | ||
|
|
||
| protected ArrayList<Directory> createFilteredFileList(String moduleKeyword, String categoryKeyword, |
There was a problem hiding this comment.
Haven't been written in the Class diagram
| } | ||
| } | ||
|
|
||
| private ArrayList<Directory> filterModules(String moduleKeyword, boolean isExact) { |
There was a problem hiding this comment.
Is it not important so it wasn't written in the diagrams?
| * @see Command | ||
| * @see TaskFile | ||
| */ | ||
| public class DeleteFileCommand extends DeleteCommand { |
There was a problem hiding this comment.
Haven't been written in the class diagram.. is it not vital?
| * @throws IncorrectDirectoryLevelException | ||
| * If there is illegal access to a directory | ||
| */ | ||
| private boolean isCurrentDirectoryInList(ArrayList<Directory> filteredList) |
There was a problem hiding this comment.
Haven't been written in the code. Is it not correct?
docs/DeveloperGuide.md
Outdated
|
|
||
| <br> | ||
|
|
||
| Below is a *sequence diagram* to illustrate the above example scenario. |
There was a problem hiding this comment.
I think the sequence diagram is really detailed and makes the above scenarios easier to understand.
But would it be better to place each picture together with its related senario? It's easier for reader to follow and understand.
docs/DeveloperGuide.md
Outdated
|
|
||
| Below is a *sequence diagram* to illustrate the above example scenario. | ||
|
|
||
|  |
There was a problem hiding this comment.
Here are some suggestions based on my understanding:
-
- Method calls should have a
(), for example useexecuteInitialDelete()instead ofexecuteInitialDelete
- Method calls should have a
-
- Maybe it's better to add constuct method on the arrow representing the call to the constructor.
docs/DeveloperGuide.md
Outdated
| 1. enter `cd cs3235` to enter the module directory then enter `addc misc` to add the category. | ||
| 2. enter `addc misc -m cs3235` to add the category at the root directory. | ||
|
|
||
| Suppose James use the first method, after the second input, the input is parsed as an **add task** command and executed, the `AddCategoryCommand#execute()` will call ``AddCategoryCommand#getParentDirectory()` to get the current parent directory, then it will instantiate an `Category` object with the name "misc". After which `CategoryManager#add()` will be called to add the new object. In the `CategoryManager#add()` method, it will call `CategoryManager#contains()` method to check if the current parent directory contains the category with name "misc", finally add the object into the `ArrayList` of `categoryList`. |
There was a problem hiding this comment.
Seems that you accidentally typed an extra symbol so the format of this paragraph is incorrect
``AddCategoryCommand#getParentDirectory()`
| Below are the class-diagram for the involved classes: | ||
|
|
||
| ``` | ||
| to-do: add the class-diagram |
There was a problem hiding this comment.
The description is quite clear but I still feel a bit hard to follow without a diagram.
Look forward to a detailed diagram!
docs/DeveloperGuide.md
Outdated
| 2. The name of the specified category/task already exist. -- No category/task will be added. | ||
| 3. The specified upper parent directory exist and the name of the specified category/task does not exist. -- The specified category/task will be added. | ||
|
|
||
| #### Feature Implementation |
There was a problem hiding this comment.
The implementation is quite clear, but have you considered alternative ways to implement this feature?
I think it would be better to make a comparison between your current choice and alternatives you may have considered.
So it's more clear to see pros and cons of your current choice.
docs/DeveloperGuide.md
Outdated
|
|
||
| <br> | ||
|
|
||
| ## **Design** |
There was a problem hiding this comment.
I think a Architecture Diagram can be provided here. So it's easier to understand the main workflow and how components interact with each other.
JunitTest of List Commands
Add more jUnit tests for directory package
add JUnit test
minor change
add JUnit test for ChangeDirectoryCommand.java
Update PPP
Add more contributions to PPP
minor updates on ug
updated email addresss
update ppp
Update iceclementi.md
shorten ppp
update README.md
No description provided.