Sweet mysteries of life



  • In the spirit of today's front page article, here's a WTF that I found a while ago that I've just been sitting on.

    This is from a class that inherits from [code]System.Web.UI.WebControls.DropDownList[/code] and the end result is a fancy drop down with all sorts of bells and whistles and special formatting - which, to be fair, is a non-trivial task. But then there's this:

    public class dropdownlistadapter : System.Web.UI.WebControls.DropDownList
    {
        protected override void RenderContents(System.Web.UI.HtmlTextWriter writer)
        {   
            for (int i = 0; i < items.Count; i++)        
            {
                bool flag = false;
                //snip some irrelvant code that does nothing with flag
                if (item.Selected)
                {
                    if (flag)
                    {
                        this.VerifyMultiSelect();
                    }
                    flag = true;                        
                }
    
        <span style='color: #800080;'>}</span>
    <span style='color: #800080;'>}</span>
    

    }

    Do you see it? No, not the fact that flag is always false. Look closer. Here, I'll let Intellisense help:

    And a link to the docs for good measure.

    Yes, this code could be re-written as if(false) { throw new HttpException(); }

    I don't have the faintest idea of what this was actually trying to accomplish.



  • @mikeTheLiar said:

    I don't have the faintest idea of what this was actually trying to accomplish.

    They thought flag would only get initialized once, which would have made it

    if (multiSelect) { throw new HttpException(); } // multiple selection not supported


  • Considered Harmful

    Makes more sense if you rename "flag" to "itemAlreadySelected", as flag is checking to see if more than one item is selected.



  • @mikeTheLiar said:


    In the spirit of today's front page article, here's a WTF that I found a while ago that I've just been sitting on.

    This is from a class that inherits from <font face="Lucida Console" size="2">System.Web.UI.WebControls.DropDownList</font> and the end result is a fancy drop down with all sorts of bells and whistles and special formatting - which, to be fair, is a non-trivial task. But then there's this:

    public class dropdownlistadapter : System.Web.UI.WebControls.DropDownList
    {
        protected override void RenderContents(System.Web.UI.HtmlTextWriter writer)
        {   
            for (int i = 0; i < items.Count; i++)        
            {
                bool flag = false;
                //snip some irrelvant code that does nothing with flag
                if (item.Selected)
                {
                    if (flag)
                    {
                        this.VerifyMultiSelect();
                    }
                    flag = true;                        
                }
    
        <span style="color:#800080;">}</span>
    <span style="color:#800080;">}</span>
    

    }

    Do you see it? No, not the fact that flag is always false. Look closer. Here, I'll let Intellisense help:

    And a link to the docs for good measure.

    Yes, this code could be re-written as if(false) { throw new HttpException(); }

    I don't have the faintest idea of what this was actually trying to accomplish.

    It's very simple. It checks every item in the dropdown. If it finds a selected item, it sets the flag. If it finds another selected item (when the flag is already set), it checks to ensure that the System.Web.UI.WebControls.DropDownList allows multiple selections.

    I.e.:

    Does the list have more than 1 item selected?
    If so, does it allow multiple selection?
    If not, an HttpException will be thrown.

    It doesn't support multiple selections, so that code can't possibly find more than one selected item, so VerifyMultiSelect() will never run and it will never throw that exception, but that's kind of beside the point.

    tl;dr: someone thought that it was a good idea to check to make sure that a dropdown that doesn't allow multiple selections should never have more than 1 item selected



  • @anotherusername said:

    It's very simple. It checks every item in the dropdown. If it finds a selected item, it sets the flag. If it finds another selected item (when the flag is already set), it checks to ensure that the System.Web.UI.WebControls.DropDownList allows multiple selections.

    It doesn't support multiple selections, so that code will never find more than one selected item, so this.VerifyMultiSelect() will never run and it will never throw that exception, but that's kind of beside the point.

    tl;dr: someone thought that it was a good idea to check to make sure that a dropdown that doesn't allow multiple selections should never have more than 1 item selected



    Actually, this looks like appropriate Object Oriented design. This is flexible enough that if the class of the dropdown changes, at a later point, this code does not need to be changed since it will behave appropriately. It's even possible that it was set up originally to run on elements that would support multiple selections in certain situations and was then changed later.

     



  • @anotherusername said:

    @mikeTheLiar said:


    In the spirit of today's front page article, here's a WTF that I found a while ago that I've just been sitting on.

    This is from a class that inherits from <font face="Lucida Console" size="2">System.Web.UI.WebControls.DropDownList</font> and the end result is a fancy drop down with all sorts of bells and whistles and special formatting - which, to be fair, is a non-trivial task. But then there's this:

    public class dropdownlistadapter : System.Web.UI.WebControls.DropDownList
    {
        protected override void RenderContents(System.Web.UI.HtmlTextWriter writer)
        {   
            for (int i = 0; i < items.Count; i++)        
            {
                bool flag = false;
                //snip some irrelvant code that does nothing with flag
                if (item.Selected)
                {
                    if (flag)
                    {
                        this.VerifyMultiSelect();
                    }
                    flag = true;                        
                }
    
        <span style="color:#800080;">}</span>
    <span style="color:#800080;">}</span>
    

    }

    Do you see it? No, not the fact that flag is always false. Look closer. Here, I'll let Intellisense help:

    And a link to the docs for good measure.

    Yes, this code could be re-written as if(false) { throw new HttpException(); }

    I don't have the faintest idea of what this was actually trying to accomplish.

    It's very simple. It checks every item in the dropdown. If it finds a selected item, it sets the flag. If it finds another selected item (when the flag is already set), it checks to ensure that the System.Web.UI.WebControls.DropDownList allows multiple selections.

    I.e.:

    Does the list have more than 1 item selected?
    If so, does it allow multiple selection?
    If not, an HttpException will be thrown.

    It doesn't support multiple selections, so that code can't possibly find more than one selected item, so VerifyMultiSelect() will never run and it will never throw that exception, but that's kind of beside the point.

    tl;dr: someone thought that it was a good idea to check to make sure that a dropdown that doesn't allow multiple selections should never have more than 1 item selected

    That's a pretty reasonable explanation. Unfortunately flag is scoped within the for block, so it's discarded at the end of every iteration. There is no case where VerifyMultiSelect() will ever be called. If you move the flag declaration out of the for loop, then it would work as you describe, and would still be entirely pointless.


  • Considered Harmful

    @lincoln said:

    Unfortunately flag is scoped within the for block, so it's discarded at the end of every iteration. There is no case where VerifyMultiSelect() will ever be called.

    D'oh! I made the rookie mistake of reading what the code was intended to do, not what the code does.



  • @Snooder said:

    @anotherusername said:

    It's very simple. It checks every item in the dropdown. If it finds a selected item, it sets the flag. If it finds another selected item (when the flag is already set), it checks to ensure that the System.Web.UI.WebControls.DropDownList allows multiple selections.

    It doesn't support multiple selections, so that code will never find more than one selected item, so this.VerifyMultiSelect() will never run and it will never throw that exception, but that's kind of beside the point.

    tl;dr: someone thought that it was a good idea to check to make sure that a dropdown that doesn't allow multiple selections should never have more than 1 item selected



    Actually, this looks like appropriate Object Oriented design. This is flexible enough that if the class of the dropdown changes, at a later point, this code does not need to be changed since it will behave appropriately. It's even possible that it was set up originally to run on elements that would support multiple selections in certain situations and was then changed later.

     

    Yes, I thought of that, but honestly, why would you need to confirm that the number of selected items agrees with VerifyMultipleSelect? Should it ever not? Could it ever not?



  • You guys are seriously over-estimating the intelligence, foresight, and skills of the author of this snippet. I can almost guarantee that he wanted to type something other than VerifyMultiSelect() and Intellisence auto-completed it for him. He never noticed and plodded on his merry way, ignorant to the fact that his logic error (scoping of flag) was the only thing that saved this from blowing up in his face. The code base is littered with comments like "//NEED BETTER ERROR SOLUTION HERE" or one of my favorites, "//THIS NEEDS TO BE AUTOMATED - HOW????????????", complete with all the extra ?s. Another good one is at the top of a bunch of nasty, hairy DB access/alter statements, the cryptic-yet-worrying statement that "//safety is off".



  • @anotherusername said:

    @Snooder said:

    @anotherusername said:

    It's very simple. It checks every item in the dropdown. If it finds a selected item, it sets the flag. If it finds another selected item (when the flag is already set), it checks to ensure that the System.Web.UI.WebControls.DropDownList allows multiple selections.

    It doesn't support multiple selections, so that code will never find more than one selected item, so this.VerifyMultiSelect() will never run and it will never throw that exception, but that's kind of beside the point.

    tl;dr: someone thought that it was a good idea to check to make sure that a dropdown that doesn't allow multiple selections should never have more than 1 item selected



    Actually, this looks like appropriate Object Oriented design. This is flexible enough that if the class of the dropdown changes, at a later point, this code does not need to be changed since it will behave appropriately. It's even possible that it was set up originally to run on elements that would support multiple selections in certain situations and was then changed later.

    Yes, I thought of that, but honestly, why would you need to confirm that the number of selected items agrees with VerifyMultipleSelect? Should it ever not? Could it ever not?



    Well, sometimes you can have a dropdown that lets you select multiple items. In which case VerifyMultipleSelect() would not throw an exception. And sometimes you'd have a dropdown that only lets you select a single item. In which case VerifyMultipleSelect() throws the exception. Doing it this way lets you overload the VerifyMultipleSelect function as necessary instead of finding this code and rewriting it as a case statement for each individual class of dropdown.

    Hell, you might even have a crazy dropdown that will allow up to 5 selected items, but only on monday. every other day it will only allow up to 4 selected items. That seems crazy, but imagine this is a dropdown in an ecommerce marketplace and Monday has a buy 4 get 1 free special.

    The point is, and the point of all object oriented design, is that these sorts of implementation details should be left as flexible as possible.

     



  • @Snooder said:

    @anotherusername said:

    @Snooder said:

    @anotherusername said:

    It's very simple. It checks every item in the dropdown. If it finds a selected item, it sets the flag. If it finds another selected item (when the flag is already set), it checks to ensure that the System.Web.UI.WebControls.DropDownList allows multiple selections.

    It doesn't support multiple selections, so that code will never find more than one selected item, so this.VerifyMultiSelect() will never run and it will never throw that exception, but that's kind of beside the point.

    tl;dr: someone thought that it was a good idea to check to make sure that a dropdown that doesn't allow multiple selections should never have more than 1 item selected



    Actually, this looks like appropriate Object Oriented design. This is flexible enough that if the class of the dropdown changes, at a later point, this code does not need to be changed since it will behave appropriately. It's even possible that it was set up originally to run on elements that would support multiple selections in certain situations and was then changed later.

    Yes, I thought of that, but honestly, why would you need to confirm that the number of selected items agrees with VerifyMultipleSelect? Should it ever not? Could it ever not?



    Well, sometimes you can have a dropdown that lets you select multiple items. In which case VerifyMultipleSelect() would not throw an exception. And sometimes you'd have a dropdown that only lets you select a single item. In which case VerifyMultipleSelect() throws the exception. Doing it this way lets you overload the VerifyMultipleSelect function as necessary instead of finding this code and rewriting it as a case statement for each individual class of dropdown.

    Hell, you might even have a crazy dropdown that will allow up to 5 selected items, but only on monday. every other day it will only allow up to 4 selected items. That seems crazy, but imagine this is a dropdown in an ecommerce marketplace and Monday has a buy 4 get 1 free special.

    The point is, and the point of all object oriented design, is that these sorts of implementation details should be left as flexible as possible.

     

    Yes, but all the code does is blow up if a non-multi-select list has multiple items selected. Unless you're writing a unit test to ensure that a custom multi-select list class is working properly, I can't see why you'd ever need to do this. By definition, if the list is working correctly, that can never happen.



  • @Snooder said:

    @anotherusername said:

    @Snooder said:

    @anotherusername said:

    It's very simple. It checks every item in the dropdown. If it finds a selected item, it sets the flag. If it finds another selected item (when the flag is already set), it checks to ensure that the System.Web.UI.WebControls.DropDownList allows multiple selections.

    It doesn't support multiple selections, so that code will never find more than one selected item, so this.VerifyMultiSelect() will never run and it will never throw that exception, but that's kind of beside the point.

    tl;dr: someone thought that it was a good idea to check to make sure that a dropdown that doesn't allow multiple selections should never have more than 1 item selected



    Actually, this looks like appropriate Object Oriented design. This is flexible enough that if the class of the dropdown changes, at a later point, this code does not need to be changed since it will behave appropriately. It's even possible that it was set up originally to run on elements that would support multiple selections in certain situations and was then changed later.

    Yes, I thought of that, but honestly, why would you need to confirm that the number of selected items agrees with VerifyMultipleSelect? Should it ever not? Could it ever not?



    Well, sometimes you can have a dropdown that lets you select multiple items. In which case VerifyMultipleSelect() would not throw an exception. And sometimes you'd have a dropdown that only lets you select a single item. In which case VerifyMultipleSelect() throws the exception. Doing it this way lets you overload the VerifyMultipleSelect function as necessary instead of finding this code and rewriting it as a case statement for each individual class of dropdown.

    Hell, you might even have a crazy dropdown that will allow up to 5 selected items, but only on monday. every other day it will only allow up to 4 selected items. That seems crazy, but imagine this is a dropdown in an ecommerce marketplace and Monday has a buy 4 get 1 free special.

    The point is, and the point of all object oriented design, is that these sorts of implementation details should be left as flexible as possible.

     


    Actually, the more I read into this the more ridiculous this whole thing is. From MSDN:
    @ListControl.VerifyMultiSelect Method said:

    Remarks


    The VerifyMultiSelect method does nothing if multiselection is supported. Otherwise, it throws an HttpException exception.

    But that's for the ListControl class. Dropdowns just always throw an exception because they never support multiple selection.



  • @mikeTheLiar said:

    Actually, the more I read into this the more ridiculous this whole thing is. From MSDN:
    @ListControl.VerifyMultiSelect Method said:

    Remarks


    The VerifyMultiSelect method does nothing if multiselection is supported. Otherwise, it throws an HttpException exception.

    But that's for the ListControl class. Dropdowns just always throw an exception because they never support multiple selection.

    It's possible that the developer thought the code might be reused for a generic list class (or it was a generic list when the original code was written, and later changed to a dropdown). It's still pointless, because all it appears to do (ignoring the scope issue) is to confirm that any list which has more than one selected item doesn't throw an exception when its VerifyMultipleSelect() is called.



  • @anotherusername said:

    Yes, but all the code does is blow up if a non-multi-select list has multiple items selected. Unless you're writing a unit test to ensure that a custom multi-select list class is working properly, I can't see why you'd ever need to do this. By definition, if the list is working correctly, that can never happen.

    I don't think you understand. Yes, this current class on which the function is being called disallows multi-select. But a future class which extends (or implements) this one may allow multi-selection. Or it may allow multi-selection in a very limited set of cases. It could be very, very bad if you assume that the guy who wrote the future implementation gets it perfect and don't check to make sure that it really does only have the requisite number of selected items. Presumably, the possibly erroneous code would be part of the code that you've excised as irrelevant from this snippet.

    This is what exceptions are FOR. Saying "it only triggers an exception if something is wrong with the list" is like saying "it only triggers an ArrayIndexException if someone tries to access the list wrongly." 

     



  • @Snooder said:

    @anotherusername said:

    Yes, but all the code does is blow up if a non-multi-select list has multiple items selected. Unless you're writing a unit test to ensure that a custom multi-select list class is working properly, I can't see why you'd ever need to do this. By definition, if the list is working correctly, that can never happen.

    I don't think you understand. Yes, this current class on which the function is being called disallows multi-select. But a future class which extends (or implements) this one may allow multi-selection. Or it may allow multi-selection in a very limited set of cases. It could be very, very bad if you assume that the guy who wrote the future implementation gets it perfect and don't check to make sure that it really does only have the requisite number of selected items. Presumably, the possibly erroneous code would be part of the code that you've excised as irrelevant from this snippet.

    This is what exceptions are FOR. Saying "it only triggers an exception if something is wrong with the list" is like saying "it only triggers an ArrayIndexException if someone tries to access the list wrongly." 

     

    Assuming that it was properly written (with the variable definition outside the scope of the for loop), it would only trigger an exception if a list that contained multiple selected items blew up when VerifyMultiSelect() was called. According to the docs, VerifyMultiSelect does not validate the list, it just verifies that it can have multiple items selected. If multiple items are selected, there's no reason VerifyMultiSelect should ever blow up.

    Like I said before, the only imaginable use of this code would be as part of a unit test, where the goal was to ensure that the code didn't ever allow a single-selection list to somehow end up having more than 1 item selected.



  • @Snooder said:

    @anotherusername said:

    Yes, but all the code does is blow up if a non-multi-select list has multiple items selected. Unless you're writing a unit test to ensure that a custom multi-select list class is working properly, I can't see why you'd ever need to do this. By definition, if the list is working correctly, that can never happen.

    I don't think you understand. Yes, this current class on which the function is being called disallows multi-select. But a future class which extends (or implements) this one may allow multi-selection. Or it may allow multi-selection in a very limited set of cases. It could be very, very bad if you assume that the guy who wrote the future implementation gets it perfect and don't check to make sure that it really does only have the requisite number of selected items. Presumably, the possibly erroneous code would be part of the code that you've excised as irrelevant from this snippet.

    This is what exceptions are FOR. Saying "it only triggers an exception if something is wrong with the list" is like saying "it only triggers an ArrayIndexException if someone tries to access the list wrongly." 

    That's just bad OOP design. Your base classes should not be tied to the implementation details of subclasses.



  • @anotherusername said:

    Assuming that it was properly written (with the variable definition outside the scope of the for loop), it would only trigger an exception if a list that contained multiple selected items blew up when VerifyMultiSelect() was called. According to the docs, VerifyMultiSelect does not validate the list, it just verifies that it can have multiple items selected. If multiple items are selected, there's no reason VerifyMultiSelect should ever blow up.

    Like I said before, the only imaginable use of this code would be as part of a unit test, where the goal was to ensure that the code didn't ever allow a single-selection list to somehow end up having more than 1 item selected.



    Again, you seem to tied to the idea that ONLY a single-selection list can have VerifyMultiSelect() be false. And that's not true, at all.

    Yes, it would be nice to have a comprehensive set of unit tests that would invalidate the need for this check. But isn't it better not to assume that future unit tests will cover EVERY unique test case and scenario; and instead make sure that if the code does get broken, it's easier to figure out where and why it happened?

    Which is better, not having the check there and leaving the possibility of it failing in the future if someone fucks up, or having the check there and having the fuckup not be some mysterious black-box that takes hours or days to figure out?

    The reason I'm so passionate about this is that right now in the codebase I work on we have a lot of code that when it fails just returns a nullpointer exception. We COULD be handling the NPEs and I keep trying to make the argument that we should, but the team lead disagrees. Which means that every couple of weeks we get a screenshot from a mystified user that just says 'Null Pointer Exception at line X" and we have to spend hours just to figure out that the user put the expiration date of the policy before the effective date or something else similar.

    My stance is, if you KNOW that a condition is likely to break your code, generate an exception for it. The guy maintaining in a couple of years will be much happier for it.

     



  • @Snooder said:

    Again, you seem to tied to the idea that ONLY a single-selection list can have VerifyMultiSelect() be false. And that's not true, at all.

    It is true, because by definition that is what VerifyMultiSelect() is supposed to be for. It throws an HttpException exception if and only if the list control does not support multi-select mode (and it doesn't return false, or anything else, ever). You're confused by the name. It does not really "verify" anything. It's more of an "AssertMultiSelect()".



  • @anotherusername said:

    @Snooder said:
    Again, you seem to tied to the idea that ONLY a single-selection list can have VerifyMultiSelect() be false. And that's not true, at all.

    It is true, because by definition that is what VerifyMultiSelect() is supposed to be for. It throws an HttpException exception if and only if the list control does not support multi-select mode (and it doesn't return false, or anything else, ever). You're confused by the name. It does not really "verify" anything. It's more of an "AssertMultiSelect()".



    That's true for the list in the example. It is NOT guaranteed to be true for every object on which the RenderContents function can be called.

     


  • ♿ (Parody)

    @Snooder said:

    That's true for the list in the example. It is NOT guaranteed to be true for every object on which the RenderContents function can be called.

    Actually, you're probably better off just monkey patching the duck type.



  • @Snooder said:

    @anotherusername said:

    @Snooder said:
    Again, you seem to tied to the idea that ONLY a single-selection list can have VerifyMultiSelect() be false. And that's not true, at all.

    It is true, because by definition that is what VerifyMultiSelect() is supposed to be for. It throws an HttpException exception if and only if the list control does not support multi-select mode (and it doesn't return false, or anything else, ever). You're confused by the name. It does not really "verify" anything. It's more of an "AssertMultiSelect()".



    That's true for the list in the example. It is NOT guaranteed to be true for every object on which the RenderContents function can be called.

     

    It's an internal method and should work as described for any list control class in the System.Web.UI.WebControls namespace. In fact, the MSDN docs even say:

    This API supports the .NET Framework infrastructure and is not intended to be used directly from your code.

    If you expect to use your own custom-rolled list class, then I suppose you'd have to implement this yourself, but the onus is on you to make it work correctly in that case. You shouldn't have to worry about whether the System classes implement it correctly. You should just have to worry about whether .NET ever decides to change it or make it go away, since it's an internal method that you aren't supposed to call directly...


  • Considered Harmful

    @anotherusername said:

    It's an internal method and should work as described for any list control class in the System.Web.UI.WebControls namespace. In fact, the MSDN docs even say:

    This API supports the .NET Framework infrastructure and is not intended to be used directly from your code.

    You know, the variable name flag already had me thinking this was decompiled code, and now I'm sure of it. I have a very strong hunch this code was originally copypasta'd from Reflector of a built-in class.



  • @anotherusername said:

    If you expect to use your own custom-rolled list class, then I suppose you'd have to implement this yourself, but the onus is on you to make it work correctly in that case. You shouldn't have to worry about whether the System classes implement it correctly. You should just have to worry about whether .NET ever decides to change it or make it go away, since it's an internal method that you aren't supposed to call directly...



    Maybe I'm missing something here, since I don't .NET, but isn't the dropdownlistadapter in the OP a custom-rolled list class? I assumed it was.

    And the "this.VerifyMultiSelect()" statement would therefore be calling whatever implementation is in dropdownlistadapter. The only reason it is calling the method from the system class is because dropdownlistadapter does not currently overload the method. But if that changes in the future, or if a new class which inherits from dropdownlistadapter is created that DOES overload that method, then it wouldn't be calling the DropdownList.VerifyMultiSelect() but a completely different method. Am I missing something special about .NET that makes that not true?

     


Log in to reply