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

[CS2103T-F12-4] Billboard #20

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

Conversation

weiijiie
Copy link

@weiijiie weiijiie commented Sep 12, 2019

alages97 pushed a commit to alages97/main that referenced this pull request Oct 1, 2019
Adjust profile images naming, Remove learning outcomes

*MSS*

1. User requests to list expenses

Choose a reason for hiding this comment

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

Do you think that the listing step is redundant? What if the user already knows the index of the expense he wants to tag?



[Discrete]
=== UC02: Filter expenses by tag

Choose a reason for hiding this comment

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

Maybe you could filter by other fields also?



[Discrete]
=== UC06: Finding a record with a specified keyword

Choose a reason for hiding this comment

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

Maybe could use precondition of the keyword being valid characters (eg. English)

_{More to be added}_

[appendix]
== Non Functional Requirements

. Should work on any <<mainstream-os,mainstream OS>> as long as it has Java `11` or above installed.
. Should be able to hold up to 1000 persons without a noticeable sluggishness in performance for typical usage.
. Should be able to hold up to 1000 expenses/income records without a noticeable sluggishness in performance for typical usage.
. A user with above average typing speed for regular English text (i.e. not code, not system admin commands) should be able to accomplish most of the tasks faster using commands than using the mouse.

Choose a reason for hiding this comment

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

Maybe can refine this NFR by indicating how typing is faster than using mouse? Quantitatively?


|`* * *` |user |Tag a record | Categorize and better manage my records

|`* * *` |user |Specify a time stamp on expenses|Know when I spend my money

Choose a reason for hiding this comment

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

Are you considering having a feature of notifying the user?


|`* * *` |user |find a person by name |locate details of persons without having to go through the entire list
|`* * *` |Visually inclined user |Attach an image to each record |Conveniently record additional details of the expense instead of typing it all out

Choose a reason for hiding this comment

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

Is this really a must-have? Can the software still maintain its function without this feature?


|`* * *` |user |Specify a time stamp on expenses|Know when I spend my money

|`* * *` |user | Sort and filter records by category or tag | Know how my spending/income is distributed

Choose a reason for hiding this comment

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

Maybe can consider adding a search function?


|`* * *` |user | Archive past records | Better manage current expenses

|`* * *` | user | Unarchive records |

Choose a reason for hiding this comment

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

Would you consider having unarchive records as a must have, or should it be classified as nice to have?

Akimatsu98 pushed a commit to Akimatsu98/main that referenced this pull request Oct 9, 2019
Copy link

@chowyiyin chowyiyin left a comment

Choose a reason for hiding this comment

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

Good job :D


The following sequence diagram shows how the undo operation works:

.Interactions Inside the Logic Component for the `undo` Command
image::UndoSequenceDiagram.png[]

Choose a reason for hiding this comment

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

undoModel arrow should be dotted.

Copy link

Choose a reason for hiding this comment

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

Ditto.

Choose a reason for hiding this comment

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

Oops, thank you for pointing out that.


These actions are facilitated by the `ArchiveWrapper` and `Archive` classes:

* `Archive` extends from `ExpenseList` in order to encapsulate an archive name and a list of expenses together as an archive.

Choose a reason for hiding this comment

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

Can consider adding a class diagram

Firstly, the user has the application running and has a non empty list of current expenses. The user can enter the `list` command to bring up
this list. +

Next, the user executes the command `archive add 3 arc/archiveName` to archive an expense. +

Choose a reason for hiding this comment

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

Might want to break this up into steps


// end::finding[]

=== Statistics

Choose a reason for hiding this comment

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

No diagrams for statistics


Billboard will store all the command entered even the command is never executed.

This function is facilitated by the `CommandHistory` class.

Choose a reason for hiding this comment

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

No information about CommandHistory. Could add a class diagram

The CommandHistory will be initialized with no command state,
and the statePointer is not pointing to any command state.

.Initial state of CommandHistory

Choose a reason for hiding this comment

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

Seems like the diagram does not really serve a purpose

.Interactions Inside the Logic Component for the `delete` Command
image::DeleteSequenceDiagram.png[]

The LogicManager will store the command to CommandHistory before parsing it to ensure all commands are saved.

Choose a reason for hiding this comment

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

Could add design considerations

@weiijiie weiijiie changed the title [CS2103T-F12-4] Expenses Tracker [CS2103T-F12-4] Billboard Oct 25, 2019
The archive feature supports the following actions:

* Creating an archive
* Added an expense to an archive
Copy link

Choose a reason for hiding this comment

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

There's past tense here, but the rest of this list is present continuous.

Copy link

@michelleykw michelleykw left a comment

Choose a reason for hiding this comment

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

For the figures under 3.2.1 in the Developer Guide, perhaps, it would be better to label the figures and split the full sequence diagram into small parts as the words are too small to be seen without zooming in.

Copy link

@tswuxia tswuxia left a comment

