Min and Max... ?



  • So I was debugging some code in our system, and traced it down to incorrect values coming out of this function, which is supposed to compute and return the minimum and maximum values out of a set.  I hadn't given the function much thought, assuming it worked (isn't that a fair assumption in an engineering company working on mature code?), but nearly fell out of my chair when I traced it through.

    See if you can spot the problem with this function:

    void GetMinMax(int &returnMin, int &returnMmax)
    {
        int min = 0;
        int max = 0;
        returnMin = 0;
        returnMax = 0;

        for (int i = 0; i < 10; i++)
        {
           if ((returnMin = value[i]) > min)
               min = returnMin;
           if ((returnMax = value[i]) > max)
               max = returnMax;
        }
    }

    (Simplified for clarity, but otherwise the entire function is verbatim.  In this anonymization, value[] has become an array of integers.) 

    And this code has been in the system for years.  And it's a military application.  Yikes!

     



  •  I wonder how the thought process while writing this code was... "funny, never noticed before that the algoritms to get the min and max values are identical... but, oh well, the variables are different, so it must work..."

    Then again, the more realistic scenario probably involves CTRL-C, CTRL-V, an inconveniently ringing phone and possibly a cigarette.



  • nice.  But more of a D'Oh!! than a WTF?!



  • @campkev said:

    nice.  But more of a D'Oh!! than a WTF?!

     

    The WTF is that this has been used for years and nobody noticed that this will almost never work properly.



  •  What..? The billions spend on war and the tons of troops wasn't the actual calculated minimum? Oops...



  •  Besides not working, whats the deal with all of the assignments going on there? Couldn't you easily get rid of min and max all together?



  • @shakin said:

    The WTF is that this has been used for years and nobody noticed that this will almost never work properly.

    Yup.  Up until recently, the values being compared in the real world was, coincidentally, a set of identical values, so the min and max were the same (as all the other values, too).  A recent change elsewhere changed this pattern, which suddenly caused results to fail elsewhere... It took quite a bit of tracing before I realized this is where the real problem was.  One tends to assume that a simple function like min/max should work, so I kept looking elsewhere...

    I literally said out loud "Who the hell wrote this!?" when I found it.  Turns out it was a predecessor who didn't last long in the company and who apparently didn't test things very vigorously.

    I replaced it with a much more sensible min/max algorithm.  Well, at least one that actually works.

     



  • @WhiskeyJack said:

    Yup.  Up until recently, the values being compared in the real world was, coincidentally, a set of identical values, so the min and max were the same (as all the other values, too).

     

    Well the source given by you wouldn't even work with a set of identical numbers due to min and max both being initialized to 0 instead of +MAXINT and -MAXINT.



  • @WhiskeyJack said:

    I replaced it with a much more sensible min/max algorithm.  Well, at least one that actually works.

     

     oo, paste here so we can criticize.



  • @WhiskeyJack said:

    void GetMinMax(int &returnMin, int &returnMmax)

    Is it safe to assume that the double 'M' in returnMmax was a translation error and not another error?



  • So what was fixed this time? "> min" was changed to "< min", but the code still returns value[9] for both min and max? ;)



  • @seriousJoker said:

    @WhiskeyJack said:

    Yup.  Up until recently, the values being compared in the real world was, coincidentally, a set of identical values, so the min and max were the same (as all the other values, too).

     

    Well the source given by you wouldn't even work with a set of identical numbers due to min and max both being initialized to 0 instead of +MAXINT and -MAXINT.


    That was the first bug that I noticed.  Appearantly before the real world data was not just always identical, but was always a list of 0s.


  • @tster said:

    @seriousJoker said:

    @WhiskeyJack said:

    Yup.  Up until recently, the values being compared in the real world was, coincidentally, a set of identical values, so the min and max were the same (as all the other values, too).

     

    Well the source given by you wouldn't even work with a set of identical numbers due to min and max both being initialized to 0 instead of +MAXINT and -MAXINT.


    That was the first bug that I noticed.  Appearantly before the real world data was not just always identical, but was always a list of 0s.
    Note how there are assignments to returnMin and returnMax inside the condition parts of the if statements.  Those are evaluated before the comparison, so the return variables get assigned on each iteration and are eventually left at value[9].  The local min and max variables are not used for anything that has consequences outside the function.


  • @WhiskeyJack said:

    See if you can spot the problem with this function:

    Which one? People have already pointed out three (bad initialisation; > rather than <; assigning to the wrong variables). I can see a fourth: why complicate the code with the use of expressions with side-effects? Either array subscripting is expensive, in which case do it once rather than twice, or it isn't, in which case do it up to four times.



  • @pjt33 said:

    @WhiskeyJack said:

    See if you can spot the problem with this function:

    Which one? People have already pointed out three (bad initialisation; > rather than <; assigning to the wrong variables). I can see a fourth: why complicate the code with the use of expressions with side-effects? Either array subscripting is expensive, in which case do it once rather than twice, or it isn't, in which case do it up to four times.

     

    If it's expensive then they could increment the pointer instead.



  • @seriousJoker said:

    @WhiskeyJack said:

    Yup.  Up until recently, the values being compared in the real world was, coincidentally, a set of identical values, so the min and max were the same (as all the other values, too).

     

    Well the source given by you wouldn't even work with a set of identical numbers due to min and max both being initialized to 0 instead of +MAXINT and -MAXINT.

     

    <sarcasm> Hey, it works for positive numbers damn it!!  They're the military, nothing is ever negative</sarcasm>



  • @tster said:

    If it's expensive then they could increment the pointer instead.

    Won't the compile automatically do that?

    Anyway, performance is the least of our worries. I need to start digging my bomb shelter.


Log in to reply