Re^4: The joys of bad code

[demerphq]

A hash lookup? Why not an array? Anyway, i wouldnt have laughed so hard if it had been written something like the following:

Public Function GetMonthName(ByVal m As Long, _
    Optional ByVal abbr As Boolean = False) As String


    Static Months(12) As String
    If (Months(1) <> "January") Then
        Months(1) = "January"
        Months(2) = "February"
        Months(3) = "March"
        Months(4) = "April"
        Months(5) = "May"
        Months(6) = "June"
        Months(7) = "July"
        Months(8) = "August"
        Months(9) = "September"
        Months(10) = "October"
        Months(11) = "November"
        Months(12) = "December"
    End If
    
   Dim tmp As String

   If (m >= 1 And m <= 12) Then
        tmp = Months(m)
   Else
        tmp = Format(m, "0")
   End If

   If abbr Then
      GetMonthName = Left(tmp, 3)
   Else
      GetMonthName = tmp
   End If

End Function

[hardburn]

An array would work just fine in this case. For things like this, I tend to automatically (for better or for worse) reach for a hash, as I usually need to do the lookup based on a string.

Even so, I don't think the orginal VB code is so bad. The switch statement is probably going to be O(n) (since it can't optimize down to a jump like C's less flexible switch statement can), but the search space is so small that it won't matter much.

I do wonder, though, if this function should be public. If the application is properly OO, this is the sort of detail that should probably be hidden away. I'd have to know more about the overall application to be sure, though.

[Anonymous Monk]

I thought you were laughing because he used Format(m,"0") in his CASE ELSE statement, while Format(date,"mmm") or Format(date,"mmmm") would have given him the results he was looking for in the first place.

[g00n]

coupling is something I noticed. if you call *GetMonthName* and pass an arg for the month name, than another arg to abbreviate. The same effect could be obtained writing a formatting function, Format(GetMonthName(arg), "mmm") or even better as you suggest calling Date. having the formatting within the function may be a short cut for now but I bet at sometime changes could be made introducing bugs.

its also pretty common trap to reimplement (usually poorly) functions that pre-exist.

Janitored by Arunbear - retitled from 'coupling', as per Monastery guidelines

[demerphq]

Well, this code has to be locale independent so I actually can sympathise with this approach. We normally deploy onto German Computers but we also end up on various others as well and the output has to be english regardless so I can see why he took this approach than figuring out how to override the users locale settings. (OTOH if you know an easy way to do it im all ears :-)

[Golo]

(OTOH if you know an easy way to do it im all ears :-)

Maybe SetLocaleInfo() is of help?

[Anonymous Monk]

Ah, I was thinking the opposite, that you would want the language to change when you changed the locale settings.

I don't have anywhere to test it, but if I remember correctly I *think* that if you do the following, it will return formatted U.S. English regardless of system settings.

Dim dToday As Date
dToday = #10/28/2004# 
MsgBox format(dToday#,"mmm")

(i.e. Use # rather than " to define your dates. It may be SQL I'm thinking of though... If you have a chance to test it, let me know :) )