• Patrick Sullivan (unregistered)

    I'm at a complete loss for words, it is good to know that "Court Ordered" always returns a "positive" response though!

    Also wouldn't returning strings or integers from the same function cause problems? Maybe I'm getting to c# on that but that looks like a big problem to me.

    Oh one more thing, love the blog, it brightens all my days!! Keep it goin!

  • Jake Vinson (unregistered)

    Every post on this site has someone writing "well, this implementation is actually good if you need to x."

    I'm going to preemptively call whomever defends this a douchebag.

  • Hassan Voyeau (unregistered)

    Without knowing what language this is, I see absolutely nothing wrong with this. A simple function that does some translation. Big deal!!!

  • Tim Cartwright (unregistered)

    It is VBScript written for an asp page that I inherited.

  • Dave M. (unregistered)

    OH ... MY ... GOD! Was this designed to output as random a value as what it expects as input? strTemp? A temporary parameter? :blink: Returns a String or an Integer?

    Tim, I am truly glad I am not you! :)

  • Jon Choy (unregistered)

    Hmm. it almost made sense to me if VBScript Select Case evaluates ALL of the conditions in order... but then I found the "0" -> No mapping.

    I think this might be a case for a "once wasn't quite enough" refactoring!

  • Jake Vinson (unregistered)

    Actually, the more I look at this code, it's a good way to get YES, NO, 1, 0, or an empty string returned randomly. Therefore, I've successfully branded myself a doucheback based on my previous comment.

    YesNoReturn(YesNoReturn(YesNoReturn(YesNoReturn(InputString))))

    Who knows what that'll return?

  • Chris B. Behrens (unregistered)

    Looks like a data scrubbing routine to me. I had to write one that took "St.", "St", "Street", "Str", "Strt", you get the idea, and transformed it to "Street" in a different field.

  • Addy Santo (unregistered)

    The first thing that passed through my head when reading this was the "Brilliant!" from the Guiness commercial :)

    This one: mms://windowsmedia.dvlabs.com/adcritic/guiness-bread.asf

  • cablito (unregistered)

    Its a multi purpose two way function. You can call it with any kind of parameters and it will return a useless conversion either way.

    YesNoReturn("Y") returns "YES" right?
    now... Yes does not seem that he wants so here is the code...

    If YesNoReturn("Y") = "YES" THEN


    'now we get the integer
    dim final as intenger
    final = VAL(YesnoReturn("YES"))
    If Cbool(Final) = True then
    msgbox "User answered Yes",1,"Dan Appleman book Test program #1"
    end if

    end if

    People actually never heard of switch... ooo asp vbscript support switch? I can´t remember.

  • cablito (unregistered)

    I hope Hassan was being ironic.

  • Tim Cartwright (unregistered)

    cablito switch == select case...

  • qwerty (unregistered)

    Actually that function does work pretty well. You just need the proper wrapping function. And I think I have it:

    Function ScrubThisSchnizzel(TakeThis)
    do
    TakeThis = YesNoReturn(UCase(TakeThis))
    loop while VarType(TakeThis) <> vbInteger And Not (VarType(TakeThis) = vbString and TakeThis="")

    if VarType(TakeThis) = vbString then
    ScrubThisSchnizzel = "WTF??"
    else
    ScrubThisSchnizzel = Chr(Asc("N") + TakeThis*11)
    end if
    End Function

  • Hassan Voyeau (unregistered)

    No. I was not being ironic. Thus far, no one has shown me what is wrong with this function. I would be nice if we were told how the function was used but as it stands, I see absolutely nothing wrong with it...

  • riviera (unregistered)

    Well, the first thing that's wrong with it is that it's written in an ugly language :)

    But seriously, good code should tell you what it's meant to be doing (as my favourite saying goes: a programming language is a form of communication -- but not just with a computer, but with other people [who read your code]).

    This code neither have any visual structure, nor it does tell you anything about it's purpose, than it returns some values to some input, but none of them makes any sense (and it's not because it's taken out of it's context).

    As previously said, one of the very first things that strike the reader is the various return values (strings and numbers). Why don't simply return just a boolean, for instance?

    Anyway, I've seen worse, but it's still a kind of shame. There are a lots of people who'd need to learn how to program properly.

  • Kettle Korn (unregistered)

    Hassan, Since you don't see what's wrong with that function, you probably won't see anything wrong with the code below:
    Private Sub csiteid_Chk(ChkStrg As String, retval As Integer)
    If (ChkStrg <> "") Then
    Dim myuserid As String
    Dim cursorNbr As Integer
    Dim isAuthorised As spResults

    Call GetBufferValue("bpes.userid", myuserid)
    Call SqlCursorEx(cursorNbr, NOLEVEL + SqlFastReadOnly, "custom", "", "")
    Call SqlFetch1(cursorNbr, "System.dbo.sp_UserAccess_UserId_Site" + SParm(myuserid) + SParm(ChkStrg), isAuthorised, LenB(isAuthorised))
    Call SqlFree(cursorNbr)
    If StrComp(Trim(isAuthorised.results), "TRUE", vbTextCompare) = 0 Then
    Else
    MsgBox "You are not authorised to use this site. Contact HCL IT if this is a problem for you."
    Call SetObjectValue("csiteid", "")
    retval = -1
    End If
    End If
    End Sub

  • qwerty (unregistered)

    Nice code Kettle! Am I reading that right, did they really have a separate stored proc for every user/site combination? Or does SParam somehow make them parameters?

  • The Wolf (unregistered)

    I have to agree with Hassan, this code could be doing something completely valid, it might be designed to convert values that users have entered that are incorrect. Say the user typed in 'n' instead of what the program is expecting, then it converts it to 'yes'.

  • Prakash (unregistered)

    WTF is this.

  • Nick (unregistered)

    Uh, OK, Wolf, whatever you say. . .

    1. The function name doesn't tell us what it does. It may return "YES", "NO", 0 or 1 depending on the input value.
    It should be two functions at least - such as "makeYesNo" for when you want a string and "makeZeroOne" for when you want a psuedo-bool.
    A function which returns entirely unrelated types, depending on the input's value is seriously bad ju-ju.

    2. WTF do "COURT ORDERED" and "NV" have to do with anything?

    3. Hmm, doesn't VB let you match multiple values in a single case (i.e. Case "X", "Y", "Z")? Then there should only be one case for each possible return value. . .

  • RJ (unregistered)


    This could be a great hiring technique. Show people these snippets, if they can't tell you why they are monstrosities or if they try to defend the code you can show them the door straight away and save everyone time :)

  • Ray S (unregistered)

    If it output EVERYTHING as Y/N or TES/NO or True/False whatever, this would be perfectly fine. Random input types matching to random output types with some odd possibilities for recursion? Yikes...


    And hey, where's the "CASE ELSE"? What if someone enters "maybe" or "I'm not really sure, let me ask my boss"? :-)

  • Simon (unregistered)

    On the face of it it looks awful returning different values when you would have thought a simple true/false return value would be in order.

    But taken out of context you have no idea what the guy was trying to achieve, Hassan is correct it looks fine on the face of it, do any of you bitches have the spec ? No you dont, except maybe Tim who posted it, perhaps if he elaberated in the first place about its function, or maybe even tell us what did he change it to ! Did you change it Tim or just leave this supossed abhoration as is.

  • Wolfgang (unregistered)

    The problem I see with this code is that, besides it returning different types of return values, there doesn't seem to be a clear need for some result. All entered values are mapped to some pseudo boolean. The strange thing is, if you feed it "Y" it will tell you "YES". But if you feed it "YES", it will tell you 1 and if you feed it 1 it will tell you "YES". Any function that eats whatever this function spews must do some translation yet again, while this little schizzle could just return either true or false, based on a variety of input, like so (C#):
    bool TranslateInput(string input)
    {
    switch (input.ToLower())
    {
    case "y":
    case "yes":
    case "1":
    case "true":
    case "court ordered":
    return true;
    case "n":
    case "no":
    case "0":
    case "false":
    case "nv":
    return false;
    default:
    throw new NotSupportedException("Encountered unexpected value '" + input + "'");
    }
    }

  • cablito (unregistered)

    I still can´t believe people saying it looks fine.

  • Hassan Voyeau (unregistered)

    And no, I don't see anything wrong with my code either. And no there is no separate stored proc for each site/user combination. SParm obviously attaches a paramter and its the function that is supplied with Microsoft's Solomon API.

  • Andy (unregistered)

    Data scrubbing? This is more like data laundering. Couple passes through this puppy and Feds'll have no idea where the value came from...

  • Metallisoft (unregistered)

    Wow, yes, this would be a great interiew tool!

    "How does this look?" ::passes paper with that, um..code..on it

    "Looks fine?"

    "You're fired."

    "But this is an interview!"

    "Exactly!"

    Haha!

  • Phil Scott (unregistered)

    Simon's quote "do any of you bitches have the spec" should go down as the subtitle of this site.

  • The Wolf (unregistered)

    Nick: Yeah, you wouldn't do it writing a function like that.

  • Tim Cartwright (unregistered)

    K, The code :
    1) was sent is as is.
    2) is not used for data scrubbing.
    3) is used in production, not in a test.
    4) has many different uses.

    Hopefully I answered every ones questions.

  • Tim Cartwright (unregistered)

    From Hassan's BLOG :

    "Active Server Pages
    I create my first ASP pages this week. A simple login
    screen, some parameters and 2 reports using tables, although I guess I should be
    using CSS, but tables are the first thing that comes to my mind. All I need to
    do now is add some encryption!"

    So, Hassan, feel free to use this function in your pages for encrypting since
    you see nothing wrong with it.

  • [email protected] (Hassan Voyeau) (unregistered)

    I'm sorry but i will not be using that function but I still don't see anything wrong with it, and you stil did not show us how you fixed that function.

  • Tim Cartwright (unregistered)

    Hassan, I could not fix it, it was in use in so many different places that to
    re-factor would cause me more time and pain than it was worth. I did not explain
    what was wrong with it, as several people did above.

    To summarize what they have said above (although not in the same words):

    A function should take in and return a well defined type(s) with repeatable
    results that are of the same type(s). The argument strTemp can be, and is
    several different types, and values. Integer String, String, Empty String, and
    Boolean String. The function can then return many different things : "YES",
    "NO", 1, 0, and "". Can you imagine if Microsoft decided to change the
    CreateObject method to return an object sometimes, and a string other times, and
    a boolean at other times? The CreateObject API would be horrendous to use. When
    creating a method, the input, and return should be well defined. It is an
    unspoken contract between your code, and the consumers of your code.

  • Robert Sharp (unregistered)

    Since Hassan can't seem to understand the myriad explanations of the ridiculousness of this code, let me take a crack at it...

    You see, it stands to reason that: if given the input Y we want the output to be the "full form" response YES, then the function would be used the other way around: to return the short-form Y when given YES.

    However, that's not what happens here. What we have here is an instance of "scratch my back and I'll, shrug, give you cholera." Or more simply, a case of

    a == b,
    b == c,
    therefore
    a != c

    which is a paradox and more than slightly impossible. It just doesn't make any sense to expand Y to YES but not contract YES to Y.

    Besides, as everyone has been mentioning, it would have made things easier and less silly if there would have been a ResponseToBool() conversion function. But hey, then we wouldn't have gotten to laugh at it. Perhaps when he programmed it, the guy was in one of those "seemed like a good idea at the moment" mentalities, sort of like asbestos in walls, or CFCs as propellants.

  • nec (unregistered)

    you don't know programmer's expect or the purpose of function while he/she is coding that crap, so it is impossible o say whether that is good or bad coding.

  • Shambo (unregistered)

    Yes you can.

    I don't mean to go on a rant here but it is 4:30 in the morning, so here goes.

    English is your second language, isn't it? You don't have a first. Clearly, you spend way too much time in darkened rooms in front of your seven-year-old computer turning a whiter shade of pale. Go outside once in a while and breathe, before your brain starts to rot from all that festering stagnation and cognitive dysfunction

  • Shambo (unregistered)

    BTW... that was for nec. I need more dew.

  • Distilled Software Hate (unregistered)

    For the people who still don't understand...

    Here's what's wrong with this code:

    No matter WHAT the correct value is supposed to be, this routine will always produce the incorrect value for some inputs, and what is worse it is guaranteed to produce almost all the possible wrong input values even when the correct input was entered.

    Which means that the routine that calls it has to do more work to validate the output of this routine than to validate the input of this routine.

  • Ineffigy (unregistered)

    On of the powerful (and deadly) features of VBScript is its loose type definitions. The function itself has nothing wrong except the fact that there is no safe exit. Use CASE ELSE to create a default incase the data does not match any of the values listed. Also, I agree that you should either return strings or integers from this function for the sake of clarity. But this also depends on the purpose for the function, if this value will just be assigned to a string then it is fine, but if you are to compare or calculate the returned value, you will want to make the values all the same type.

  • Bleeding Eyes (unregistered)

    Hassan.. can you please supply a quick resume of your work history so that we know which projects to avoid inheriting

  • Alchemist (unregistered)

    :D

  • Ronabop (unregistered)

    I've seen this before. By people with the best of intentions, and the slightest mistakes made in over-assumption. (I also saw it shut out 16,000 live user sessions, and the phone switchboard go absolute batshit as a result).

    Okay, a bizarre hypothetical (based on a real problem): Say for example, a programmer has a series of designer-created web pages that they cannot change, but must write back-end code for, gluing into functions that they also cannot change (a very typical 3-tier-hell problem, if there are 3 groups of even slightly disconnected people). While the programmer cannot change the values being fed to them by the page, or the values in the storage handling tier, the programmer still does need to properly handle them, for each given scenario.

    Examples:
    1. A login page feeds them a "remember login" value of Y/N, but the arguments to AuthenticateUser(arg) requires arg to be of the format of "YES"/"NO".
    2. A preference page feeds them a "show last login" value of YES/NO, but the arguments to DisplayLastLogin(arg) requires the args to be of the format of 1/0.
    3. A record changing page feeds them a "Legally bound" value of "COURT ORDERED"/"NV", but the arguments to AttendanceRequired(arg) requires the args to be the format of 1/0.

    Now, if one was working with a small flock of page designers, and a small flock of backend programmers, and was stuck in the middle (while trying to glue 20K plus web pages), I can see how it might be truly tempting to go with this route, a universal "rewrite" function. Rather than write cleanups each freaking time a "bad" value comes in, feed it to the library.

    However....

    As soon as a designer Feeds them a value of "False", that has to go into the database as '0', this whole clever glue plan rapidly unwinds.... unless the HTTP_REFERER (or some other argument) is taken into account, in which case the "False" text can be handled with an additional check (ugh...).

    It rapidly decends into a massive bloat library of "if diplay tier says $this, it means $that in the data tier".

    The solution to this scenario, however, is more political than programmatic.

    -Bop


  • (cs) in reply to Nick

    Anonymous:


    1. The function name doesn't tell us what it does. It may return "YES", "NO", 0 or 1 depending on the input value.
    It should be two functions at least - such as "makeYesNo" for when you want a string and "makeZeroOne" for when you want a psuedo-bool.
    A function which returns entirely unrelated types, depending on the input's *value* is seriously bad ju-ju.

    You forgot one, I think

    Case "" ----> YesNoReturn = ""

    So if you put nothing in, you get nothing out.

    Just as a dumb question:  Does upper- or lower-case matter in this?  I notice that everything is in upper-case except for the False and True cases.

  • (cs) in reply to riviera

    Anonymous:
    Well, the first thing that's wrong with it is that it's written in an ugly language :)

    But seriously, good code should _tell_ you what it's meant to be doing (as my favourite saying goes: a programming language is a form of communication -- but not just with a computer, but with other people [who read your code]).
    ..........

    I agree. Look at the source code for this flight simulator by Carl Banks. It clearly shows the reader what the program does.

    Unlike the drivel above. It's written some VB scripting thingy for chrisakes!!

     

  • Igor (unregistered)

    I get it, you simply run this function recursively on its own result until you get either true or false. And if someone passes in "" then it gets stuck in a loop and you easily find out who passed an empty string.

  • Jan (unregistered)

    Isn't the problem with this function, that imagining the system in which it would make sense, would give you nightmares for weeks?

  • The Distant Future (unregistered)

    One thing it's missing, Case: "Paula" YesNoReturn = "Brillant"

  • Industrial Automation Engineer (unregistered) in reply to Hassan Voyeau

    Well, let's see: The function returns a string or an integer There is no default case, which might cause undefined behaviour. There is no "unit," aka "COURT ORDERED" and "NV" represent a wildly different concepts. What is "NV" anyway

  • Industrial Automation Engineer (unregistered) in reply to Robert Sharp

    I think Hassan wrote the code

Leave a comment on “YesNoReturn”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article