convertDateToFormattedCalendarWeek = "error"
Debug.Print ("function convertDateToFormattedCalendarWeek called with invalid parameter mode " & mode)
Bad idea. A bad function call involving invalid parameters is a serious run-time error. By returning a String
you're forcing the caller to systematically verify that it's not getting the "error"
magic string literal.
The function has one job. If it can't do it, it should blow up: there's a run-time error specifically for this:
Err.Raise 5, "ConvertDateToFormattedCalendarWeek", _
"Invalid 'mode' parameter value: " & mode
Run-time error 5 is essentially your ArgumentException
. If I consume your function and pass an invalid parameter, I'm expecting that error to be raised.
But what's that mode
anyway?
Public Function convertDateToFormattedCalendarWeek(input_date As Date, mode As Integer) As String
From the signature, I've no idea. [Seriously] consider using an enum instead:
Public Enum WeekFormattingMode
WFM_AmericanStandard
WFM_ISO8601
End Enum
And now your signature can be (note, member names should be PascalCase
):
Public Function ConvertDateToFormattedCalendarWeek(input_date As Date, mode As WeekFormattingMode) As String
Now the caller gets auto-completion when calling the function, and has to work very hard to supply an invalid parameter value.
This also eliminates magic constants in your implementation, and replaces the otherwise needed comments, which makes the code more self-descriptive:
Select Case mode
Case WFM_AmericanStandard
'handle American Standard formatting
Case WFM_ISO8601
'handle ISO-8601 formatting
Case Else
Err.Raise 5, "ConvertDateToFormattedCalendarWeek", _
"An invalid 'mode' parameter was supplied"
End Select
The parameters are implicitly passed ByRef
, but they're not assigned and the caller doesn't need to use them - they're really read-only inputs that the function needs in order to work with; such parameters should be passed ByVal
.
I don't agree with all your naming conventions, but as long as it's consistent, it should be good. I like that you're not using the very commonly abused Systems Hungarian notation. Option Explicit
shouldn't be part of a convention though - it should be mandatory for any VBA code that isn't toy code - and heck, even toy code should have it.
Since you're using underscores in identifiers, I need to mention that it's important that this convention isn't applied to member names, especially public ones. This is simple code and there's no harm, but in more complex object-oriented code, an underscore in a procedure name can mean code that can't compile: VBA is reserving the underscore for a very specific meaning in member names.
I like that your function is pure, and therefore easily testable. But where are the tests?
With Rubberduck (a free, open-source VBE add-in I'm working on with a bunch of fellow VBA reviewers) you could document your requirements - and have tests that you can run anytime you make a change, as simple as this:
Option Explicit
Option Private Module
'@TestModule
'@Folder("Tests")
Private Assert As New Rubberduck.AssertClass
'@TestMethod
Public Sub AmericanStandardOutputContainsSlashSpacer()
On Error GoTo TestFail
Const splitter As String = " / "
Dim actual As String
actual = ConvertDateToFormattedCalendarWeek(Date, WFM_AmericanStandard)
Assert.IsTrue InStr(expected, splitter) > 0
TestExit:
Exit Sub
TestFail:
Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub
And so on:
- ISO standard weeks start on Monday
- American standard weeks start on Sunday
- ...
Every part of the specifications should have at least one test that covers it. And then you'll want tests to document behavior for invalid inputs, too:
'@TestMethod
Public Sub RaisesErrorGivenInvalidMode()
Const ExpectedError As Long = 5
On Error GoTo TestFail
Const InvalidMode As Long = -1
If InvalidMode = WFM_AmericanStandard Or InvalidMode = WFM_ISO8601 Then
Assert.Inconclusive "InvalidMode must be an invalid parameter value."
End If
Dim actual As String
actual = ConvertDateToFormattedCalendarWeek(Now, InvalidMode)
Assert:
Assert.Fail "Expected error was not raised."
TestExit:
Exit Sub
TestFail:
If Err.Number = ExpectedError Then
Resume TestExit
Else
Resume Assert
End If
End Sub
Arguably, that "mode" parameter indicates that the function is doing too many things. If you split it into FormatDateAmericanStandard
and FormatDateISO8601
there would be no ambiguity whatsoever about how a date is formatted when you look at the calling code. Compare to:
foo = convertDateToFormattedCalendarWeek(someDate, 0)
Is foo
formatted per the American standard or ISO-8601? The enum certainly helps, but then if I have to pick between this:
foo = ConvertDateToFormattedCalendarWeek(someDate, WFM_AmericanStandard)
bar = ConvertDateToFormattedCalendarWeek(someDate, WFM_ISO8601)
And this:
foo = FormatDateAmericanStandard(someDate)
bar = FormatDateISO8601(someDate)
I pick the latter without hesitation - note how it completely eliminates the whole discussion about raising an error vs. returning "error"
, and eliminates the need to test/document what happens given an invalid mode
parameter. And it makes the code much more explicit and self-descriptive, too.
A note about the name: you're not "converting a date" - you're "formatting a date" per a specified standard; I'd make the function name start with FormatDate
.