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

Added Rounded #s for Debt Payments #6

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

Conversation

alloylab
Copy link
Contributor

added round_up & round_down to mathfuncs,
added debtSingleRepayment_Rounded & debtRepayments_Rounded to
DebtAmortizator… added beginning and ending balance to test round math
worked out. Decided to keep since I think it would be nice have in
there. Rounding logic was decided on when testing against a couple
official amortization schedules

added round_up & round_down to mathfuncs,
added debtSingleRepayment_Rounded & debtRepayments_Rounded to
DebtAmortizator… added beginning and ending balance to test round math
worked out. Decided to keep since I think it would be nice have in
there. Rounding logic was decided on when testing against a couple
official amortization schedules
@uruba
Copy link
Owner

uruba commented Mar 22, 2016

Awesome, thanks for your contribution. I'd however like to discuss some points with you before merging this, so that the code is consistent:

  • the naming of the methods/variables – I have used exclusively camel case all throughout the project so, for the sake of consistency, it would be best to stick to it also here (although I might agree that it may disrupt the clarity with the long names [some of which may be too difficult to distinguish at a first glance], it's a project-wide problem and as such it should also be tackled project-wide [which is something we may further discuss; if you want, you can definitely tell me what do you think would be the best according to you])
  • indentation – should be 4 spaces every level
  • unit tests – a new test method/assertion should be added for every new feature so that it can be assessed that it is working properly at any given project state (the unit test file for these classes are DebtAmortizatorTest.php and MathFuncsTest.php)

You definitely don't have to make any changes if you don't want to, I can do this instead as an additional commit. Also discussion is welcome if you have any remarks on this.

addressed issues uruba stated in last push
@alloylab
Copy link
Contributor Author

Sorry for the delay been swamped... i think i addressed most your concerns here... if I missed anything please let me know I will fix them

@uruba
Copy link
Owner

uruba commented Aug 1, 2016

Hello, sorry for my late reply, thanks for you contribution, but I see that the tests are failing (it seems like something in the class DebtAmortizator, from the looks of the output on Travis). Please have a look at it if you can.

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.

None yet

2 participants