• Amazinderek (unregistered)

    It must be the brute-force string processing on the XML string to retrieve the base_price value, instead of accessing it through the DOM.

  • diaphanein (unregistered)

    If I'm reading this right, you've got all this fancy xml node selection using xpath, which is pretty cool.  BUT, then the developer turns around and uses string finds and substrings into the raw xml of the found product_group node in order to retrieve an attribute value. 

    And that is truly a WTF.

  • spacey (unregistered) in reply to diaphanein

    :)

    I figure its the never ending trail of If statements....

  • MadCoder (unregistered)

    If IsValidGroup is a check, why are we setting the value of it?

  • Poppa Kapp (unregistered)

    Looks like if you can afford (Credit Limit) the price of a single unit, you can buy as many of them as you'd like, regardless of the extended price.  Further, you can buy as many different items this way as long as the unit price of any one of these items is below the credit limit.

  • (cs)

    Can't comment on the larger part of the code, but I did notice that the customer can get on credit as much as he/she wants -- as long as no single item exceeds the credit limit. So maybe he can't get a 60-foot yacht, but he can get 10,000 48-footers to make up for it....

  • Brian Enigma (unregistered)

    I like how the person who deleted the final "If IsValidGroup Then...endif" left  the comment and the add indented as if it were still there.

  • (cs)

    I have to assume that InStr section was a result of a new developer being brought on to "fix" the existing code, and not having the slightest clue what he/she was doing. I mean, they use the InStr/Mid functions to get the @base_price attribute value, and then THREE LINES DOWN they use selectSingleNode to get the exact same value. The only thing I can guess is that the programmer had the IDE set to a 128-point font and thus can only see two lines at a time. Either way, somebody deserves a boot to the head.

     

     

  • Fregas (unregistered) in reply to Poppa Kapp

    Anonymous:
    Looks like if you can afford (Credit Limit) the price of a single unit, you can buy as many of them as you'd like, regardless of the extended price.  Further, you can buy as many different items this way as long as the unit price of any one of these items is below the credit limit.

    Yah, I was thinking the same thing.

    Of course, one might argue that storing a product catalog in XML rather than a relational database is itself a WTF, but hey, its written in VB.

  • (cs) in reply to A Wizard A True Star
    A Wizard A True Star:

    I have to assume that InStr section was a result of a new developer being brought on to "fix" the existing code, and not having the slightest clue what he/she was doing. I mean, they use the InStr/Mid functions to get the @base_price attribute value, and then THREE LINES DOWN they use selectSingleNode to get the exact same value. The only thing I can guess is that the programmer had the IDE set to a 128-point font and thus can only see two lines at a time. Either way, somebody deserves a boot to the head

    I agree, that looks like the "fix" to me also.

  • (cs)

    Alex Papadimoulis:
    IsValidGroup = True
    If IsValidGroup Then ...

    WTF? Am I missing something?

  • (cs)
    Alex Papadimoulis:
      'add products and product group
  If IsValidGroup Then
    'build the product group
    Set product_group = New Catalog.ProductGroup
    	'ED: Snip 15 lines lines

    <span style="color: rgb(0, 130, 0);">'add the products</span>
<span style="color: rgb(0, 0, 255);">For</span> <span style="color: rgb(0, 0, 255);">Each</span> product_node <span style="color: rgb(0, 0, 255);">In</span> product_group_node.selectNodes(<span style="color: rgb(132, 130, 132);">"product"</span>)<br><br>	  <span style="color: rgb(0, 130, 0);">'validate that the price is not above credit limit</span>
  lpos = InStr(product_node.xml, <span style="color: rgb(132, 130, 132);">"base_price"</span>) + 12<br>	  rpos = InStr(lpos,product_group_node.xml,<span style="color: rgb(132, 130, 132);">""</span><span style="color: rgb(132, 130, 132);">""</span>)<br>	  <span style="color: rgb(0, 0, 255);">If</span> <span style="color: rgb(0, 0, 255);">CSng</span>(Mid(product_group_node.xml, lpos, rpos - lpos)) &lt;= customer.CreditLimit <span style="color: rgb(0, 0, 255);">Then</span>
	product_group.AddProduct _
	  product_group.selectSingleNode(<span style="color: rgb(132, 130, 132);">"@product_number"</span>).Value, _<br>		  product_group.selectSingleNode(<span style="color: rgb(132, 130, 132);">"@base_price"</span>).Value, _<br>		  product_group.selectSingleNode(<span style="color: rgb(132, 130, 132);">"@product_desc"</span>).Value<br>	  <span style="color: rgb(0, 0, 255);">End</span> <span style="color: rgb(0, 0, 255);">If</span>

