Comment On Spot The Fix

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.

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.

:)



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

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

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.

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....

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.

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.

 

 

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.

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.

Alex Papadimoulis:

IsValidGroup = True
If IsValidGroup Then ...

WTF? Am I missing something?

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



	'add the products

    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, _
		  product_group.selectSingleNode("@product_desc").Value
	  End If



	Next  

    'add it to the user's catalog

	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.

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 customer.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.  

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.

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

That's a bit odd innit?

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

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)

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.

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.

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.

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.

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.

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"

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.

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."

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.

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.

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

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

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.

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

	  '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.

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.

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.

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.

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.

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.

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 ...

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

Yes, we do.

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"...

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

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...

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

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

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.

  '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.

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...

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?

 

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.