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-T15-1] LongAh! #44

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

Conversation

djleong01
Copy link

@djleong01 djleong01 commented Mar 7, 2024

LongAh! is a CLI-based application designed to help users track debts within friend groups and determine the least transaction method of settling these debts. It is optimized for busy people with large transaction quantities among friends.

@djleong01 djleong01 changed the title [CS2113-T15-1] Long Ah! [CS2113-T15-1] LongAh! Mar 7, 2024
JeffinsonDarmawan pushed a commit to JeffinsonDarmawan/tp that referenced this pull request Mar 21, 2024
…ormatting

Change successful removal message
Copy link

@yuki-zmstr yuki-zmstr left a comment

Choose a reason for hiding this comment

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

Hi! I think your DG is nice and detailed. You could accompany the text description with more diagrams, as people like to see diagrams more than read text! (As far as I understand:) )

Comment on lines 40 to 48
```plantuml
@startuml
left to right direction

abstract class longah.commands.Command {
+ {abstract} execute(Group)
}

class longah.node.Transaction {

Choose a reason for hiding this comment

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

You could try and save the .puml as an image, and embed that image into the .md file.

Comment on lines 4 to 11
Command <|-- AddCommand
Command <|-- AddMemberCommand
Command <|-- AddTransactionCommand
Command <|-- AddGroupCommand
Command <|-- DeleteCommand
Command <|-- DeleteMemberCommand
Command <|-- DeleteTransactionCommand
Command <|-- DeleteGroupCommand

Choose a reason for hiding this comment

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

I think you do not need to mention every Command, perhaps mention a few examples. That is enough to give the reader an overview of the inheritance. Currently the diagram looks rather long.

Comment on lines 4 to 6
Command <|-- AddCommand
Command <|-- AddMemberCommand
Command <|-- AddTransactionCommand

Choose a reason for hiding this comment

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

You can try to use the hide circle command to hide the 'circle C', as our textbook does not mention that we need to have those icons

Comment on lines 519 to 526
### Chart

<ins>Overview</ins>

The Chart class is responsible for generating a visual representation of the transaction solution in the form of a chart.

The chart is displayed in a separate window and shows the individual balances among the group members.

Choose a reason for hiding this comment

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

Perhaps you could include a sample image of how the Chart will look like!

Comment on lines 315 to 329
<ins>Transaction Methods</ins>

- *parseTransaction*: Parses the user input to extract lender and borrowers, then adds them to the transaction.

- *addBorrower*: Adds a borrower to the transaction.

- *getLender*: Returns the lender of the transaction.

- *isLender*: Checks if a given member is the lender of the transaction.

- *isborrower*: Checks if a given member is a borrower in the transaction.

- *isInvolved*: Checks if a given member is involved in the transaction.

- *toStorageString*: Converts the transaction to a string format for storage.

Choose a reason for hiding this comment

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

I think in contrast to code that we write, documents could use more indentations, so that it's easy to spot headers. For example, you could indent the lines with tags.

Comment on lines +246 to +253
* *storageFolderPath*: A string containing the path to the storage directory specific to the group.
* *storageMembersFilePath*: A string containing the path to the `members.txt` directory associated with the group.
* *storageTransactionsFilePath*: A string containing the path to the `transactions.txt` directory associated with the group.
* *membersFile*: A File object representing the `members.txt` file associated with the group.
* *transactionsFile*: A File object representing the `transactions.txt` file associated with the group.
* *members*: A MemberList object representing the list of Members in the group.
* *transactions*: A TransactionList object representing the list of Transactions in the group.
* *scanners*: A size 2 array of Scanners to be used for loading data from the data storage files. The first Scanner in the array is used for reading from `members.txt` while the second is used for reading from `transactions.txt`.

Choose a reason for hiding this comment

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

Perhaps you could accompany this with a class diagram, showing the attributes!

Copy link

@claribelho claribelho left a comment

Choose a reason for hiding this comment

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

Diagrams are good, but there's a lack of class and object diagrams!

Each `StorageHandler` instance creates `members.txt` and `transactions.txt` in their respective folders. The file format is as follows.

* `members.txt` - NAME | BALANCE
* `transactions.txt` - LENDER NAME | TRANSACTION TIME(if applicable) | BORROWER1 NAME | AMOUNT1 | BORROWER2 NAME...

Choose a reason for hiding this comment

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

Unclear on what this is representing


<ins>Usage Example</ins>

![StorageHandler Sequence Diagram](diagrams/StorageHandlerSequenceDiagram.png)

Choose a reason for hiding this comment

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

Class names in sequence diagrams should have columns, eg :User


<ins> Usage Example </ins>

![pinhandler longah.png](diagrams%2Fpinhandler%20longah.png)

Choose a reason for hiding this comment

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

Class LongAh calls new PINHandler(). This should be the label for the arrow from LongAh instead of Initialise. Also, as in this case the constructor of PINHandler() is called, the activation bar should be directly below the class name :PINHandler

<ins>Usage Example</ins>

Adding a new transaction:
![addTransaction.png](diagrams%2FaddTransaction.png)

Choose a reason for hiding this comment

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

Class names should not be at the bottom of the sequence diagram

Copy link

@jasonlienardi jasonlienardi left a comment

Choose a reason for hiding this comment

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

Overall neat and tidy DG with good depth. Only minor errors found.

Choose a reason for hiding this comment

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

Is the format for the Class labels correct? Should it have a colon in front?

Choose a reason for hiding this comment

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

Should the bottom of the activation bar be open? Or should it be closed?

Choose a reason for hiding this comment

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

For self-invocation should the activation bar be on instead of pointing to the arrow? (eg. update members' balances)

Choose a reason for hiding this comment

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

Should there be an activation bar for self-invocation? (eg. updateTransactionSolution(), solveTransactions())

Copy link

@kyuichyi kyuichyi left a comment

Choose a reason for hiding this comment

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

Overall clean and neat DG, but diagrams have some slight issues.


The UML diagram below provides an overview of the classes and their interactions within the LongAh application.

![main.png](diagrams%2Fmain.png)
Copy link

Choose a reason for hiding this comment

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

Diagram is too complicated. Do not need to include getters and setters in the digram.


<ins>Methods</ins>

- *viewBalancesBarChart*(List<String> members, List<Double> balances): Generates a bar chart displaying member balances. It
Copy link

Choose a reason for hiding this comment

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

Most methods shown do not include the variables (if have). Perhaps can standardize if including the variables or not.


<ins>Implementation Details</ins>

![Command Inheritance Diagram](diagrams/CommandInheritance.png)
Copy link

Choose a reason for hiding this comment

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

Is there any way to simplify this diagram?

<ins>Usage Example</ins>

Adding a new transaction:
![addTransaction.png](diagrams%2FaddTransaction.png)
Copy link

Choose a reason for hiding this comment

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

Would be better if the lines did not intersect with the arrows.


*Data Storage:*

Each `StorageHandler` instance creates `members.txt` and `transactions.txt` in their respective folders. The file format is as follows.

Choose a reason for hiding this comment

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

Suggest adding the path directory for where these text files are being saved in.

The Transaction class is responsible for representing a single transaction in the LongAh application between 2 members.
It contains information about the lender, borrowers, and the amount involved in the transaction.


Choose a reason for hiding this comment

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

Nitpick: Extraneous spacing

* Update upon change, not upon exit - This allows for data to be saved even if the application exits ungracefully.
* *checkTransactions* - Methods are provided to have a quick check to ensure that data from data storage is not corrupted.

### Member and MemberList

Choose a reason for hiding this comment

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

Good use of a sequence diagram, though consider adding a class diagram as well since the class functionality is rather complicated

FeathersRe and others added 30 commits April 15, 2024 19:18
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