Conversation
This comment has been minimized.
This comment has been minimized.
mo92othman
left a comment
mo92othman
left a comment
There was a problem hiding this comment.
Hey @Barboud! Overall, good work 👍
The logic is clear, and the application works as expected. I have left some small comments.
One small setup note: project configuration files (package.json, .gitignore``, Prettier config) are required to be placed inside the project folder itself ( finance-tracker) as the assignments ask for that. Then you needed to create `.gitignore` for `node_modules/`
| export function addTransaction(transaction) { | ||
| if (!transaction[0].id) { | ||
| transaction[0].id = transactions.length + 1; | ||
| } | ||
| transactions.push(...transaction); | ||
| return transactions; | ||
| } |
There was a problem hiding this comment.
I see this function expects an array of transactions, but the assignment goal is to add one transaction at a time.
What you wrote is correct, but since the function (by its name) adds a single transaction, we could simplify it to accept one transaction object instead.
finance-tracker/finance.js
Outdated
| export function getLargestExpense() { | ||
| const filteredTransactions = getTransactionsByType('expense'); | ||
| let largestTransaction = []; | ||
| let largestExpense = null; | ||
| for (const filteredTransaction of filteredTransactions) { | ||
| if (filteredTransaction.amount > largestExpense) { | ||
| largestExpense = filteredTransaction.amount; | ||
| largestTransaction.pop(filteredTransaction); | ||
| largestTransaction.push(filteredTransaction); | ||
| } | ||
| } | ||
| return largestTransaction; |
There was a problem hiding this comment.
The goal of this function is to find one largest expense, but the result is stored inside an array.
But we don't need to store it into an array, because you did that, you needed extra logic (push / pop).
Keeping a single transaction object for the largest expense would be enough. For example:
export function getLargestExpense() {
let largest = null;
for (const transaction of transactions) {
if (transaction.type === 'expense') {
if (largest === null || transaction.amount > largest.amount) {
largest = transaction;
}
}
}
return largest;
}There was a problem hiding this comment.
My problem in this task was confusing an array with an object.
I was trying to evaluate the data structure in data.js, and I ignored the role of each function.
Thanks for the explanation.
| export function getTotalIncome() { | ||
| let totalIncome = 0; | ||
| for (const transaction of transactions) { | ||
| if (transaction.type === 'income') { | ||
| totalIncome += transaction.amount; | ||
| } | ||
| } | ||
| return totalIncome; | ||
| } |
| addTransaction, | ||
| getTotalIncome, | ||
| getTotalExpenses, | ||
| getBalance, | ||
| getTransactionsByCategory, | ||
| getLargestExpense, | ||
| printAllTransactions, | ||
| printSummary, | ||
| getTransactionsByType, | ||
| getTotalTransactions, | ||
| } from './finance.js'; |
There was a problem hiding this comment.
Small suggestion 🙂
Some imported functions are not used in this file.
It’s best practice to remove unused imports to keep the code clean and easier to read.
mo92othman
added
Reviewed
📝 HackYourFuture auto gradeAssignment Score: 0 / 100 ✅Status: ✅ Passed Test Details |
2 participants
No description provided.