patternMinor
Calculate a payment due after X age
Viewed 0 times
afterdueagepaymentcalculate
Problem
I have inherited a function which calculates a first payment due after a certain age.
Using the function I typically pass in database values to custom calculate certain dates:
As far as the parameters, EffDate is the date the account became active, DOB is date of birth, mode is a payment mode value and age is the attained age to check. The function looks like this:
Is there a better or more efficient way than to achieve this?
Using the function I typically pass in database values to custom calculate certain dates:
Dim result = FPDA("#1/12/1952#", "9/29/1948", 1, 55)
MsgBox(result) 'in this example, I am returning first payment due after age 55As far as the parameters, EffDate is the date the account became active, DOB is date of birth, mode is a payment mode value and age is the attained age to check. The function looks like this:
Public Function FPDA(ByVal EffDate As Date, ByVal DOB As Date, ByVal Mode As Integer, ByVal Age As Integer) As Date
'FUNCTION TO CALCULATE THE FIRST PAYMENT DUE AFTER SPECIFIED AGE.
Select Case Mode
Case "1"
Do Until EffDate > DateAdd("yyyy", Age, [DOB])
EffDate = DateAdd("yyyy", 1, EffDate)
Loop
Case "2"
Do Until EffDate > DateAdd("yyyy", Age, [DOB])
EffDate = DateAdd("m", 6, EffDate)
Loop
Case "4"
Do Until EffDate > DateAdd("yyyy", Age, [DOB])
EffDate = DateAdd("q", 1, EffDate)
Loop
Case "12"
Do Until EffDate > DateAdd("yyyy", Age, [DOB])
EffDate = DateAdd("m", 1, EffDate)
Loop
End Select
Return EffDate
End FunctionIs there a better or more efficient way than to achieve this?
Solution
This is going to be a brain dump -type review. Feel free to jump straight to TL;DR if my thinking out loud gets annoying :)
Let's start with the function's signature:
You're taking all parameters
I can live with
Looking at the body of the function I think what
So we can have a body like this:
It's more readable and less error-prone, but it's doing the exact same thing. It gives another meaning to the received parameter
I would make a copy of
And change the last line to:
Now every branch looks like a Copy+Paste party. How about this:
And then you can just do:
But this is rather ugly.
Or better yet - get rid of the
TL;DR
And then you can just do this:
And now that you've got code that's easy to read, follow and maintain, you can start thinking about how the loop could be simplified.
Strive to get rid of
Let's start with the function's signature:
Public Function FPDA(ByVal EffDate As Date, ByVal DOB As Date, ByVal Mode As Integer, ByVal Age As Integer) As DateYou're taking all parameters
ByVal, which is good - it means the function cannot alter these values if it tried, it's receiving a copy of what the caller gave it.I can live with
EffDate, DOB and Age identifiers. But as I've mentioned in the comments, Mode is a no-no, especially if it's a magic Integer variable.Looking at the body of the function I think what
Mode is really about is something more like this:Public Enum TimeSpanType
Year
HalfYear
Quarter
Month
End EnumSo we can have a body like this:
Select Case Mode
Case TimeSpanType.Year
Do Until EffDate > DateAdd("yyyy", Age, [DOB])
EffDate = DateAdd("yyyy", 1, EffDate)
Loop
Case TimeSpanType.HalfYear
Do Until EffDate > DateAdd("yyyy", Age, [DOB])
EffDate = DateAdd("m", 6, EffDate)
Loop
Case TimeSpanType.Quarter
Do Until EffDate > DateAdd("yyyy", Age, [DOB])
EffDate = DateAdd("q", 1, EffDate)
Loop
Case TimeSpanType.Month
Do Until EffDate > DateAdd("yyyy", Age, [DOB])
EffDate = DateAdd("m", 1, EffDate)
Loop
End Select
Return EffDateIt's more readable and less error-prone, but it's doing the exact same thing. It gives another meaning to the received parameter
EffDate, which might work now, but it's error-prone, and this is where ByVal is saving you.I would make a copy of
EffDate:Dim result As Date
result = EffDateAnd change the last line to:
Return resultNow every branch looks like a Copy+Paste party. How about this:
Dim interval As String
Select Case Mode
Case TimeSpanType.Year
interval = "yyyy"
Case TimeSpanType.HalfYear
interval = "m"
Case TimeSpanType.Quarter
interval = "q"
Case TimeSpanType.Month
interval = "m"
End SelectAnd then you can just do:
Do Until result > DateAdd(interval, Age, [DOB])
result = DateAdd(interval, 1, result)
LoopBut this is rather ugly.
DateAdd lives in the Microsoft.VisualBasic namespace, which only exists to make VB6 developers happy with transition to the Wonderful World of .NET... but by using that namespace, you're missing out on everything the Wonderful World of .NET has to offer!Date is just a language alias for System.DateTime, which exposes the .NET way of life - exposing methods like AddMonths, which is really all you need here:Dim monthsToAdd As Integer
Select Case Mode
Case TimeSpanType.Year
monthsToAdd = 12
Case TimeSpanType.HalfYear
monthsToAdd = 6
Case TimeSpanType.Quarter
monthsToAdd = 3
Case TimeSpanType.Month
monthsToAdd = 1
End SelectOr better yet - get rid of the
Select...Case altogether, because Enums are Integers under the hood:Public Enum TimeSpanType
Year = 12
HalfYear = 6
Quarter = 3
Month = 1
End EnumTL;DR
And then you can just do this:
Dim interval As Integer
interval = CType(Mode, Integer)
Do Until result > DOB.AddMonths(interval*Age)
result = result.AddMonths(1)
LoopAnd now that you've got code that's easy to read, follow and maintain, you can start thinking about how the loop could be simplified.
Strive to get rid of
Imports Microsoft.VisualBasic anywhere you see it. VB.NET is .NET, not VB6. Your code will only get better.Code Snippets
Public Function FPDA(ByVal EffDate As Date, ByVal DOB As Date, ByVal Mode As Integer, ByVal Age As Integer) As DatePublic Enum TimeSpanType
Year
HalfYear
Quarter
Month
End EnumSelect Case Mode
Case TimeSpanType.Year
Do Until EffDate > DateAdd("yyyy", Age, [DOB])
EffDate = DateAdd("yyyy", 1, EffDate)
Loop
Case TimeSpanType.HalfYear
Do Until EffDate > DateAdd("yyyy", Age, [DOB])
EffDate = DateAdd("m", 6, EffDate)
Loop
Case TimeSpanType.Quarter
Do Until EffDate > DateAdd("yyyy", Age, [DOB])
EffDate = DateAdd("q", 1, EffDate)
Loop
Case TimeSpanType.Month
Do Until EffDate > DateAdd("yyyy", Age, [DOB])
EffDate = DateAdd("m", 1, EffDate)
Loop
End Select
Return EffDateDim result As Date
result = EffDateReturn resultContext
StackExchange Code Review Q#41410, answer score: 7
Revisions (0)
No revisions yet.