9
\$\begingroup\$

I need to generate a T-SQL script for loading data into this table:

create table dwd.FiscalCalendars (
     _Id int not null identity(1,1)
    ,_DateInserted datetime not null
    ,_DateUpdated datetime null
    ,CalendarDate date not null
    ,CalendarDayOfWeek int not null
    ,CalendarDayOfMonth int not null
    ,CalendarDayOfYear int not null
    ,CalendarWeekOfYear int not null
    ,CalendarMonthOfYear int not null
    ,CalendarYear int not null
    ,FiscalDayOfWeek int not null
    ,FiscalDayOfMonth int not null
    ,FiscalDayOfQuarter int not null
    ,FiscalDayOfYear int not null
    ,FiscalWeekOfMonth int not null
    ,FiscalWeekOfQuarter int not null
    ,FiscalWeekOfYear int not null
    ,FiscalMonthOfQuarter int not null
    ,FiscalMonthOfYear int not null
    ,FiscalQuarterOfYear int not null
    ,FiscalYear int not null
    ,Holiday nvarchar(50) null
    ,constraint PK_FiscalCalendars primary key clustered (_Id asc)
    ,constraint NK_FiscalCalendars unique (CalendarDate)
);

In order to generate the script, I first need to generate each record. I decided to write some VBA code to do that. Here's the function I came up with - I'll make another to work backwards, so as to generate records for previous fiscal years for which I don't know the start date - hence I called this one "GenerateForwardCalendar":

Private Function GenerateForwardCalendar(ByVal startDate As Date, ByVal fStartYear As Integer, ByVal years As Integer) As Collection

    Dim result As New Collection

    Dim current As FiscalCalendarDate
    Dim currentDate As Date
    currentDate = startDate

    Dim fYear As Integer
    Dim fQuarterOfYear As Integer
    Dim fMonthOfQuarter As Integer
    Dim fWeekOfMonth As Integer
    Dim fDayOfWeek As Integer

    For fYear = fStartYear To fStartYear + years
        Set current = FiscalCalendarDate.Create(currentDate, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, fYear, vbNullString)

        For fQuarterOfYear = 1 To 4

            current.FiscalDayOfQuarter = 1
            current.FiscalWeekOfQuarter = 1
            current.FiscalMonthOfQuarter = 1

            For fMonthOfQuarter = 1 To 3

                current.FiscalDayOfMonth = 1
                current.FiscalWeekOfMonth = 1

                If IIf(IsLeapYear(current.calendarYear) And current.FiscalMonthOfYear = 12, True, fMonthOfQuarter Mod 2 = 0) Then

                    For fWeekOfMonth = 1 To 5
                        GoSub ProcessWeekDays
                    Next

                Else

                    For fWeekOfMonth = 1 To 4
                        GoSub ProcessWeekDays
                    Next

                End If

                current.FiscalMonthOfQuarter = current.FiscalMonthOfQuarter + 1
                current.FiscalMonthOfYear = current.FiscalMonthOfYear + 1

            Next

            current.FiscalQuarterOfYear = current.FiscalQuarterOfYear + 1

        Next
    Next

CleanExit:
    Set GenerateForwardCalendar = result
    Exit Function

ProcessWeekDays:

    For fDayOfWeek = 1 To 7
        current.FiscalDayOfWeek = fDayOfWeek

        Dim item As FiscalCalendarDate
        Set item = FiscalCalendarDate.Copy(current)
        Debug.Print item.ToString
        result.Add item

        currentDate = currentDate + 1
        current.CalendarDate = currentDate
        current.FiscalDayOfWeek = current.FiscalDayOfWeek + 1
        current.FiscalDayOfMonth = current.FiscalDayOfMonth + 1
        current.FiscalDayOfQuarter = current.FiscalDayOfQuarter + 1
        current.FiscalDayOfYear = current.FiscalDayOfYear + 1
    Next

    current.FiscalWeekOfMonth = current.FiscalWeekOfMonth + 1
    current.FiscalWeekOfQuarter = current.FiscalWeekOfQuarter + 1
    current.FiscalWeekOfYear = current.FiscalWeekOfYear + 1

    Return

End Function

Here's the IsLeapYear function I'm using, for the leap years shall have a 5th week in the last month of the 4th quarter, making them 53-week years:

Public Function IsLeapYear(ByVal calendarYear As Integer) As Boolean
    IsLeapYear = (Month(DateSerial(calendarYear, 2, 29)) = 2)
End Function

So, this gives me a collection of FiscalCalendarDate objects, which I will be iterating later, and passing to this function:

