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

[CS2113T-T12-2] PayMeBack #31

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

Conversation

yeezao
Copy link

@yeezao yeezao commented Sep 30, 2021

placeholder

@yeezao yeezao changed the title [CS2113-T12-2] PayMeBack [CS2113T-T12-2] PayMeBack Oct 28, 2021
## Design

### Architecture
The ***Architecture Diagram*** given above explains the high-level design of the App.

Choose a reason for hiding this comment

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

Diagram remained to be finished

Choose a reason for hiding this comment

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

yep I agree, better to have atleast a few class diagrams

The `Expense` class deals with most functionalities related to adding an expense inside a trip. The sequence diagram below shows how an expense is initialised.
![](../Resources/Expense%20Sequence%20Diagram.jpeg)

When `Parser` calls the `executeExpense` method, it creates an expense object, and also calls the `promptDate` method to set that expense object’s date. `promptDate` calls `isDateValid` to validate user input.


Choose a reason for hiding this comment

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

Is it possible to add some in this part?

Copy link

@KishorKumar11 KishorKumar11 left a comment

Choose a reason for hiding this comment

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

Overall, good job! Explanations are clear and precise, however just need to improve the readability factor 👍🏽


### `Expense` Class
The `Expense` class deals with most functionalities related to adding an expense inside a trip. The sequence diagram below shows how an expense is initialised.
![](../Resources/Expense%20Sequence%20Diagram.jpeg)

Choose a reason for hiding this comment

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

Unable to view sequence diagram. Perhaps you could recheck the directory/link?

Comment on lines 82 to 83
The `Parser` class
- Contains the logic to check user commands and break the input into the necessary contents for other classes to use
Copy link

@KishorKumar11 KishorKumar11 Oct 29, 2021

Choose a reason for hiding this comment

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

Perhaps, documentations like this can be followed for the other classes too?

Comment on lines 2 to 3

## Acknowledgements

Choose a reason for hiding this comment

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

Adding hyperlinks to subtopics would be a great idea! (like content pointers used in UG)

Comment on lines 55 to 60
The Person Class,
* Represents an individual that participated in an expense or a whole trip.
* A user-defined amount of `Person` objects will be created by the user during the create function of the `Trip` Class.
* Every time an object is created of the `Expense` Class, the user may define the people who were involved in the expense, however the people who are added to the expense must be already a `Person` object in the `Trip` object that the expense was made.
* One `Person` object who was involved in the expense will then be appointed as the payer of the group, the user will then have to indicate how much (in foreign currency) each of the participating persons spent for that particular expense. This is then stored and updated in each of the respective `Person` object’s `moneyOwed` HashMap, where a positive double refers to how much the person owes the respective Person object (i.e. the key of the HashMap) and a negative double refers to how much the Person object (i.e. the key of the HashMap) owes to that instance of the Person object.
* Example: If the HashMap = {person2 = 22, person3 = -11} in the person1 object, then person1 owes person2 $22 and person3 owes person1 $11.

Choose a reason for hiding this comment

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

As a fellow developer, I would like to recommend that the description for each your classes could follow a standard format. This would help the reader easily understand the idea/explanations of the concepts.

Copy link

@IshaaanVyas IshaaanVyas left a comment

Choose a reason for hiding this comment

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

Needs abit more content

## Design

### Architecture
The ***Architecture Diagram*** given above explains the high-level design of the App.

Choose a reason for hiding this comment

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

yep I agree, better to have atleast a few class diagrams

The ***Architecture Diagram*** given above explains the high-level design of the App.

Given below is a quick overview of main components and how they interact with each other.

Choose a reason for hiding this comment

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

You could add an object diagram here to show how the classes interact with one another

- Prints the information through the terminal.

### `Parser` Class

Choose a reason for hiding this comment

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

A UML class diagram would be helpful here to show the associations used for each command being called here.

The remaining components are as follows:

`Ui`: The User Interface of the App
`Parser`: The command executor

Choose a reason for hiding this comment

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

Having line breaks here may improve your DG as it makes the DG more readable and easier to understand, and it will be slightly less cluttered.

and is a container class for the expenses (each expense being represented by an
instance of the `Expense` class) and persons (each person being represented by an
instance of the `Person` class) tagged to the trip.

Choose a reason for hiding this comment

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

Since the Trip class contains many other classes and constructors of the other classes, would a UML diagram here help with the clarity of your DG?

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.

9 participants