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-T18-4] NUScents #18

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

Conversation

lckjosh
Copy link

@lckjosh lckjosh commented Oct 5, 2023

Welcome to NUScents, the tailor-made financial tracker for SOC students at NUS. It is optimized for use via a Command Line Interface (CLI) to offer a clutter-free solution for our users to manage and monitor their financial activities.

Brian030601 added a commit to Brian030601/tp that referenced this pull request Oct 18, 2023
Copy link

@irving11119 irving11119 left a comment

Choose a reason for hiding this comment

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

Overall your code has decent abstraction and follow good OOP principles. However, continue looking into how you can refactor your code to make it more readable and neater!

Comment on lines 138 to 145
String date = extractValue("allowance", arguments, DATE_PATTERN, false);
String description = extractValue("allowance", arguments, DESC_PATTERN, false);
String additionalInformation = extractValue("allowance", arguments, NOTE_PATTERN, true);
String category = extractValue("allowance", arguments, CATEGORY_PATTERN, true);
AllowanceCategory allowanceCategory = parseAllowanceCategory(category);
String format = datePatternValidation(date);
SimpleDateFormat formatter = new SimpleDateFormat(format);
Date formattedDate = parseDate(date, format, formatter);

Choose a reason for hiding this comment

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

Perhaps you could abstract this out into a separate method, especially seeing how its quite similar to the code below.

Comment on lines 77 to 128
private static void transactionDecoder(File file, ArrayList<Transaction> transactions)
throws FileNotFoundException, ParseException, NuscentsException {
Scanner data = new Scanner(file);
while (data.hasNext()) {
String transactionDetails = data.nextLine();
char transactionType = transactionDetails.charAt(0);
String[] columns;
float amount;
Date date;
String description = "";
String note = "";
String category= "";
SimpleDateFormat formatter = new SimpleDateFormat("dd MMMM, yyyy");
switch (transactionType) {
case 'A':
columns = transactionDetails.split("\\s*\\|\\s*");
amount = Float.parseFloat(columns[1]);
date = formatter.parse(columns[2]);
description = columns[3];
note = "";
if (columns.length > 4) {
note = columns[4];
}
AllowanceCategory allowanceCategory = null;
if (columns.length > 5) {
category = columns[5];
allowanceCategory = parseAllowanceCategory(category);
}
transactions.add(new Allowance(amount, date, description, note, allowanceCategory));
break;

case 'E':
columns = transactionDetails.split("\\s*\\|\\s*");
amount = Float.parseFloat(columns[1]);
date = formatter.parse(columns[2]);
description = columns[3];
if (columns.length > 4) {
note = columns[4];
}
ExpenseCategory expenseCategory = null;
if (columns.length > 5) {
category = columns[5];
expenseCategory = parseExpenseCategory(category);
}
transactions.add(new Expense(amount, date, description, note, expenseCategory));
break;

default:
break;
}
}
}

Choose a reason for hiding this comment

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

This method is a bit length, consider if you can use more abstraction to make it cleaner and shorter!

ui.showException(e);
} catch (IOException | ParseException e) {
Ui.showLine();
System.out.println("Something went wrong: " + e.getMessage());

Choose a reason for hiding this comment

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

Perhaps printing of error message here can also go into your UI class for good SLAP.


if (isFound) {
Ui.showFilterMessage(filteredTransactions, category);
System.out.println("Filtered transactions in the category " + category + ":");

Choose a reason for hiding this comment

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

Perhaps your system output here can be moved to the UI class? This ensures each class has follows Single Responsibility Principle e.g. printing to the console is handled solely by the UI class.

@@ -0,0 +1,43 @@
package seedu.nuscents.ui;

public class Messages {

Choose a reason for hiding this comment

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

Good use of a class to store your constant messages to be printed

Copy link

@yingx9 yingx9 left a comment

Choose a reason for hiding this comment

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

Overall a nice DG with great and understandable sequence diagrams, only minor formatting mistakes

Comment on lines +155 to +156
| Version | As a ... | I want to ... | So that I can ... |
|---------|--------------------|-----------------------------------------------------------|----------------------------------------------------------|
Copy link

Choose a reason for hiding this comment

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

image

I think your table formatting is broken in the github.io site


The following sequence diagram shows how the add transaction operation works:

<img src="images/AddTransactionSequenceDiagram.png" width="600" />
Copy link

Choose a reason for hiding this comment

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

image

Maybe you can omit the classes and User at the bottom because it might not be necessary to repeat them again? Same for the other sequence diagrams

Comment on lines 83 to 84
The following sequence diagram shows how the add transaction operation works:
<img src="images/ViewSequenceDiagram.png" width="600" />
Copy link

Choose a reason for hiding this comment

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

The sentence says "how the add transaction operation works" but the sequence diagram is about the ViewCommand. Maybe that's an error?

image

Comment on lines 53 to 63
1. Parser

The Parser class identifies the "view" command and extracts the taskIndex (transaction index) from the user's input.

2. ViewCommand

The ViewCommand object is created by the Parser. It encapsulates the user's request to view a specific transaction. This object is passed to the Nuscents class for execution.

3. Nuscents

In the Nuscents class, the execute() method of the ViewCommand is called, and the taskIndex is extracted from the command. It then calls the viewTransaction(taskIndex) method on the TransactionList.
Copy link

Choose a reason for hiding this comment

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

image

The numbering is broken in the github.io site


The following sequence diagram shows how the add transaction operation works:

<img src="images/AddTransactionSequenceDiagram.png" width="600" />

Choose a reason for hiding this comment

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

image

I think it is bit hard to read the words in the red boxes...


The following sequence diagram shows how the add transaction operation works:

<img src="images/AddTransactionSequenceDiagram.png" width="600" />

Choose a reason for hiding this comment

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

Is the classes at the bottom required? I think it makes it confusing

and modifications.

The following sequence diagram shows how the add transaction operation works:
<img src="images/ViewSequenceDiagram.png" width="600" />

Choose a reason for hiding this comment

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

image

Is a new ViewCommand created each time parseCommand is called?


The following sequence diagram shows how the add transaction operation works:

<img src="images/AddTransactionSequenceDiagram.png" width="600" />

Choose a reason for hiding this comment

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

so when a parseCommand is called, a new addCommand and expense class is created? Should it not have the constructor method?


Given below is the example usage scenario and how the list transaction mechanism behaves at each step.

Step 1. The user launches the application. The `TransactionList` will be initialized with the transactions stored in

Choose a reason for hiding this comment

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

Would it be good to extract this file storage and retrieval out to a section on its own to explain how it works?


### `helpCommand` Usage Scenario

1. **Step 1**: The user launches the application. The initial screen appears.

Choose a reason for hiding this comment

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

Not sure if "1. Step 1" is necessary. would using "Step 1" suffice?

|--------|----------|---------------|------------------|
|v1.0|new user|see usage instructions|refer to them when I forget how to use the application|
|v2.0|user|find a to-do item by name|locate a to-do without having to go through the entire list|
Version | As a ... | I want to ... | So that I can ...

Choose a reason for hiding this comment

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

The formatting for the User Stories seems to be broken on the page

Brian030601 pushed a commit to Brian030601/tp that referenced this pull request Nov 13, 2023
lckjosh and others added 28 commits November 14, 2023 00:42
Include more class diagrams
Update Implementation: list transactions feature
Fix github pages formatting issue
Fix format in DG list implementation steps, add instructions for manual testing
bug fix: update links in README.md
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.