• Katrina (unregistered)

    Hi Alex.

  • Manni (unregistered)

    Wow... I... uh... wow....

    So he's basically looking for a 10-character ISBN, the first 9 of which are guaranteed to be numbers, the last can be either a number or X. This could have been done so much simpler it's shocking. It reminds me of that code a little while back where the guy made a <select> tag in HTML by hand and manually entered all the possibilities for hours and minutes.

    I can honestly say I've never used "GoTo", I avoid it like the plague or a hooker with herpes. I love how he has a GoTo that takes the code to the next line anyway, and at the very end he has "Exit Function". Good thing he put that in there, because the ambiguous "End Function" right afterward is unpredictable.

    And correct me if I'm wrong, but if you pass the parameter as a string, won't IsEmpty() return True if the Len() = 0? That situation would already be caught with the line "If Len() <= 9" (which could have all been simplified to "If Len() <> 10")

    Maybe he just didn't know VB supported loops? Maybe he had a head injury?

  • Gah (unregistered)

    It burns!

  • Matt K (unregistered)

    +1 Point for readability! -4000 points for stupidity. I sincerely hope no one defends this code.

  • Manni (unregistered)

    My mistake, IsEmpty will always return False on a string, as long as it's Dim'ed or passed as a parameter. Either way, IsEmpty is useless in this context.

  • Mike R. (unregistered)

    Scary that this went through 5 versions and is still like this.

    There are just hundreds of ways this could be done better.

    I love the error messages. Very descriptive to the person calling this "beast" of a function ;)

  • Chris (unregistered)

    I love the end of the function:

    Exit Function
    End Function

    I guess sometimes you REALLY have to be sure you don't walk past the end of the function... :)

  • Bu (unregistered)

    Does VB support regular expressions? If so, this is yet another one-liner that didn't happen because the instruction manual is still in the shrinkwrapped box. :)

  • stubs (unregistered)

    I especially like if <= 11 and >= 9 when you want something that is a length of 10... Lets you know right off the bat that you are in for a treat.

  • Steve (unregistered)

    Guess he never heard of Regular Expressions

    lol @ "it burns", that made me laugh for a few minutes

  • Jason Mauss (unregistered)

    How come the error number is '666' ? Is this the work of the devil? ;-)

  • Derick Bailey (unregistered)

    Bu and Steve: VB6 doesn't support RegEx out of the box... you actually have to use the VBScript runtime for that.

    One time on a vb6 project, i needed to do a regular expression to simplify 30 or 40 lines of code down into like 3 lines of regular expressions... and my manager forbid me from doing it because he didn't want to include the VBScript runtime in the setup package.

  • Martin (unregistered)

    Must be another example of <A HREF="http://thedailywtf.com/archive/2004/09/17/1844.aspx">consultant code</A> where every line of code equals more money!

  • Ben Hutchings (unregistered)

    And after all that, it doesn't even check the checksum! Perhaps the programmer tried to do it with Select and gave up when the code grew too long for the VB editor or compiler to handle.

  • Steve O. (unregistered)

    Most VBers don't know regular expressions....

  • Joshua Bair (unregistered)

    If (aValue <> "no") And (aValue <> "yes") Then Err.Raise 666, ,"SCREW YOU! I DON'T KNOW WHAT THE F**K HAPPENED!"

  • Maik (unregistered)

    Alex, as for non-VB code, check your inbox...

  • Hassan Voyeau (unregistered)

    This is how I would have done it...

    Private Function isValidISBN(strISBN As String) As Boolean
    Dim regex As New RegExp


    regex.Pattern = "^\d{9}(\d|X)$"
    isValidISBN = regex.Test(strISBN) 'And isValidCheckSum(strISBN)
    End Function

    'download MS windows script hear : http://msdn.microsoft.com/library/default.asp?url=/downloads/list/webdev.asp

  • Hassan Voyeau (unregistered)

    Actually isValidCheckSum(strISBN) in above code should be hasValidISBNCheckSum(strISBN)

  • Sean Lynch (unregistered)

    I think everyone missed what the coder of this function was really after. He was going to ask his boss for a raise during the code review, and was using repeating 'yep as a subliminal message. It worked so good the first time, that he made 4 more modifications over the next year.

  • Ray S (unregistered)

    What can I say. :-)

    Like Alex says, I'd love to submit more non-VB code. Unfortunately since most of the crud my freelancing covers is VB. Nothing against VB - it's my language of choice too. Most of these things began life as a someone making a macro in MS Office, and they grow "organically" into custom business apps.

    On the plus side, it gets me lots of work. On the downside, it makes my fave language look like crap. :)

    Anyway, I would list all the problems I can see with this, but it would end up being longer than the function itself. :)

  • Jeff S (unregistered)

    Even though VB doesn't have support for regular expressions built-in (w/o using a library), it does have the LIKE operator:

    if ISBN like "#########[0-9X]" then "OK" else "not OK"

    should do the trick. this is w/o even needing to understand regular expressions.

  • josh (unregistered)

    ISBN numbers are usually written with hyphens in them, too.

  • WanFactory (unregistered)

    Perhaps his employment contract was as follows:
    in January you get paid by the number of lines of code you write

    in February you get paid by the number of lines of code you write

    in March you get paid by the number of lines of code you write....

    Wow.

  • Hassan Voyeau (unregistered)

    Note, VBScript does not support the like operator.

  • Andy (unregistered)

    OMFG! Of all the daily WTFs here, I think this must be the worst.... If I came across any person calling themselves a developer who produced this kind of code I'd make it my life's work to get that person fired.... This is beyond bad, far, far beyond. The permutations of problems with this code will haunt me for the rest of the day.... Based on this code alone I would immediately throw out any other code this so-called developer had ever written. I am shocked that this person could keep their job for over a year with this kind of code. Their boss should be fired too.... I am woozy, I must go re-caffeinate....

  • Ray S (unregistered)

    Oh, I forgot to mention - one of the times that this function is called is from code that takes input from a barcode scanner. for those of you that don't know, when ISBNs are encoded in EAN13 barcodes, the checkdigit is removed. Part of this obviously recalculates the checkdigit before passing the string on to this function.

    Yes, the "missing" checkdigit calculation not only exists - it's in one of the functions that actually call this one...

  • AjarnMark (unregistered)

    Well, hey, at least he used SELECT CASE statements and not a series of nested IF...THEN...ELSE statements like a piece of "code" I ran across last week. UGH!

  • Mike Powell (unregistered)

    Best. WTF. Ever.

    I was already laughing by the time I finished reading the name of the function.

  • mikey (unregistered)

    Hey, the code would mostly work (assuming aValue's initialized by the compiler/runtime) and it's got some great features:
    - protects against fractional string lengths due to slight inaccuracies in len(), e.g. 10.5 characters -- what if somebody passes in a slightly broken UTF16 string that's a valid ISBN?
    - allows for reordering of case statements to improve performance, in case there are digits that prove more popular
    - easy maintenance when ISBN format changes (http://www.niso.org/standards/resources/ISBN.html)
    - unrolled loop makes it much faster

    Really, I think we need to appreciate how beautifully optimised this code is.

  • Jake Vinson (unregistered)

    Thank god, I'm developing an app that needs to check if a phone number is valid. Looking at that code, all I have to do is have 10 switch statements, and a few others to handle (, ), and -.

    Seriously, though, I have a hard time believing this one.

  • Bu (unregistered)

    Mikey, I think I'm gonna hire you for my marketing department! OOH maybe even an infomercial! :)

    "Have your credit card ready and dial toll free..."

  • Jeff S (unregistered)

    @Hassan -- this can't be VBScript, since there are typed variable declarations.

  • Hassan Voyeau (unregistered)

    @Jeff -- I never said this code was VBScript.

  • Jeff M (unregistered)

    Wow. I think something just short-circuited in my brain. What a POS! A well commented POS, I will give him that.

  • Andrew Tetlaw (unregistered)

    Oh no it's LotusScript again! Arrrrrrrrg

  • andy (unregistered)
  • Richard Tallent (unregistered)

    ISBN is the mark of the beast! We shall all be stamped with numbers and entered into Amazon's Great Catalog of Books and Souls.

    This is why I've petitioned Microsoft to rename VB.NET to B#. Not that you can't be just as much of a dunderhead in .NET, but the annals of corporate Morton moronity are too chocked-full of head-shaking "doh!" moments like this for VB programmers like the rest of us (who use it by choice after casting off the curly-braced brainwashing of public universities).

  • Perry (unregistered)

    The icing on the cake for this code is that the function returns a variant, which is the value of the rather heavily-used "aValue" variable. There's nothing like returning a variant value of "yes" or "no" to be efficient...
    <br>
    If CheckIfISBNIsGoodISBNOrNot(value) = "yes" Then...

  • Centaur (unregistered)

    +1 Readability for well-named variables
    +1 Style for correct use of GoTo (that doesn’t make the code look like spaghetti)
    +1 Efficiency for unrolling the loop
    +1 Internationalization for supporting encodings where [0-9] are not contiguous (do these exist?)
    -1 Localization for hard-coding "yes" and "no"…
    -1 Maintainability …in multiple places
    +1 Style for making an explicit GoTo TheEnd at the end of ErrorChecking, because this allows for another block to be inserted there and not have it executed after ErrorChecking
    0 for marking #todo# in code… after all, who reads them?
    …and -20 for not doing the simplest thing that can possibly work :)

  • ProffK (unregistered)

    I don't believe this. The function name and error codes smack of a college, back of the class, bad coding competition.

  • RJ (unregistered)

    Actually in defense of the original author of the code .....

    nah, can't do it. What was I thinking?!?

  • stu (unregistered)

    it... runs... fast ?

    then again, I should use the IF statement instead of CASE.

  • Martin DeMello (unregistered)

    <i>
    GoTo TheEnd
    TheEnd:

    # ...........

    Exit Function
    End Function
    </i>

    Looks like someone doesn't trust the program counter :)

  • J&#246;rgen S (unregistered)

    Uh, this must be a fabrication. I simply refuse to believe that there are people as dumb as this one. The intelligence displayed in this piece of code isn't enough to stimulate automatic breathing. I've heard of nobody programming while in a respirator.

  • KoFFiE (unregistered)

    Damn - I really was stunned for a moment... When I saw the first Select Case appearing on my screen, I was like "oh no - he didn't"... and stayed like that for 5 minutes...

    What I really do like about this code is his errorhandling... What if the CPU just jumped from it's own to one of the labels? Well, hell must have broken loose then, and he raises the error 666... Good thinking, can't be safe enought now can we? Just missing the "PrayForMerci" label... Also, his errormessages are so clear we should really make a difference between the 2 error cases. Also when something went wrong, you really couldn't return "no", now could you? Also - booleans were invented only for educational purposes...

  • Mike R. (unregistered)

    It's obvious this code is the root of all evil ;)

    * Error Code 666 ...
    * 9 Identical select blocks
    * ... Select Blocks for finding digits?!
    * GoTo is considered "unclean"
    * String return value when boolean could suffice.

    (...)

  • Jeff S (unregistered)

    I would pay to see what the first version of this code looked like !!!!


    Kind of like when you drive the down the road and see a car accident, you can't help but look...

    the first version was probably:

    If ISBN ="000000000" or
    ISBN ="000000001" or
    ISBN ="000000002" or
    ...etc...

    over a billion lines of code !!!

  • Jannik Anker (unregistered)

    I have to join the (surprisingly small) group of readers who cannot believe that someone could actually be dumb enough to write code like this.

    Actually, I "coded" ASP/VBScript for a full week before discovering loops (quite a while back ;-)... Makes you wonder just how long this guy has been in the game!

  • TJ (unregistered)

    'Init Variables
    programmerSucks="yep"
    isGoodForTheIndustry="no"
    shouldbeFlippingBurgers="yep"

Leave a comment on “To ISBN or not ISBN, that is the question ”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article