• Fregas (unregistered) in reply to Maurits

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

    How is it a good practice to test a boolean right after you set it?  What possible purpose would that serve?

  • (cs) in reply to Fregas
    Fregas:

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

    How is it a good practice to test a boolean right after you set it?  What possible purpose would that serve?



    Rick Scott:

    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.


    I'm not sure how much I agree with that, but I'm just pointing out that some one has already addressed your questions.

    The first check certainly isn't necessary, but I don't find it bitch-worthy.  It doesn't make it difficult to understand what's going on, and I doubt it'll hurt performance by any amount humanly perceptible.  Odd?  Sure.  Silly?  Maybe.  A WTF?  I don't really think so.
  • (cs) in reply to Fregas

    OK, I withdraw my comment that the credit limit is the WTF.  I'm wrong... and what's worse, I made an idiot out of myself with all those exclamation points :$

    But I'm holding firm on the boolean thing.

    The following code pattern is typical:

    bool aok = true; // assume aok unless faults are found

    if (aok && !test_1()) { aok = false; }
    if (aok && !test_2()) { aok = false; }
    if (aok && !test_3()) { aok = false; }
    ...
    if (aok && !test_n()) { aok = false; }

    the idea being that once a test fails, don't bother evaluating further tests.

    It's true that the first if() clause tests against a boolean that was just set... but this is good, and here's why.

    What order should the tests run in?  For performance reasons, it's nice to be able to put the cheapest / most discriminatory tests first.  Leaving the extra aok && in the first if() makes it easier to swap tests around by dragging lines of code.

  • (cs) in reply to Fregas
    Anonymous:

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

    How is it a good practice to test a boolean right after you set it?  What possible purpose would that serve?

    If you need to use the return value of that call later (e.g. return it), it's usually the best way.  It's pretty much the only way if the method call is not idempotent..

  • (cs) in reply to Fregas

    Anonymous:
    How is it a good practice to test a boolean right after you set it?  What possible purpose would that serve?

    It might reduce mistakes if the code gets changed later on (by someone who is not paying attention).

    <FONT face="Courier New">flag = true;</FONT>

    <FONT face="Courier New">if(flag) {</FONT>

    <FONT face="Courier New">   flag = test1();</FONT>

    <FONT face="Courier New">}</FONT>

    <FONT face="Courier New">if(flag) {</FONT>

    <FONT face="Courier New">   flag = test2();</FONT>

    <FONT face="Courier New">}</FONT>

    Now, suppose the if statements get swapped so that test2 is before test1.  The code still works.

    <FONT face="Courier New">flag = test1();</FONT>

    <FONT face="Courier New">if(flag) {</FONT>

    <FONT face="Courier New">   flag = test2();</FONT>

    <FONT face="Courier New">}</FONT>

    Put the test2 before test1, and the code no longer works.

     

  • Billiam (unregistered) 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?



    No wonder the "average programmer" has trouble understanding his explanations :P
  • (cs) in reply to Maurits

    Maurits:

    The following code pattern is typical:

    bool aok = true; // assume aok unless faults are found

    if (aok && !test_1()) { aok = false; }
    if (aok && !test_2()) { aok = false; }
    if (aok && !test_3()) { aok = false; }
    ...
    if (aok && !test_n()) { aok = false; }

    And I think that pattern is often a WTF.

    Maurits:

    the idea being that once a test fails, don't bother evaluating further tests.

    And that would be great if that's what it does but it doesn't.  If the first test fails, it checks the value of aok n - 1 + m times.  For the rest of the loop you must make sure to put if(aok) around everything of the code breaks.  I think that is asinine.

    Refactor this in a few ways:

    if (!test_1() && !test_2() && !test_3() && !test_n()) {
        // all validation passed
    }

    or you could break it up into two methods.  Or instead of using a boolean flag, use continues.

  • (cs) in reply to loneprogrammer
    loneprogrammer:

    <FONT face="Courier New"></FONT>

    Put the test2 before test1, and the code no longer works.

    I've never found this to be a convincing reason to use a code convention.  The idea that coding something a certain way will allow people to move code around semi-randomly without breaking anything is not that believable to me.  You have to be very careful when modifying code, regardless of how it was written.  It's not something you can do in a slap-dash manner and expect it to work.  Actually, conventions that make this kind of cut and paste coding easy often lead to bugs because it might work or appear to for a a few scenarios.  I'd rather our junior developers come ask for help than move stuff around and check it in.

    It's better to focus on making the intent of the code clear.

  • (cs) in reply to dubwai
    dubwai:

    And that would be great if that's what it does but it doesn't.  If the first test fails, it checks the value of aok n - 1 + m times.  For the rest of the loop you must make sure to put if(aok) around everything of the code breaks.  I think that is asinine.

    Refactor this in a few ways:

    if (!test_1() && !test_2() && !test_3() && !test_n()) {
        // all validation passed
    }

    or you could break it up into two methods.  Or instead of using a boolean flag, use continues.


    I typically do something like this:

    bool ValidateSomething () {
      if (!test_1()) return false;
      if (!test_2()) return false;
      if (!test_n()) return false;

      return true; // Validation succeeds.
    }

    In general, I find most test sequences that use status flags can be refactored into a single function whose sole purpose is to perform that sequence of tests.  This both removes the redundant testing and encapsulates the sequence into an intelligently named function.
  • (cs) in reply to Chris F

    Chris F:
    I typically do something like this:

    bool ValidateSomething () {
      if (!test_1()) return false;
      if (!test_2()) return false;
      if (!test_n()) return false;

      return true; // Validation succeeds.
    }

    Or just:

    return (!test_1() && !test_2() && !test_n());

    Of course, it's not always that simple which is when you example comes into play.

  • (cs) in reply to dubwai

    It seems to me that this isn't a WTF.

    It may be that there's a hacked line or two of code parsing out text directly from the xml that should use the dom to do so -- but that's an excusable hack. It may be a result of debugging or encountering bad xml. It's not a "what kind of clueless git could write code like this?" mistake.

    It may be that the data is being taken from the parent node and not the child or whatever. But again this is an east kind of mistake to make and (I think) argues for using simpler local variable games (e.g. group, product, item instead of product_group_node etc.) where the context is provided by the method and so having variables whose names are 90% similar makes it easier to miss small logical errors.

    There may be a WTF here, but I don't see it. It's a slip, or a minor hack made while debugging, not a "I tried to break this egg with this sledgehammer and missed" kind of mistake that, say, the "using templates to perform integer multiplication of constants" optimization represented.

    So my opinion: this is not a WTF.

  • (cs) in reply to podperson

    Perhaps Mr. Alex could step in and elighten us as to what the actual WTF he intended us to ridicule in this post is, so we can stop bickering.  :)

  • (cs) in reply to dubwai
    dubwai:

    Maurits:
    the idea being that once a test fails, don't bother evaluating further tests.

    And that would be great if that's what it does but it doesn't.  If the first test fails, it checks the value of aok n - 1 + m times.  For the rest of the loop you must make sure to put if(aok) around everything of the code breaks.  I think that is asinine.

    I am, admittedly, making the assumption that checking the value of a boolean is cheaper than a test. :)

    I think we have the same desire to avoid indent-hell...

    if (test_1())
    {
        if (test_2())
        {
           ...
              ...   if(test_n())
                    {
                       // do everything in here
                       // (hope your Tab key doesn't wear out)
                       // (hope you don't mind horizontal scrolling)
              ...   }
        }
    }

  • (cs) in reply to Maurits
    Maurits:
    I am, admittedly, making the assumption that checking the value of a boolean is cheaper than a test. :)

    I think we have the same desire to avoid indent-hell...

    I'm not really concerned with the cost so much as the readability and reliablity of the code.  I prefer not using flags in most cases because using them is similar to saying "We're done, so lets keep going."  In an assembly line, if a bad part is made, they don't slap a sticker that says 'bad' and pass it through the rest of the assemply line so everyone can check if it's got a 'bad' sticker on it.  That wouldn't make sense.  Everything after must check this boolean flag.  It just more places for people to make mistakes doing work that the compiler is better at.

    The basic premise of strucutured programming is factored into modern lanuguage control structures.  It's not something that needs to be done manually.

  • John Hardin (unregistered) in reply to MGM
    Anonymous:
    the credit limit is effectively ignored for quantities greater than 1.
    Huh? What does the list of the possible products you can order (the catalog) have to do with what you actually DO order?

    Omitting the individual items that exceed the credit limit is an efficiency hack - they won't be able to order them in the first place, so why offer them?

    But restricting the user to only ordering X number of products such that X * price(X) < creditlimit is a function of the ordertaking code, NOT the catalog-generation code!

  • (cs) in reply to John Hardin
    Anonymous:
    Omitting the individual items that exceed the credit limit is an efficiency hack - they won't be able to order them in the first place, so why offer them?


    Well, maybe they have a coupon...
  • (cs)

    I hope no one brought this up or I'll look retarded...well, more retarded than your average VB programmer.

    InStr() will return the position of the first occurrence of the string you are searching for, and in the event that the string is not found, it will return zero (0). If his InStr() call fails, it will set lpos = 12, and it will grab what is mostly likely textual characters rather than numbers. Ever try to run text through CSng()? Crash and burn, baby.

    Then again, there's On Error Resume Next, and even if this guy is using it, he's not testing for errors. InStr() was the way to take halfway not-so-crappy code and turn it into a fragile piece of software that can't tolerate the slightest variance in the input file.

  • (cs)

    Alex Papadimoulis:

    <FONT face="Courier New"><FONT size=1>'find product groups for the current system
    Set product_groups_node = xmldom.SelectNodes("/root/systems[@type=" & SystemType & "]/product_group")
    </FONT></FONT>

    I presume that <FONT face="Courier New" size=2>SystemType</FONT> is a numeric value - otherwise a string would cause interesting behaviour without enclosing quotes...

    <FONT face="Courier New"><FONT size=2>Set product_groups_node = xmldom.SelectNodes("/root/systems[@type='" & SystemType & "']/product_group")</FONT></FONT>

    Hey!  I'm sure the variable's untyped (Variant) so it doesn't matter anyway right? ;-)

  • admwright (unregistered) in reply to Maurits

    Maurits:
    The following code pattern is typical:

    bool aok = true; // assume aok unless faults are found

    if (aok && !test_1()) { aok = false; }
    if (aok && !test_2()) { aok = false; }
    if (aok && !test_3()) { aok = false; }
    ...
    if (aok && !test_n()) { aok = false; }

    the idea being that once a test fails, don't bother evaluating further tests.

    Unfortunatly VB will evaluate all parts of an if statement to determine the result. so in the example here all of the tests will be evaluated even if the first fails.

  • MartinL (unregistered)

    In my opinion the WTF is this:

    There is a check whether there are any products in the group with quantity greater zero, and if there are, all products up to the customer's credit limit are listed, including those that are currently unavailable.

    On the other hand, if all products in the group are off limits for this customer, an empty product group will be added to the customer's catalog.

  • (cs) in reply to admwright
    I'm not really concerned with the cost so much as the readability and reliablity of the code.


    Cost can be a concern, but I figure if someone's using VB for their codebase, it's not the primary concern.  But as far as readability is concerned, in my opinion, a single condition check is not necessarily more readable.  It depends on the number of sub-conditions, I suppose.  Which looks better is in the eye of the beholder.

    As far as reliability, of course any coder worth a damn will agree with you.  However, both methods can be reliable, so as far as readability is concerned, that is up to the individual or company coding standards.

    Unfortunatly VB will evaluate all parts of an if statement to determine the result. so in the example here all of the tests will be evaluated even if the first fails.


    I can't tell from the code presented which version of VB it is, but VB.NET does actually have a short-circuited AND operator(AndAlso).
  • (cs) in reply to Ytram

    Ytram:
    I'm not really concerned with the cost so much as the readability and reliablity of the code.


    Cost can be a concern, but I figure if someone's using VB for their codebase, it's not the primary concern.  But as far as readability is concerned, in my opinion, a single condition check is not necessarily more readable.  It depends on the number of sub-conditions, I suppose.  Which looks better is in the eye of the beholder..


    The point isn't the single statement so much as not using boolean flags.

    Ytram:

    As far as reliability, of course any coder worth a damn will agree with you.  However, both methods can be reliable, so as far as readability is concerned, that is up to the individual or company coding standards.

    Replacing breaks, continues, and returns where they show the methods intent clearly with boolean flags makes the code base more fragile, in my experience.  In that respect I find code written that way to be less reliable.  I've never seen a bug that came as a result of the use of breaks, etc. while I have seen many that were the result of artificial method flow control through the use of booleans.  The theory behind using booleans this way is that having only one return is less confusing.  In reality, returns are generally easy to see and what a return statement does is obvious.  If the code reaches a return, the method execution terminates.  A boolean flag is not clear.  I don't know what happens in the rest of the method when aok is set to false.  It might skip over the rest of the logic.  It might only skip over part of it.  There might be code that only executes if aok is false.  There might be code that changes aok back to true.  I have no way of knowing this without looking through the rest of the conditionals.  If the method or loop body is finished, it should be clear.  I shouldn't have to visually parse the rest of the method to find that out.  It's bad practice to assume that setting a flag is equivalent to a return or break or continue.
  • (cs)

    I'd give up the answer, but it's just too damn funny to watch everyone miss the obvious gaffe. What's Next, Alex?

  • (cs)

    Does it matter much if you say

    someProcedure argument1, argumentN

    instead of

    someProcedure(argument1, argumentN)

    ?

    Or is this just TelligentSystems screwing up (again)? I don't think I want to know it if this is a VB " " " feature " " "!

    BTW that innerest if-block looks kinda weird:

    Is product_group adding products that it selects from itself to... itself? What's Next, an infinite loop?
    I'd have to know a bit more about that product_group object's class, I guess.

  • (cs) in reply to joost

    I think the WTF is the names.

    EVERYTHING is a product_group. Ther has to be 6 different things with product_group in their name. Its almost impossible to tell them all apart.

  • (cs) in reply to UncleMidriff

    I agree with UncleMidriff. All the calls to product_group.selectSingeNode(X) are returning whatever the default values are for X in the newly initialized product_group. Presumably they should be calls to

    product_group_node.selectSingleNode(X)

    Of course, not being a VB6 hacker, I may be misinterpreting the code.

  • (cs)

    This code seems to be quite full of WTFs. What about:

     

    rpos = InStr(lpos,product_group_node.xml,"""")

    Seems quite weird to me searching for a double quote as last position of a string, because you'll allways find the first quote in the string. So even if the value of base_price is enclosed by quotes (why would it?), it would not work...

  • Speller (unregistered)

    ... I'll leave it as an exersize for the reader to find this same reason ...

    WTF???

  • Anon (unregistered) in reply to Some dude
    Some dude:
    It sets IsValidGroup to true and then immediately tests if it is true.

    That's a bit odd innit?

    Not really. Part of the developer's problem is that several conditions must be satisfied before the actual processing may be done.

    He chose to keep track of 'all tested conditions satisfied so far' in a variable. Testing that variable before checking the next condition nicely decouples the conditions. In other words, you can remove and add conditions without affecting the rest of the code. You can even insert another condition before the first condition!

    I really like such decouplings. The average programmer will not be able to resist the urge to optimize the code into a cascade of 'if's and will claim that such code will run faster.

    Well, first, indentation may push 'if' cascades to the edge of the screen.

    Second, a half-decent compiler will figure out that it doesn't have to do the first test because IsValidGroup was just set to 'true'. For all subsequent tests it will figure out that once IsValidGroup has become 'false', it cannot ever become 'true' again because no code affecting IsValidGroup is executed. Having realized this, the compiler gains significant freedom for other optimizations.

    This part of the code is definitely not WTF; it was written by s.o. who understands decoupling.

  • (cs)

    To those of you who would argue this is a gaffe:

    IsValidGroup = True

    If IsValidGroup Then [...]

    It's not. That code is sensible. It makes the following validation lines, of which there are lots, all consistent in form instead of the first one sticking out as special for no reason. Now they can be freely added, removed, commented, un-commented, or re-arranged (e.g., perhaps to test the more commonly false conditions first in order to speed up the checks) without the danger of accidentally putting the line like this

    IsValidGroup = customer.CanAccessProductGroup [...]
    further down, or missing it out completely.

Leave a comment on “Spot The Fix”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article