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

feat: add second OOP lab #585

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ihechikara
Copy link
Member

Checklist:

Closes #470

Copy link
Contributor

@jdwilkin4 jdwilkin4 left a comment

Choose a reason for hiding this comment

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

Hey @ihechikara !

I think the project itself works well but the user stories overall are to strict and step by step.

Remember the goal of the labs is for campers to build things on their own and think through the problems. That is the only way you grow and learn as a developer.

If everything is prescribed to them in a step by step fashion, then they don't really need to think through what to do because they are just going through the motions of a tutorial.

I left some comments on where we should generalize things to make this more open ended so it matches what is currently merged into main.

@@ -0,0 +1,38 @@
1. You should define a class named `BankAccount` with a `constructor` that has a `balance` parameter (default value of 0). The constructor should also initialize an empty `transactions` array to store transaction records.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. You should define a class named `BankAccount` with a `constructor` that has a `balance` parameter (default value of 0). The constructor should also initialize an empty `transactions` array to store transaction records.
1. You should define a class named `BankAccount` with a `constructor` that has a `balance` parameter. The default parameter value should be set to `0`. The constructor should also initialize an empty `transactions` array to store transaction records.

Comment on lines +4 to +8
- Check if the `amount` is greater than 0.
- If the amount is valid, push an object with the properties `type: 'deposit'` and the `amount` into the `transactions ` array.
- Call the `updateBalance` method to update the account's balance.
- Log a message to the console indicating the deposit was successful and display the new balance.
- If the `amount` is invalid, log an error message indicating that the deposit amount must be greater than zero.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is too step by step and doesn't allow the camper to think through what needs to be done.
this should be more generalized so the camper is forced to think about how to solve the problem

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it will be much easier from a testing perspective to return the string instead of logging it to the console.

Comment on lines +11 to +15
- Check if the `amount` is greater than 0 and less than or equal to the current `balance`.
- If valid, push an object with the properties `type: 'withdraw'` and the `amount` into the `transactions` array.
- Call the `updateBalance` method to update the account's balance.
- Log a message to the console indicating the withdrawal was successful and display the new balance.
- If the `amount` is invalid or exceeds the balance, log an error message indicating insufficient balance or invalid amount.
Copy link
Contributor

Choose a reason for hiding this comment

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

same issue here.
This should be more generalized to have camper think more about what needs to be done and not so step by step.

- Log a message to the console indicating the withdrawal was successful and display the new balance.
- If the `amount` is invalid or exceeds the balance, log an error message indicating insufficient balance or invalid amount.

4. You should define a method named `checkBalance` that logs the current account balance to the console.
Copy link
Contributor

Choose a reason for hiding this comment

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

we should just have them return the string instead of logging it.
it will be much easier to test that way

Comment on lines +20 to +21
- Use the `reduce` function to sum the transactions in the `transactions` array.
- Update the `balance` by adding up all the deposit amounts and subtracting all the withdrawal amounts.
Copy link
Contributor

Choose a reason for hiding this comment

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

this one too should be more generalized.
we should allow for multiple valid answers but right now we are only asking them to user reduce but there are multiple ways to solve this

Comment on lines +24 to +26
- Use the `filter` function to create a new array containing only the transactions with `type: 'deposit'`.
- Use the `map` function to extract the `amount` from each deposit transaction.
- Log the list of deposit amounts to the console and return this list.
Copy link
Contributor

Choose a reason for hiding this comment

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

we should be more general here to and say something like "Return an array containing the amounts from each deposit transaction."

also, we should remove the log statement here.

Comment on lines +29 to +31
- Use the `filter` function to create a new array containing only the transactions with `type: 'withdraw'`.
- Use the `map` function to extract the `amount` from each withdrawal transaction.
- Log the list of withdrawal amounts to the console and return this list.
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be updated to something like "Return an array containing the amounts from each withdrawal transaction."

also, we should remove the log statement here.

Comment on lines +33 to +38
8. You should create an instance of the BankAccount class named myAccount. Then, you should:
- Use the `deposit` method to deposit into the account.
- Use the `withdraw` method to withdraw from the account.
- Use the `checkBalance` method to log the current balance to the console.
- Use the `listAllDeposits` method to list all deposits made.
- Use the `listAllWithdrawals` method to list all withdrawals made.
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, I don't think we really need this last part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting update Frontend Cert Labs Team This work is for the labs team only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: add second lab for OOP in frontend cert
3 participants