Private Function GenerateInsertStatement(ByVal value As FiscalCalendarDate) As String
    Dim result As String
    With value
    result = Framework.Strings.Format("INSERT INTO dwd.FiscalCalendars (_DateInserted,CalendarDate,CalendarDayOfWeek,CalendarDayOfMonth,CalendarDayOfYear,CalendarWeekOfMonth,CalendarWeekOfYear,CalendarMonthOfYear,CalendarYear," & _
                                      "FiscalDayOfWeek,FiscalDayOfMonth,FiscalDayOfQuarter,FiscalDayOfYear,FiscalWeekOfMonth,FiscalWeekOfQuarter,FiscalWeekOfYear,FiscalMonthOfQuarter,FiscalMonthOfYear,FiscalQuarterOfYear,FiscalYear,Holiday) ", _
                                      "VALUES (@ts,'{0}',{1},{2},{3},{4},{5},{6},{7},{8},{9},{10},{11},{12},{13},{14},{15},{16},{17},{18},'{19}');", _
                                    .CalendarDate, .CalendarDayOfWeek, .CalendarDayOfMonth, .CalendarDayOfYear, _
                                    .CalendarWeekOfMonth, .CalendarWeekOfYear, .CalendarMonthOfYear, .calendarYear, _
                                    .FiscalDayOfWeek, .FiscalDayOfMonth, .FiscalDayOfQuarter, .FiscalDayOfYear, _
                                    .FiscalWeekOfMonth, .FiscalWeekOfQuarter, .FiscalWeekOfYear, _
                                    .FiscalMonthOfQuarter, .FiscalMonthOfYear, .FiscalQuarterOfYear, .FiscalYear, .Holiday)
    End With

End Function

Could this code be improved? The idea isn't to execute the T-SQL, merely to generate it.

\$\endgroup\$
  • \$\begingroup\$ Note, the "Holiday" field will be manually updated later on. \$\endgroup\$ – Mathieu Guindon Nov 24 '14 at 18:44
5
\$\begingroup\$

Why duplicate the for loop?

If ...
    For fWeekOfMonth = 1 To 5
        GoSub ProcessWeekDays
    Next
Else
    For fWeekOfMonth = 1 To 4
        GoSub ProcessWeekDays
    Next
End If

I suggest to store the 5 or 4 in a variable and parameterize one loop with it.


I can see this easily becoming a nightmare:

result = Framework.Strings.Format("INSERT INTO dwd.FiscalCalendars (_DateInserted,CalendarDate,CalendarDayOfWeek,CalendarDayOfMonth,CalendarDayOfYear,CalendarWeekOfMonth,CalendarWeekOfYear,CalendarMonthOfYear,CalendarYear," & _
                                  "FiscalDayOfWeek,FiscalDayOfMonth,FiscalDayOfQuarter,FiscalDayOfYear,FiscalWeekOfMonth,FiscalWeekOfQuarter,FiscalWeekOfYear,FiscalMonthOfQuarter,FiscalMonthOfYear,FiscalQuarterOfYear,FiscalYear,Holiday) ", _
                                  "VALUES (@ts,'{0}',{1},{2},{3},{4},{5},{6},{7},{8},{9},{10},{11},{12},{13},{14},{15},{16},{17},{18},'{19}');", _
                                .CalendarDate, .CalendarDayOfWeek, .CalendarDayOfMonth, .CalendarDayOfYear, _
                                .CalendarWeekOfMonth, .CalendarWeekOfYear, .CalendarMonthOfYear, .calendarYear, _
                                .FiscalDayOfWeek, .FiscalDayOfMonth, .FiscalDayOfQuarter, .FiscalDayOfYear, _
                                .FiscalWeekOfMonth, .FiscalWeekOfQuarter, .FiscalWeekOfYear, _
                                .FiscalMonthOfQuarter, .FiscalMonthOfYear, .FiscalQuarterOfYear, .FiscalYear, .Holiday)

Too many things can go wrong:

  • You might make a mistake in the {n} count
  • You might make a mistake when aligning the quoted '{n}' items with the correct parameters

The problem is that you have too many things that have to be well aligned:

  • the column list in the INSERT
  • the column list in the format string
  • the column list in the format params

Rewrite with good old-fashioned concatenation instead of Strings.Format. That way you eliminate one potential error vector. The result will also be more readable.

It might be a good idea to make the columns appear on their own lines, both when writing the list for INSERT and the values. That way you could easily copy-paste one of the lists in notepad and put side-by-side with the other to see where you missed something. (It can happen that you swear everything is well-aligned, but you just won't see the mistake until you actually break up the lines and put them side by side. Speaking from experience here, unfortunately.)

\$\endgroup\$
3
\$\begingroup\$
For fWeekOfMonth = 1 To 5
    GoSub ProcessWeekDays
Next

All I'm going to say is that you know better and this is neither cute nor clever. This is a slippery slope to Spaghetti Code.

GoSub is leftover from an ancient version of Visual Basic that didn't have subroutines. Use a proper subroutine or function.

For what it's worth, it appears to be a correct and clean use of GoSub. You just shouldn't be using it.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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