<span style="color: rgb(0, 0, 255);">Next</span>  <br><br>    <span style="color: rgb(0, 130, 0);">'add it to the user's catalog</span>
userCatalog.AddProductGroup product_group

    

    End If
    

    Next



    Maybe I'm just stupid, but the use of product_node, product_group, and product_group_node seems a bit mixed up.  He's getting lpos from product_node, but then rpos is set to the first occurrence of "" in product_group_node, starting from the index+12 of the first occurrence of "base_price" in product_node?  And then it seems like he is adding products to product_group using values that are already in product_group...that doesn't seem right to me, but I am willing to admit that I may be missing something.
  • Hank Miller (unregistered) 

    IsValidGroup = True
    if IsValidGroup Then ...


    That first check is a tiny WTF.   Not worty of a post here, but still not 
    something that a real programmer would do...


    In general I'd do something like:

    if cu    stomer.CanAccessProductGroup( product_group_node.selectSingleNode("@group_code").Value ) AND 
    product_group_node.selectNodes("product[@quantity > 0]").length > 0

    but I'm told VB6 has some real WTFs if you try to do it that way, so I'm 
    not going to comment on what something that is a matter of choice anyway.   
    If nothing else, this way is more readable considering the length of each 
    single check.

    You know, on second though I think I've have a different function:
    userGroupValid(...) that would do all the validation, so I can do 
    if userGroupValid(...) then 
      set product group...
    The real WTF is this forum which made my post really ugly.  (I cleaned it up 
    as best I can)   Not to mention captchas that can't be read by the colorblind.


  • Rick Scott (unregistered) in reply to Hank Miller

    No, the first check is not even a minor WTF. Its a good practice to allow the programmer to move checks around as needed without affecting the overall program. If the code is performing badly because the first 5 checks are almost always true, whereas it turns out that the 6th check is where most fail, a simple cut/paste will improve performance to a small degree. Also, it affords the code a degree of symmetry and repetition to allow your eyes to focus on what's different.

  • Some dude (unregistered)

    It sets IsValidGroup to true and then immediately tests if it is true.

    That's a bit odd innit?

  • (cs)

    The WTF is obviously that there is no On Error Resume Next in there.

  • Lenny (unregistered)

    beginGrossDisregardForXml = "'validate that the price is not above credit limit"
    endGrossDisregardForXml = "If CSng(Mid(product_group_node.xml, lpos, rpos - lpos)) <= customer.CreditLimit"

    lpos = InStr(WtfCode, beginGrossDisregardForXml)
    rpos = InStr(WtfCode, endGrossDisregardForXml)

  • MGM (unregistered) in reply to Fregas

    Ugh!  I can't believe how many people are making wild guesses what the WTF is.

    You have no evidence the product catalog is not in a DB.  The program may be interfacing with a web service, or there could be dozens of other good reasons why the data is in XML.

    If I write an app in C# instead of VB, that doesn't preclude me from using XML, it only makes it easier.

    The WTF is the inexplicable choice of using InStr instead of DOM to find the element - and even moreso how that less-readable solution makes it easier for a programmer/reviewer to miss that the credit limit is effectively ignored for quantities greater than 1.

  • (cs) in reply to MGM

    Anonymous:
    Ugh!  I can't believe how many people are making wild guesses what the WTF is.
    ...

    The WTF is the inexplicable choice of using InStr instead of DOM to find the element - and even moreso how that less-readable solution makes it easier for a programmer/reviewer to miss that the credit limit is effectively ignored for quantities greater than 1.

    You know, several people did figure that out.

  • lw (unregistered) in reply to Brian Enigma
    Anonymous:
    I like how the person who deleted the final "If IsValidGroup Then...endif" left  the comment and the add indented as if it were still there.

    That is inside the final Then...Endif at least as of 14:14 PDT.
  • (cs) in reply to Voller
    Voller:

    Alex Papadimoulis:
    IsValidGroup = True
    If IsValidGroup Then ...

    WTF? Am I missing something?

    That kind of code is very common with developers who worship structured programming.  Alex deleted a lot of the checks.  The developer didn't want to use 'continue' and didn't want the last check to be inconsistent.  That's my theory, anyway.  That's not really the WTF, though.

  • (cs) in reply to dubwai

    The code sets a boolean and then immediately tests it.  This is not a WTF at all, it's perfectly good programming practice.

    The code uses mixed methods to extract data from XML.  This is not a WTF at all either, though it is somewhat questionable.

    The WTF is clearly that the CREDIT LIMIT FOR CUSTOMERS IS NOT ENFORCED!!!

    If you disagree, think about the consequences of each breach-of-good-practice.

    Boolean: code looks silly.
    Mixed methods: code looks really silly, gets a little harder to maintain.
    Credit limits not enforced: client loses money by selling too much to bad customers.

  • (cs) in reply to Maurits

    Maurits:
    If you disagree, think about the consequences of each breach-of-good-practice.

    Boolean: code looks silly.

    Are you saying that if you don't use unecessary flags, the code will look silly?  I think nothing is sillier than using flags when there is no need.

    "Twenty-seven different flags! Stick 'em on your car! Stick 'em on your window! Stick 'em in your office! Stick 'em on your cubicle! F*cking shove them up your ass! We've got special flags to shove up your ass when you're sleeping! No reason not be a patriot when you're asleep. C'mon, don't let those terrorists win. If you don't have a flag sticking out of your ass, the terrorists win! Always have flags! Give your children flags! Everyone should have a flag! At all times flags! Eat the flag! Eat it! Special edible flags! Have flags grafted on the inside of your eyelids so at all times you have flags! Have flag pills to eat so you sh*t out a flag! That's true patriotism. Don't be asleep on the job here, America"

  • Lenny (unregistered) in reply to dubwai
    dubwai:

    Are you saying that if you don't use unecessary flags, the code will look silly?  I think nothing is sillier than using flags when there is no need.

    "Twenty-seven different flags! Stick 'em on your car! Stick 'em on your window! Stick 'em in your office! Stick 'em on your cubicle! F*cking shove them up your ass! We've got special flags to shove up your ass when you're sleeping! No reason not be a patriot when you're asleep. C'mon, don't let those terrorists win. If you don't have a flag sticking out of your ass, the terrorists win! Always have flags! Give your children flags! Everyone should have a flag! At all times flags! Eat the flag! Eat it! Special edible flags! Have flags grafted on the inside of your eyelids so at all times you have flags! Have flag pills to eat so you sh*t out a flag! That's true patriotism. Don't be asleep on the job here, America"



    I don't know what this has to do with the WTF, but it kicks ass! 

    Thank you for saving today's otherwise boring WTF with your sarcastic patriotic rhetoric.
  • (cs) in reply to Lenny

    Anonymous:

    I don't know what this has to do with the WTF,

    Oh, absolutely nothing. The idea tha you must have boolean flags to not look silly reminded me of it.

    Anonymous:
    but it kicks ass!

    That's true.

    Anonymous:

    Thank you for saving today's otherwise boring WTF with your sarcastic patriotic rhetoric.

    It's David Cross off of "Shut Up You F'ing Baby"  I left off the end:

    "All flags made by Chinese prison labor, guaranteed.  gar-un-teed."

  • (cs) in reply to dubwai
    dubwai:

    Maurits:
    If you disagree, think about the consequences of each breach-of-good-practice.

    Boolean: code looks silly.

    Are you saying that if you don't use unecessary flags, the code will look silly?



    That is precisely the opposite of what Maurits said.
  • (cs) in reply to dubwai
    dubwai:

    Maurits:
    If you disagree, think about the consequences of each breach-of-good-practice.

    Boolean: code looks silly.

    Are you saying that if you don't use unecessary flags, the code will look silly?  I think nothing is sillier than using flags when there is no need.

    "Twenty-seven different flags! Stick 'em on your car! Stick 'em on your window! Stick 'em in your office! Stick 'em on your cubicle! F*cking shove them up your ass! We've got special flags to shove up your ass when you're sleeping! No reason not be a patriot when you're asleep. C'mon, don't let those terrorists win. If you don't have a flag sticking out of your ass, the terrorists win! Always have flags! Give your children flags! Everyone should have a flag! At all times flags! Eat the flag! Eat it! Special edible flags! Have flags grafted on the inside of your eyelids so at all times you have flags! Have flag pills to eat so you sh*t out a flag! That's true patriotism. Don't be asleep on the job here, America"

    Funniest WTF reply post ever.

  • alienjones (unregistered) in reply to Maurits
    Maurits:


    The WTF is clearly that the CREDIT LIMIT FOR CUSTOMERS IS NOT ENFORCED!!!




    Looking at the code


    'add it to the user's catalog

    userCatalog.AddProductGroup product_group


    It would seem to me that the code is building a catalog to show the customer only items beneath their credit limit that they MAY purchase and not items that they HAVE purchased.


    But I could be wrong

  • dude (unregistered)

    The wtf is definitely NOT the business goals of the 20 some lines of code that are posted.

  • Golly (unregistered) in reply to dude

    The real WTF is that it's VB. If someone has the fundimental intelligence required to understand xml, then they should know not to use VB for anything.

  • mad_scientist (unregistered) 

    <font size="3">When assigning to the product_group, the code uses the values:

      product_group.selectSingleNode("@product_number").Value,
      product_group.selectSingleNode("@base_price").Value,
      product_group.selectSingleNode("@product_desc").Value

    Um, product_group is the Catalog.ProductGroup, not the XML node.
    The code should be using product_group_node.selectSingleNode()...

    At least, that's how it appears to me.  I've never used VB6</font>

  • Betty (unregistered) in reply to mad_scientist 

    	  'validate that the price is not above credit limit
    	  lpos = InStr(product_node.xml, "base_price") + 12
    	  rpos = InStr(lpos,product_group_node.xml,"""")
    	  If CSng(Mid(product_group_node.xml, lpos, rpos - lpos)) <= customer.CreditLimit Then

    it could just be me but it seems unusual to do an instr in product_node then another in product_group_node and expect the results to work.
    the first argument in the rpos InStr is lpos the result of the previous instr which does not relate to this string directly.

  • Zek (unregistered) in reply to Poppa Kapp

    The credit limit vs quantity debate cannot be the real wtf. It makes perfect sense that if you are creating a catalog for viewing that you would add any item that is within the customers credit limit. This is clearly not a function written to validate a purchase. The quantity vs credit limit issue would be addressed in a purchase function/method.  It would be a real wtf for the developer to validate a purchase in a create catalog function if you ask me.

  • Jon (unregistered)

    I agree with alienjones: this code is clearly building a list of the things that the user has the ability to buy. Whether or not the user can buy the entire stock isn't the issue here.


    The base_price weirdness is the most obvious WTF. Less obvious is that the comparison to the customer's credit limit could be put directly in the call to SelectNodes on the inner loop. Overall, it's not all that interesting.

  • (cs)

    As others have pointed out, one WTF here is partially the use of InStr() to apparently parse out the value of an attribute in a node.  What happens if the XML contains: <product base_price = "1.00"/>.

    However, the real WTF (although it's iffy since there were 15 lines snipped) seems to be the call to the "AddProduct" method on the "Catalog.ProductGroup" object.  Note that the parameters that are passed to the AddProduct() method are "product_group.selectSingleNode()" calls.  Now, if we assume that this object that was created does indeed have a "selectSingleNode" property and the call to get the base price is valid, why was that not used instead of the InStr() oddness.

    I almost tend to think, by the looks of the code, that perhaps the parameters to the AddProduct() method were meant to use product_group_node.selectSingleNode() instead.  (And in that case, the same could have still been used instead of InStr().)

    Of course, I could be missing something myself, but I do agree with the others that the filtering of items from the user's catalog that are under their credit limit was the intent of that function as opposed to calculating totals.

  • (cs)

    The real WTF is the direct manipulation of a raw data format in domain logic code.  Hello design neophyte, meet my friend, data layer.  I'm sure you two will be good friends.

  • (cs) in reply to Chris F
    Chris F:
    The real WTF is the direct manipulation of a raw data format in domain logic code.  Hello design neophyte, meet my friend, data layer.  I'm sure you two will be good friends.


    I wonder if normal people ever read these forum posts and laugh harder than we do at the WTF.
  • vhawk (unregistered) in reply to sadmac
    sadmac:
    Chris F:
    The real WTF is the direct manipulation of a raw data format in domain logic code.  Hello design neophyte, meet my friend, data layer.  I'm sure you two will be good friends.


    I wonder if normal people ever read these forum posts and laugh harder than we do at the WTF.


    Hey - I have been accused of many things in life but never of being normal.  And yes I do sometimes laugh harder at the comments than at the WTF. As with the missing 'On Error resume Next' observation a couple of comment up - anyone who has ever had to read VB code or a long time ago before we knew better actually used it, would just love that one ...
  • (cs)

    Seeing the title, and reading the comments, all I can do is agree with the people who thought that this

    'validate that the price is not above credit limit
      lpos = InStr(product_node.xml, "base_price") + 12 rpos = InStr(lpos,product_group_node.xml,"""")
      If CSng(Mid(product_group_node.xml, lpos, rpos - lpos)) <= customer.CreditLimit Then

    was the fix. It just seems added in by someone who did not write the original (or had a huge blackout, but I doubt that).

    Drak

  • jzlondon (unregistered) in reply to vhawk

    Yes, we do.

  • fatgeekuk (unregistered) in reply to dubwai
    dubwai:

    Anonymous:

    I don't know what this has to do with the WTF,

    Oh, absolutely nothing. The idea tha you must have boolean flags to not look silly reminded me of it.

    Anonymous:
    but it kicks ass!

    That's true.

    Anonymous:

    Thank you for saving today's otherwise boring WTF with your sarcastic patriotic rhetoric.

    It's David Cross off of "Shut Up You F'ing Baby"  I left off the end:

    "All flags made by Chinese prison labor, guaranteed.  gar-un-teed."

    hehe, as I read that I was thinking... Hmmm, that sounds like Denis Leary, but I don't remember that bit from "Lock 'n' Load" or "Cure for Cancer"...

  • G Eek! (unregistered)

    Dear God, I subscribe to WTF to lighten up my day a little, and I end up debugging VB code :'(

  • (cs)
    Alex Papadimoulis:
        For Each product_node In product_group_node.selectNodes("product")

    	  'validate that the price is not above credit limit
	  lpos = InStr(product_node.xml, "base_price") + 12
    	  rpos = InStr(lpos,product_group_node.xml,"""")
    	  If CSng(Mid(product_group_node.xml, lpos, rpos - lpos)) <= customer.CreditLimit Then
		product_group.AddProduct _
		  product_group.selectSingleNode("@product_number").Value, _
    		  product_group.selectSingleNode("@base_price").Value, _



    lpos is determined by searching in product_node
    rpos is determined by searching in product_group_node
    credit limit is extracted from product_group_node
    product_group.AddProduct is called with nodes extracted from product_group itself
    (how can this work?)

    Eighter Alex' anonynizer went wrong, or this code is unlikely to work at all...
  • Thomas Eyde (unregistered) in reply to Golly

    Anonymous:
    The _real_ WTF is that it's VB. If someone has the fundimental intelligence required to understand xml, then they should know not to use VB for anything.

    If you don't understand VB, then you are intelligent to do anything but programming

  • Thomas Eyde (unregistered) in reply to Golly

    Anonymous:
    The _real_ WTF is that it's VB. If someone has the fundimental intelligence required to understand xml, then they should know not to use VB for anything.

    If you don't understand VB, then you are intelligent enough to do anything but programming

  • (cs) in reply to G Eek!
    Anonymous:
    Dear God, I subscribe to WTF to lighten up my day a little, and I end up debugging VB code :'(


    OMG!!! I feel the same way.  It is sad.  So sad.

    I should get out more.

        -dZ.
  • (cs) in reply to Amazinderek 

      'validate that the user has access to the group
      If IsValidGroup Then IsValidGroup = _
        customer.CanAccessProductGroup( product_group_node.selectSingleNode("@group_code").Value )

      'ED: Snip 6 further validations

      'and that there are avaiable products in the group
      If IsValidGroup Then IsValidGroup = _
        product_group_node.selectNodes("product[@quantity > 0]").length > 0


       .... I wonder: why bother doing the rest of the validations if we know the first one is bad?
    Afriad of calling Exit Function? Seems like a waste of time.

  • (cs) in reply to christoofar
    christoofar:


     .... I wonder: why bother doing the rest of the validations if we know the first one is bad?
    Afriad of calling Exit Function? Seems like a waste of time.


    I see that type of code a lot in programs designed by people that lack functional abstraction skill.  These people end up globbing individually coherent code blocks into one big mess of a function, and have to use structural flags like the above IsValidGroup to save themselves.

    Indeed, a better designer would have recognized that "checking the validity of a product group" and "adding valid product groups to the user catalog" are clearly different logical operations and thus different functions should be built to support the operations.

    Of course, a better designer would also recognize that even though it's shiny XML, it's still a raw data format that circumvents the static type system and should be encapsulated.  But you try explaining downcast encapsulation to the average programmer...
  • (cs) in reply to Chris F

    Chris F:
    Of course, a better designer would also recognize that even though it's shiny XML, it's still a raw data format that circumvents the static type system and should be encapsulated.  But you try explaining downcast encapsulation to the average programmer...

    I don't understand what you are saying.  This XML data should be put into some kind of object?

     

  • (cs) in reply to loneprogrammer
    loneprogrammer:

    Chris F:
    Of course, a better designer would also recognize that even though it's shiny XML, it's still a raw data format that circumvents the static type system and should be encapsulated.  But you try explaining downcast encapsulation to the average programmer...

    I don't understand what you are saying.  This XML data should be put into some kind of object?

     


    In a word, yes.  Which is more coherent?  This snippet:

    If CSng(product_group_node.selectSingleNode("@base_price").Value) <= customer.CreditLimit Then
    ...
    End If

    Or this:

    If product_group.BasePrice <= customer.CreditLimit Then
    ...
    End If

    The former snippet essentially performs three operations: Finding the base price value from a node of XML data, casting it to the appropriate data type, and performing a check against the customer credit limit.  The two initial operations can be encapsulated into a mapping procedure that typically lives in the data layer.  There are some significant advantages to this approach:

    1) Removes boilerplate.
    2) Separates domain logic from data logic -- what if we decided we'd be retrieving product groups from a generic recordset object (such as returned by ADO)?  If you'd encapsulated your mapping, you would only have to change your mapping.  Since everything is so commingled here you also have to change domain code!
    3) Provides a single place for unit tests to hammer out mapping bugs.  In my experience, the number of bugs caused by two programmers inadvertantly casting to different data types (mixing singles with integers is a common mistake), or typoing the XML node or recordset column name is huge.

Leave a comment on “Spot The Fix”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article