-
Notifications
You must be signed in to change notification settings - Fork 65
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
[T09-2] Anakin #65
base: master
Are you sure you want to change the base?
[T09-2] Anakin #65
Conversation
Hi @truongthanh2606, your pull request title is invalid. For PR sent to satisfy a learning outcome, the PR name should be in the format of For team PR, the PR name should be in the format of Please follow the above format strictly and edit your title for reprocessing. Note: this comment is posted by a bot. If you believe this is done in error, please create an issue at nus-se-pr-bot and add a link to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kindly read the comments for suggestions. Do NOT close this PR. Please update your Developer Guide's appendix sections as stated in the project milestone requirements. Please read the requirements carefully for every week so that you don't miss the milestone requirements. Otherwise, fine work. Remember to tag all other group members (using the @ notation) in the PR description so that they can receive notifications regarding this PR.
README.adoc
Outdated
|
||
= Anakin | ||
|
||
image:https://api.codacy.com/project/badge/Grade/51c8e6629cba4d068c77e39bf0094e4c[link="https://app.codacy.com/app/leeyjjoel/main?utm_source=github.com&utm_medium=referral&utm_content=CS2103-AY1819S1-T09-2/main&utm_campaign=Badge_Grade_Dashboard"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are there 2 codacy badges in the ReadMe.md?
build.gradle
Outdated
'site-name': 'AddressBook-Level4', | ||
'site-githuburl': 'https://github.com/se-edu/addressbook-level4', | ||
'site-name': 'Anakin', | ||
'site-githuburl': 'https://github.com/CS2103-AY1819S1-T09-2/Main.git', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the site-githuburl still /Main.git instead of /main.git? Should it contain .git?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rectified in 68fbd75
docs/DeveloperGuide.adoc
Outdated
@@ -12,7 +12,7 @@ ifdef::env-github[] | |||
:note-caption: :information_source: | |||
:warning-caption: :warning: | |||
endif::[] | |||
:repoURL: https://github.com/se-edu/addressbook-level4/tree/master | |||
:repoURL: https://github.com/CS2103-AY1819S1-T09-2/Main.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repoURL should not have .git at the back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rectified in ccaca8f
docs/UserGuide.adoc
Outdated
* *History* : `history` | ||
* *Undo* : `undo` | ||
* *Redo* : `redo` | ||
* See above please :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to not have a command summary? Perhaps you can add it back once the user guide is filled with more commands.
@@ -12,7 +12,7 @@ ifdef::env-github[] | |||
:note-caption: :information_source: | |||
:warning-caption: :warning: | |||
endif::[] | |||
:repoURL: https://github.com/se-edu/addressbook-level4/tree/master | |||
:repoURL: https://github.com/CS2103-AY1819S1-T09-2/Main.git | |||
|
|||
By: `Team SE-EDU` Since: `Jun 2016` Licence: `MIT` | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are the updated user stories, use cases, NFRs and product scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refer to them at https://github.com/CS2103-AY1819S1-T09-2/main/blob/master/docs/DeveloperGuide.adoc
docs/UserGuide.adoc
Outdated
* Existing values will be updated to the input values. | ||
* When editing tags, the existing tags of the person will be removed i.e adding of tags is not cumulative. | ||
* You can remove all the person's tags by typing `t/` without specifying any tags after it. | ||
*Name will be taken as a string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to check whether you have applied the markup syntax correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rectified in 2e3bcec
docs/UserGuide.adoc
Outdated
**** | ||
|
||
Examples: | ||
* `newdeck n/My First Deck` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good practice to ensure that your examples have a consistent format across the commands.
/** | ||
* Represents a Card inside a Deck. | ||
*/ | ||
public class Anakin_Card { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class names should not have underscores. Ensure that they follow PascalCase. Perhaps you can rename it to AnakinCard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in #146
/** | ||
* Represents a Deck inside Anakin. | ||
*/ | ||
public class Anakin_Deck { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above comment, the class name needs to be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in #146
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, good job. Most of the documentation is updated. Some points to note:
- ContactUs page needs to be changed (This is your product, they should not be contacting Prof. Damith)
- For features yet to be implemented by the milestone, mark them as "Coming in V2.0" in the UserGuide
- Certain badges does not seem to be following your team repo, but a member's repo's status instead. (E.g. AppVeyor, Codacy)
As usual, do not close the PR. Continue to work on your individual features and upgrade the documentation as needed.
docs/DeveloperGuide.adoc
Outdated
.List of per-file attributes, excluding Asciidoctor's built-in attributes | ||
|=== | ||
|Attribute name |Description |Default value | ||
*Value proposition*: manage flashcards faster than a typical mouse/GUI driven app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps your value proposition can be more detailed so that users are more tempted to try the app? (What is the benefit of managing flashcards faster than manual/mouse/GUI driven app?)
docs/DeveloperGuide.adoc
Outdated
|
||
The files in link:{repoURL}/docs/templates[`docs/templates`] controls the rendering of `.adoc` files into HTML5. | ||
These template files are written in a mixture of https://www.ruby-lang.org[Ruby] and http://slim-lang.com[Slim]. | ||
|`* * *` |user |find a person by name |locate details of persons without having to go through the entire list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Anakin keeping a list of persons in addition to flashcards?
docs/DeveloperGuide.adoc
Outdated
|
||
[discrete] | ||
==== `Logic` component | ||
*Alternate Use Case* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean there are alternative methods to delete a card from the deck? Perhaps you can place them under extensions [E.g. 3b. User selects a deck. (Rest of alternate path)]
docs/DeveloperGuide.adoc
Outdated
|
||
[appendix] | ||
== Non Functional Requirements | ||
|
||
. Should work on any <<mainstream-os,mainstream OS>> as long as it has Java `9` or higher installed. | ||
. Should be able to hold up to 1000 persons without a noticeable sluggishness in performance for typical usage. | ||
. Should be usable on a laptop with average hardware | ||
. Internal state should be persistent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the term internal state can be clearer? Does it refer to storage? Or whenever the App is launched, it resumes the previous session state the user has left it? You can define the meaning under glossary.
|
||
[[Docs-PerFileDocSettings]] | ||
=== Per-file Documentation Settings | ||
[appendix] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for removing the design, implementation, documentation, testing and devops? These are required in the end product. Remember that the developer guide is supposed to guide new developers in performing certain tasks like testing, setting up documentation etc.
docs/UserGuide.adoc
Outdated
Exits the program. + | ||
Format: `exit` | ||
**** | ||
* Note: User must be inside a deck to perform this command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Note:" here feels odd when the statement is placed in a box.
* *History* : `history` | ||
* *Undo* : `undo` | ||
* *Redo* : `redo` | ||
A convenient cheat sheet of commands |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your command summary, the description of certain commands seems different from that in the earlier section.
README.adoc
Outdated
ifdef::env-github[] | ||
image::docs/images/Ui.png[width="600"] | ||
image:docs/images/ui-mockups/Gameplay-Question.png[width="400"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UI mockups should be 1 screenshot
** A more sophisticated GUI that includes a list panel and an in-built Browser. | ||
** More test cases, including automated GUI testing. | ||
** Support for _Build Automation_ using Gradle and for _Continuous Integration_ using Travis CI. | ||
|
||
== Site Map | ||
|
||
* <<UserGuide#, User Guide>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should remove the link to Learning Outcomes in your final product.
} | ||
|
||
/* | ||
Name name = ParserUtil.parseName(argMultimap.getValue(PREFIX_NAME).get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments:
- You are supposed to tag your latest commit (before the deadline) as v1.2.
- You have to tag features listed in UserGuide as "Coming in v2.0" if it is not implemented yet.
- Do continue to update your DeveloperGuide, and watch out for mistakes in your diagrams.
- Try to standardize your component diagrams.
- Try to fix CheckStyle errors so that Travis can actually run tests to see if your code is working as intended.
Please read the comments, and ask questions if there is anything unclear.
Keep up the good work.
image::m133225.jpg[width="150", align="left"] | ||
{empty}[http://github.com/m133225[github]] [<<johndoe#, portfolio>>] | ||
=== David Goh | ||
image::lessthanfree.png[width="150", align="left"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lessthanfree Your picture may need to be changed such that your face can be seen more clearly.
README.adoc
Outdated
ifdef::env-github[] | ||
image::docs/images/Ui.png[width="600"] | ||
image:docs/images/ui-mockups/Gameplay-Question.png[width="400"] | ||
image:docs/images/ui-mockups/Gameplay-Answer.png[width="400"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to have your one of your images named "docs/images/Ui.png" so that our scripts can download it.
docs/UserGuide.adoc
Outdated
|======================================================================= | ||
|Command | What does it do? | ||
|`list` | Lists all decks | ||
|`newdeck [n/NAME]` | Creates a new deck. /n is optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be "n/" instead of "/n"?
|
||
*API* : | ||
link:{repoURL}/src/main/java/seedu/address/logic/Logic.java[`Logic.java`] | ||
The `LogicManager` contains an `AnakinModel`, an `AnakinParser`, and a `CommandHistory`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cant seem to find AnakinModel
in your diagram. Your diagram has an orange dotted line pointing from LogicManager
to CommandHistory
. Does that mean that it contains and modifies CommandHistory
? [It was not stated in the Legend]
|
||
*API* : link:{repoURL}/src/main/java/seedu/address/model/Model.java[`Model.java`] | ||
Structure of the Model Component |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to standardize your class diagram appearance.
|
||
*API* : link:{repoURL}/src/main/java/seedu/address/model/Model.java[`Model.java`] | ||
Structure of the Model Component | ||
|
||
The `Model`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe your model is actually called AnakinModel
instead of Model
? [The diagram does not reflect the new name]
|
||
// end::dataencryption[] | ||
These operations are exposed in the Model interface as: Model#addCard(Card card), Model#deleteCard(Card card), Model#updateCard(Card target, Card editedCard) respectively. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to rename Model
to the actual name of the interface.
docs/DeveloperGuide.adoc
Outdated
|
||
Certain properties of the application can be controlled (e.g App name, logging level) through the configuration file (default: `config.json`). | ||
image::NewCard sequence diagram.png[width="790", align="left"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Make sure the arrows touch the activation bars.
- Would alt be more suitable than opt? If exceptions are thrown, would the rest of the operation still occur in
Anakin
andUniqueCardList
? Using opt would imply that an Exception instance is created, but add operation still continues. - It is fine to leave out cases where exception is thrown. You just need to explain the main scenario of
newcard
operation.
docs/DeveloperGuide.adoc
Outdated
|
||
[TIP] | ||
Attributes left unset in the `build.gradle` file will use their *default value*, if any. | ||
image::NewDeck.png[width="790", align="left"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments for the above Sequence diagram applies here.
|
||
=== Site Template | ||
Step 5. Card A is shown to the user more regularly when he reviews the same deck in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do try to add more information on the implementation (Diagrams and its explanations can help) (Design considerations can help too)
Add command, Date and UUID
And Formatting
Import Export feature
Fix bugs related to import deck command
Also update exception messages for EditDeck and NewDeck
* Add FindCommandSystemTest Deck level checks * Fix Style Errors * Fix Style Errors * Add Deck_G and Deck_H * Fix Style Errors * Change person to deck * Change AddressbookMessages to Messages Co-Authored-By: leeyjjoel <lee.yi.jie.joel@gmail.com>
Also update exception messages for EditDeck and NewDeck
Co-Authored-By: Lessthanfree <35485082+Lessthanfree@users.noreply.github.com>
Co-Authored-By: Lessthanfree <35485082+Lessthanfree@users.noreply.github.com>
Co-Authored-By: Lessthanfree <35485082+Lessthanfree@users.noreply.github.com>
Fix bug: developer guide does not show image content
Fix UserGuide and Add appendix to Dev Guide
Update PPP
Update ppp
New card sequence diagram is now shown in developer guide
Fix image not shown bug
Update Developer Guide and PPP
Update Developer guide
* Fix style errors * Fix style errors
@leeyjjoel @tomforge @yujiatay @Lessthanfree