-
Notifications
You must be signed in to change notification settings - Fork 140
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-W10-1] BudgetBuddy #16
base: master
Are you sure you want to change the base?
[CS2113-W10-1] BudgetBuddy #16
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, the code quality is good. There are some minor issues that are easily fixable. Pls take a look at the comments in the review.
String categoryPart = parts[3].trim(); | ||
categoryPart = categoryPart.substring(1, categoryPart.length() - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be extracted for better SLAP
assert expenseList != null : "Expense list cannot be null"; // Assert that the expense list is not null | ||
assert incomeList != null : "Income list cannot be null"; // Assert that the income list is not null | ||
assert budgetList != null : "Budget list cannot be null"; // Assert that the budget list is not null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant comments here
|
||
FileWriter fw = new FileWriter(filePath, false); // Overwrites the file | ||
|
||
// Save expenses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant comment
remove all such redundant ones
*/ | ||
@Override | ||
public void execute() { | ||
// Assume validation has guaranteed that existingBudget is not null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think you can use assert here instead of this comment?
if (category == null && month == null) { | ||
LOGGER.log(Level.INFO, "Displaying expenses listed with no Filter"); | ||
ExpenseManager.listExpenses(); | ||
} else if (category == null){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing after the condition
List<Double> xAxis = new ArrayList<>(); | ||
List<Double> yAxis = new ArrayList<>(); | ||
|
||
// Loop through each month of the year |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant comment again
counter++; | ||
} | ||
} | ||
if(result.equals("")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing issues
if (IncomeManager.getIncomes().size() > 0){ | ||
for (Income income: IncomeManager.getIncomes()){ | ||
savings += income.getAmount(); | ||
YearMonth incomeYearMonth = IncomeManager.getYearMonthFromDate(income.getDate()); | ||
if (incomeYearMonth.compareTo(firstIncome) == -1){ | ||
firstIncome = incomeYearMonth; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks deeply nested. can you try to use some guard clause to eliminate the nesting if possible?
Added functions & dependencies for EditIncome feature Added JavaDoc Comments to EditIncome feature Added Automated text-ui-tests for EditIncome feature
Add Edit Income Function
Update DG for Add Budget Feature
- Modified sequence diagram of Add Budget feature
Update details in Deduct Budget feature for DG
Fix auto-correction of invalid dates
Users need a clear way to visualise their Expenses for a month in their respective categories. Add a function with XChart that displays a PieChart which is colour- coded and shows percentage of Expenses. The PieChart provide enough information to allow users to make informed decisions about their expenses.
Add Function to Display PieChart of Expense
-Add addExpense sequence diagram -Add listRemainingBudget sequence diagram -Add expense, income and transaction class diagram
-add expense -delete expense -add income -delete income -list remaining budget
Update UG and DG
…into branch-DeveloperGuide # Conflicts: # docs/DeveloperGuide.md
…BugFix Revert "Fix Bugs and Update UG"
…into branch-updatePPP
Update PPP and UG
…into branch-PE-D-Issues
Update Naming Convention for Edit feature
Update Home Page
Fix naming convention in UG
Fix naming convention in DG Manual Testing
Branch add s in search expense
…into branch-updatePPP # Conflicts: # src/main/java/seedu/budgetbuddy/Ui.java
Branch update help command
In reviewing the class diagram, would it be beneficial to disclose parameters and methods at the class level for the Expense and Income classes? While abstraction simplifies the architectural overview, could the inclusion of specific details regarding how these classes interact through shared parameters and method invocations enhance understanding and facilitate system integration? Regarding the non-functional requirements, should the dependency on Java 17 be classified as a functional requirement, given that the operability of the project hinges on this version? Does this classification reflect the necessity of this requirement for the software’s operation, distinguishing it from typical non-functional requirements that address system qualities? In reference to section 5.5.2.1.2 of the document, is it necessary to maintain the second test case under the 'Test Cases' heading if it does not provide additional verification value or distinct scenario coverage? Would its exclusion potentially streamline the testing process and enhance focus on more critical tests? In reviewing the documentation further, I noticed that the term "Mainstream" is used only once to categorize compatible operating systems. Given its limited use, would it not be more direct and clear to list the compatible operating systems explicitly within the main document? This approach would enhance accessibility and eliminate the need for referencing the Glossary for a single mention, streamlining the document significantly. On the other hand, the term "Transactions" is effectively used to describe the interactions between financial classes such as Budget, Expense, and Income. This term accurately depicts the financial exchanges or data manipulations that occur within the system, offering a precise illustration of the relationships among these entities. Hence, the use of "Transactions" is appropriate and should be retained as it clearly conveys the intended processes and relationships within the project's framework. |
BudgetBuddy is an easy-to-use expense tracking and budgeting tool that helps students stay organised and understand their spending habits. It is optimised for CLI users, allowing faster expense entry by typing in commands.