Choose a reason for hiding this comment

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

  • Section 2.6 has insufficient details
    image

  • Section 2.4 2.5 the diagrams were not updated
    image

  • It would be better to use different fonts for methods, arguments etc. in the description
    image

Copy link

@heze8 heze8 left a comment

Choose a reason for hiding this comment

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

Overall, it's reasonably neat and clean, good luck on improving it.

Other comments are left in the specific section.

this list. +

Next, the user executes the command `archive add 3 arc/archiveName` to archive an expense. +
The command is first parsed by `BillboardParser` to determine what kind of general command it is. `archive` indicates it is an archive command so the remaining input is parsed through `ArchiveCommandParser`. +
Copy link

Choose a reason for hiding this comment

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

This paragraph uses the same font and bolding for classes, arguments, commands, and quotations. This might be confusing to the reader as they might not know what each bolded word represents. Maybe separating out the font and standardizing would make it better.

`Storage` to serialize into JSON format and writes it into the JSON file.

==== Design Considerations
===== Aspect: Data Structure to hold archives in `ArchiveWrapper`
Copy link

Choose a reason for hiding this comment

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

Neat and tidy, perhaps a table could also be considered.

Copy link

@tysg tysg left a comment

Choose a reason for hiding this comment

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

Good features presented, overall neat and detailed.

@@ -98,7 +100,7 @@ image::LogicClassDiagram.png[]
*API* :
link:{repoURL}/src/main/java/seedu/address/logic/Logic.java[`Logic.java`]
Copy link

Choose a reason for hiding this comment

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

What's the difference between this diagram and the diagram under the architecture part? Seems like it's repeated..


The following sequence diagram shows how the undo operation works:

.Interactions Inside the Logic Component for the `undo` Command
image::UndoSequenceDiagram.png[]
Copy link

Choose a reason for hiding this comment

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

Ditto.

image::UndoRedoState4.png[]

Step 6. The user executes `clear`, which calls `Model#commitAddressBook()`. Since the `currentStatePointer` is not pointing at the end of the `addressBookStateList`, all address book states after the `currentStatePointer` will be purged. We designed it this way because it no longer makes sense to redo the `add n/David ...` command. This is the behavior that most modern desktop applications follow.
Step 6. The user executes `clear`, which calls `VersionedBillboard#commit()`. Since the `currentStatePointer` is not pointing at the end of the `stateList`, all billboard states after the `statePointer` will be purged. We designed it this way because it no longer makes sense to redo the `add n/buy ...` command. This is the behavior that most modern desktop applications follow.
Copy link

Choose a reason for hiding this comment

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

If clear deletes all the states, then where does current state point to?

Choose a reason for hiding this comment

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

The current state should point to bb3:Billboard, sorry for my typo. Just in case you are unclear about how it works, I will further explain it. If you already know how it works, please ignore below. If the user executes any other billboard editing command(add, delete etc), then all the state after the current state will be removed, then execute the command, and store it into the state list, current state pointer will point to that state. And you will not able to redo cause the state pointer is pointing to the last state in the list.

`ArchiveWrapper` is used in `ModelManager` and its respective operations are called to access and manipulate archive expenses when an archive command is entered. +
Such operations include:

* `ArchiveWrapper#AddArchive(Archive)` - Adds the given archive to the current map of archive objects.
Copy link

Choose a reason for hiding this comment

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

This information can be found in Java doc. We feel that it's a bit redundant to leave them here.


These interactions with `Model` by `AddArchiveCommand` can be shown in the cropped portion of the full sequence diagram below:

image::AddArchiveCommandSequenceDiagram_executeCommand.png[]
Copy link

Choose a reason for hiding this comment

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

Good diagram!!


Below is the full sequence diagram of the operation:

image::AddArchiveCommandSequenceDiagram.png[]
Copy link

Choose a reason for hiding this comment

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

The font is a bit too small


The following sequence diagram shows how the adding tag operation works.

image::AddTagSequenceDiagram.png[]
Copy link

Choose a reason for hiding this comment

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

retrieveTag's return object is not defined in this sequence diagram.

Copy link

Choose a reason for hiding this comment

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

also the lines should stop after the cross, though we are aware of the technical difficulty of doing so.

waynefong0401 and others added 30 commits November 11, 2019 21:20
* master:
  Fixed some sentences
  Completed developer guide
  Updated overall flow for developer guide and added new chart
  Edited introduction for developer guide
* master:
  Fix increment tag count
  Fix bugs mentioned in Wai Fong's review
  Fix codacy
  Fix bugs and javadoc typos
  Fix checkstyletest
  Fix codacy
  Fic checkstyletest
  Fix test
  Add recurring expenses test cases
  Remove duplicate getClone method
  Implement list and add recurrence without undo/redo and save/load
  Init commit for recurring expenses
# Conflicts:
#	docs/DeveloperGuide.adoc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet