FINERACT-1981: Rate factor using simple interest for EMI calculation #3902
Conversation
74155cb to
7a3d244
Compare
There was a problem hiding this comment.
this can be final as well, no?
There was a problem hiding this comment.
Done! updated
There was a problem hiding this comment.
I dont think we need to convert this to BigDecimal
There was a problem hiding this comment.
I believe the fnValue calculation was not part of the original PR... I am hesitant whether these methods should be moved to the next PR, or this PR to be renamed as it is not only calculating the rate factor now.
periodFrom and periodEnd sounds a little bit confusing to me. I would recommend: previous period and actual period to be used.
Also i would prefer if not the objects are provided, rather the required parameters. This way we can avoid getting object as parameter and working on the objects and returning one of the object (which in this case it is unnecessary).
Something like this:
public static BigDecimal fnValue(BigDecimal previousFnValue, BigDecimal rateFactor)
This way the method is focusing on the calculation only and not focusing on what object needs to be built.
There was a problem hiding this comment.
Done! code removed from this PR
There was a problem hiding this comment.
Why do we need to set the precision to 8? I believe the MoneyHelper MathContext is working with 12 precision and database we support 6 after the decimal point (usually). We might need to discuss whether we need something different here... i would say the MoneyHelper MathContext precision is the favoured to me.
There was a problem hiding this comment.
Done! We are using the MatchContext from MoneyHelper
There was a problem hiding this comment.
We dont need to rescale. The MathContext should contain the required precision.
There was a problem hiding this comment.
Done! code removed from this PR
There was a problem hiding this comment.
This condition is unnecessary:
rnValue = rnValue.multiply(rateFactorPeriod.getRateFactor(), mc); gives you the right value even it is the 1st period.
There was a problem hiding this comment.
Done! code removed from this PR
There was a problem hiding this comment.
I am assuming this one is mimicking when we convert it to Money object, but it would be better instead of mimicking actually convert it to Money
There was a problem hiding this comment.
Done! code removed from this PR
adamsaghy
left a comment
There was a problem hiding this comment.
Kindly see my comments!
7a3d244 to
d52fd93
Compare
There was a problem hiding this comment.
It would be better if the MathContext comes as parameter ;)
d52fd93 to
d554d62
Compare
d554d62 to
f2d58cb
Compare
Description
In order to calculate the monthly payment, we first need to calculate something called the Rate Factor. We're going to be using simple interest. The Rate Factor for simple interest is calculated by the following formula:
FINERACT-1981
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Write the commit message as per https://github.com/apache/fineract/#pull-requests
Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
Create/update unit or integration tests for verifying the changes made.
Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.