Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
    
Well, if you need to explain the meaning of the variable, I guess you could consider renaming the variables. –  Marc-Andre Feb 11 at 15:47
    
I would, but that is the standard for the business terminology –  Jason Bayldon Feb 11 at 15:54
2  
The first thing I'd address is the cryptic Mode parameter. It's dying to be an Enum with real descriptive names instead of magic Integer values. And EffDate would look much cooler as FxDate, but that's just me :) –  Mat's Mug Feb 11 at 19:45
1  
A better name for Mode could be TimeSpanType, with values like Yearly, Quarterly, Monthly and SemiAnnual. –  Mat's Mug Feb 11 at 19:48
add comment

3 Answers 3

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.

share|improve this answer
    
Thank you, your write up was very good. I was able to stick with it the whole way. The function was UDF in access which explains the dateadd() and vb6 likeness . Why is it better to use a variable such as 'result'? Thank you for taking the time –  Jason Bayldon Feb 11 at 21:19
    
Because when you're reassigning EffDate, you're changing its meaning - it's no longer the parameter value you received from the caller, it's something else, and that makes it confusing code, because as you scroll down the code the meaning of the identifier changes and you have to mentally keep up with it (I've seen worse than this though). As for the naming itself, it could be personal preference, but I find an identifier named result is very explicit about its meaning. –  Mat's Mug Feb 11 at 21:27
    
I really like the complete process from Mode to the final Enum. This help having an incremental process to the final solution, it's more easily to reproduce. –  Marc-Andre Feb 12 at 16:10
add comment

You're doing a lot more looping than necessary. It would probably be more efficient to jump the return date to the same year than adjust the month accordingly. As was mentioned an enum, properly named makes the code much more readable and manitainable. Something like this:

Public Function FPDA(ByVal EffDate As Date, ByVal DOB As Date, ByVal PaymentMode As PaymentInterval, ByVal Age As Integer) As Date
    'FUNCTION TO CALCULATE THE FIRST PAYMENT DUE AFTER SPECIFIED AGE.
    'Find the year the payments is to be made
    Dim TargetDate = DOB.AddYears(Age)
    'Find the payment start day for the year in question
    FPDA = EffDate.AddYears(TargetDate.Year - EffDate.Year)
    'These conditionals adjust the return value according to 
    'whether the date is before or after the birthdate and whether 
    'the interval is bigger than the payment interval
    If FPDA < TargetDate Then
        Do Until FPDA >= TargetDate
            FPDA = FPDA.AddMonths(PaymentMode)
        Loop
    ElseIf FPDA > TargetDate Then
        While FPDA.Month - TargetDate.Month >= PaymentMode
            FPDA = FPDA.AddMonths(PaymentMode * -1)
        End While
    End If
    If FPDA.Month <> 2 AndAlso FPDA.Day <> EffDate.Day Then
        FPDA = New Date(FPDA.Year, FPDA.Month, EffDate.Day)
    End If
End Function

Enum PaymentInterval
    Yearly = 12
    Semi_Annually = 6
    Quaterly = 3
    Monthly = 1
End Enum

This will only loop in specific cases and then only for a few iterations.

This assumes that the payment day is the same as the EffDate day

I tried to test this with as many different scenarios as I could. If I missed something let me know.

If you would like simplicity over performance your original code could be optimized like this:

Public Function FPDA2(ByVal EffDate As Date, ByVal DOB As Date, ByVal PaymentMode As PaymentInterval, ByVal Age As Integer) As Date
    FPDA2 = EffDate
    Do Until FPDA2 >= DOB.AddYears(Age)
        FPDA2 = FPDA2.AddMonths(PaymentMode)
    Loop
    If FPDA2.Month <> 2 AndAlso FPDA2.Day <> EffDate.Day Then
        FPDA2 = New Date(FPDA2.Year, FPDA2.Month, EffDate.Day)
    End If
End Function

Both achieve the exact same result, but one is more complicated and more efficient and the other is very simple but inefficient

share|improve this answer
    
Thanks for your help. –  Jason Bayldon Feb 11 at 21:21
add comment

Since the previous posts covered most of the structural issues (VB6 data structures v .NET, enumerating the payment modes, etc.), I'll address the remaining elephant in the room here. Looping through dates like this is similar to counting on your fingers to find out how many fingers you have on your hands. Nobody would even consider (or maybe "should" is the better word) doing this with a numeric type:

Dim Fingers As Integer = 1;

Do Until Fingers > 9
    Fingers = Fingers + 1
Loop

Return Fingers

Much better to just calculate the result, especially if this is being looped over in the calling code. You tend to see code like the sample you posted when people are afraid to work with dates. The calculation involved isn't that hard. You first figure out the payee's birthday for the given age and find the following [insert financial vehicle here] anniversary based on the effective date. This lets you calculate the number of months until the next anniversary. Taking the modulus of the number of months until the anniversary and the number of months between payments gives you the month that the next payment is due. The only reason to work with anything other than the month is to cover cases where the birthday falls on a modal cycle:

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

    Dim ModeFrequency As Integer = 12 / Mode
    Dim BirthDay As DateTime = DOB.AddYears(Age)
    Dim TargetMonth As EffDate.Month

    If BirthDay.Month > TargetMonth Then
        TargetMonth = TargetMonth + 12
    End If

    Dim Offset As Integer = ((TargetMonth - DOB.Month) Mod ModeFrequency) + (BirthDay.Month - EffDate.Month)
    DateTime NextPayment = new DateTime(BirthDay.Year, EffDate.Month, EffDate.Day).AddMonths(Offset)

    'Test for DOB landing in payment month, but before or on the birthday.
    If NextPayment <= BirthDay
        NextPayment = NextPayment.AddMonths(ModeFrequency)
    End If

    FPDA = NextPayment

End Function

Forgive any syntax errors - my VB is rusty. If you can't change the calling convention and have to work with VB6 types, there are similar date handling functions for the VB6 Date type to extract the year, month, and day. The logic would be the same though. The code above can be stream-lined a bit, TargetMonth and ModeFrequency are declared mainly for the sake of clarity.

One final note (and this is more something to be aware of as opposed to something I would change) is that the code is relying on how the language handles dates to enforce a business rule. In this case it is when the payment is due if the effective date has more days of the month than the due date. The original code (and the sample above) both assume that the due date reverts to the last day of the month (behavior of .AddMonths() and DateAdd()). Some companies assume the first of the next month. Some companies like the one I currently work for won't assign an issue date after the 28th.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.