HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Calculate a payment due after X age

Submitted by: @import:stackexchange-codereview··
0
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:

Dim result = FPDA("#1/12/1952#", "9/29/1948", 1, 55)
    MsgBox(result) 'in this example, I am returning first payment due after age 55


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:

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 Function


Is 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:

Public Function FPDA(ByVal EffDate As Date, ByVal DOB As Date, ByVal Mode As Integer, ByVal Age As Integer) As Date


You'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 Enum


So 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 EffDate


It'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 = EffDate


And change the last line to:

Return result


Now 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 Select


And then you can just do:

Do Until result > DateAdd(interval, Age, [DOB])
    result = DateAdd(interval, 1, result)
Loop


But 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 Select


Or 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 Enum


TL;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)
Loop


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 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 Date
Public Enum TimeSpanType
    Year
    HalfYear
    Quarter
    Month
End Enum
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 EffDate
Dim result As Date
result = EffDate
Return result

Context

StackExchange Code Review Q#41410, answer score: 7

Revisions (0)

No revisions yet.