Beefy Boxes and Bandwidth Generously Provided by pair Networks
Perl: the Markov chain saw
 
PerlMonks  

Re^3: The joys of bad code

by hardburn (Abbot)
on Oct 26, 2004 at 16:41 UTC ( #402697=note: print w/ replies, xml ) Need Help??


in reply to Re^2: The joys of bad code
in thread The joys of bad code

Glad somebody was brave enough to ask. It seems like reasonable code to me, too. There are better ways to do it in other languages, but if you're writing in VB, you have to live with the limitations that implies. (In Perl I'd either use DateTime or make it a hash lookup.)

"There is no shame in being self-taught, only in not trying to learn in the first place." -- Atrus, Myst: The Book of D'ni.


Comment on Re^3: The joys of bad code
Re^4: The joys of bad code
by demerphq (Chancellor) on Oct 26, 2004 at 17:36 UTC

    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

    ---
    demerphq

      First they ignore you, then they laugh at you, then they fight you, then you win.
      -- Gandhi

      Flux8


      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.

      "There is no shame in being self-taught, only in not trying to learn in the first place." -- Atrus, Myst: The Book of D'ni.

      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.

        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

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


        ---
        demerphq

          First they ignore you, then they laugh at you, then they fight you, then you win.
          -- Gandhi

          Flux8


Re^4: The joys of bad code
by Dylan (Monk) on Oct 26, 2004 at 17:37 UTC
    An array lookup would work, too.
Re^4: The joys of bad code
by jdporter (Canon) on Oct 28, 2004 at 17:47 UTC
    Actually, there's a pretty nifty native way to do this. Probably less efficient than a hash lookup, but more consistent.
    Function MyMonthName(ByVal m As Long, Optional ByVal appr As Boolean = + False) As String Dim v v = CDate("2004-" & m & "-01") Dim fmt As String If appr Then fmt = "MMM" Else fmt = "MMMM" End If MyMonthName = Format(v, fmt) End Function

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://402697]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others exploiting the Monastery: (9)
As of 2014-12-27 17:43 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    Is guessing a good strategy for surviving in the IT business?





    Results (177 votes), past polls