Skip to content

Conversation

@Yshinprograms
Copy link

Enable first-year university undergraduates living on-campus to confidently manage their finances, so they can reduce financial stress and fully enjoy their university experience.

Yshinprograms and others added 29 commits March 30, 2025 15:53
Moved YS' PPP into docs/team
Updated UG,DG, and added Jielei's PPP
Copy link

@okkhoy okkhoy left a comment

Choose a reason for hiding this comment

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

Generally, the code is well written.
There are many redundant comments in the code; please consider removing them.
The number of assertions and logging in the codebase is not as much as I would like. The distribution is also not uniform across the codebase.
There is some scope to improve SLAP in the code. Pl. check the individual comment to get an idea.

}

static {
configureLogger(); // Configure the logger when the class is loaded
Copy link

Choose a reason for hiding this comment

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

redundant comment; naming is pretty good to understand what it is

try {
incomeCommand.incomeExecute(new IncomeManager(), ui);
} catch (BudgetTrackerException e) {
// Handle the exception by printing the message
Copy link

Choose a reason for hiding this comment

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

remove all such redundant comments

Comment on lines 9 to 10
//substring functions (substring(17)/substring(20), etc.) are used to trim the input
//to only keep the amount/index/goal description part
Copy link

Choose a reason for hiding this comment

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

these look like "note-to-self" comments. should be removed

Comment on lines 114 to 120
} else {
System.out.println("Here are the savings in your list:");
for (int i = 0; i < savingsRecords.size(); i++) {
System.out.println((i + 1) + ". \t" + savingsRecords.get(i));
}
System.out.println("You have " + savingsRecords.size() + " saving(s) in total.");
}
Copy link

Choose a reason for hiding this comment

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

can consider using the if condition as a guard clause and remove the nesting.

* Runs the savings management system, processing user commands.
* @param input The Scanner object for user input.
*/
public void run(String input) {
Copy link

Choose a reason for hiding this comment

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

you can extract individual processing to different methods and improve the SLAP in this method.

/**
* Runs the main program loop, processing user commands until exit.
*/
public void runDuke() {
Copy link

Choose a reason for hiding this comment

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

what is this VS the other run method I saw earlier in the code here?

Comment on lines 125 to 129
fundsAlert.update(15.0, 100.0, 85.0, 0.0);
String output3 = outputStream.toString();
assertFalse(output3.contains("WARNING"),
"No alert should be triggered when threshold is lowered below the available funds");
}
Copy link

Choose a reason for hiding this comment

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

you can move some of these into @before and @after methods if you wish to keep the tests clean and remove somewhat repetitive code

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