Re-inventing the wheel



  • This thread is dedicated to all code snipets that shows ugly  code doing something that already exists.

     Here is a replacement for the Java method : Math.min(int,int). Not only the name doesn't say what it's doing, but this one let you believe you must place arguments in the right order :

     

    	/**
    * Returns the tailleMaximum if taille > tailleMaximum.
    *
    * @param tailleMaximum .
    * @param taille .
    * @return the tailleMaximum if taille > tailleMaximum.
    */
    protected int initSizes(int tailleMaximum, int taille) {
    if (tailleMaximum < taille) {
    return tailleMaximum;
    }
    return taille;
    }


  • I particularly like the bizarre mix of French and English in the comments.  I wonder if this code was outsourced and commented later. 

    <hints id="hah_hints"></hints>


  • The "not english" "english" comment mixing happens all the time. People code in english, comment en english, but sometimes the specs are writen in the native language. And it's easier to have variables and function names that match the specs. (I had the great joy of implementing a german spec, with only little understanding of the german language...)



  • Looking at this and reading it, I don't think it is a reinvention of Math.min.

    Take a look, you pass in two values and receive one back.  One parameter is simple a value, the other is a max value.

    It really seems like this function is checking to see if your selected value in below the maximum value allowed for that value.

    In other words, simply put:

    Pick a number between 1 and 100

    If you say 230 and call this function you get this:

    InitSizes(100, 230) with a result of 100 because that is max

    If you instead say 23 you get this: 

    InitSizes(100,23) with a result of 23 because it is a valid value below the max.

    I don't see the WTF unless it is with your interpretation fo the code.

     

    I just want to add that they could have implented this using Math.min, but that is not it's purpose.  It is to initialize sizes of something.  What else is around this I have no idea and the code could be expanded later.



  • @KattMan said:

    If you say 230 and call this function you get this:

    InitSizes(100, 230) with a result of 100 because that is max

    If you instead say 23 you get this: 

    InitSizes(100,23) with a result of 23 because it is a valid value below the max.

    And would you expect different results if you didn't code this function and just called Min() instead? I mean Min(100, 230) = 100 and Min(100, 23) = 23, right?

     



  • @Mr_Bad_Example said:

    And would you expect different results if you didn't code this function and just called Min() instead? I mean Min(100, 230) = 100 and Min(100, 23) = 23, right?

     

    I know this may have gone in before my edit.

    This code could have been re-factored down to this, and if that is the case, yes, use min.  But this function is not Math.min, but should rather implement it.  The function is initSizes, which means it could do something else.  Refactoring can add to this or take away from it. 



  • Once I came across a reimplimentation of the php function file_exists(). Apparently the programmer didn't like booleans.

    function doesFileExist($filename) {

      if (file_exists($filename)) {

         return 'yes';

     } else {

        return 'no';

    }



  • @shakin said:

    Apparently the programmer didn't like booleans.
     

    Or required a plain English answer for something like ... UI?

     



  • sadly, you are most likely overthinking it.

    "one must always assume that coders are idiotic and they had no good reason for their idiotic code" - common wisdom 



  • We had a guy at at my work who instead of returning false when a function failed (in the few cases that he did use functions in PHP) would return "ERROR!"

    This was used mostly in functions that would check whether an entered MAC address or IP is valid, and that was done with about 80 lines of code that read it in character by character and set a bunch of flags - I replaced them both with regular expressions, and fixed up the return value. Now we're rewriting the whole thing properly using MVC, but its current form is a disaster. At one point I was able to log in using ' or 1=1-- as the username and password. Needless to say I got that fixed up very very fast, then checked for injection vulnerabilities elsewhere, and they were everywhere.



  • I usually call methods doing this cap().  That is, I set an upper cap on the value I intend to use.  It's a very good way of checking configurable parameters to make sure some IT monkey at the customer site doesn't decide that maxCurrentThreads should equal 128,000,000.



  • @mrprogguy said:

    I usually call methods doing this cap().  That is, I set an upper cap on the value I intend to use.

    AKA, "min()". 



  • Challenge: write a function that takes two arguments and returns the smaller argument if the first argument is less than the second argument, otherwise return the larger argument.



  • @Eternal Density said:

    Challenge: write a function that takes two arguments and returns the smaller argument if the first argument is less than the second argument, otherwise return the larger argument.

    You mean like this?

    int func(int arg1, int arg2){
      return arg1;
    }
    


  • @CapitalT said:

    You mean like this?

    int func(int arg1, int arg2){
      return arg1;
    }
    Full points for functionality, though it's not particularly creative... :P


  • int func(int arg1, int arg2,arg1orig,arg2orig){

        if(arg1 == 0){

            return arg1orig;

        }

        if(arg2 == 0){

            return arg2orig;

        }

        return func(--arg1,--arg2,arg1orig,arg2orig);

    }

    Extra points for recursion!



  •  Sorry modification:

    public int worstCompare(int num1, int num2){
        worstCompare(num1,num2,num1,num2);
    }

    public int worstCompare(int num1, int num2, int origNum1, int origNum2){
        if(num1 == Integer.MIN_VALUE){
            return origNum1;
        }
        else if(num2 == Integer.MIN_VALUE){
            return origNum1;
        }
        else {
            return worstCompare(--num1,--num2,origNum1,origNum2);
        }
    }

    This is in java. There is a method for 2 args as the problem states. Won't fail on negative numbers. This also ensures that every comparison runs for a very very long time, after all < > are illegal chars in java! Cant even print em... java will format your hard drive and kill your first adopted hamster if you use it!

    Edit, changed second return since we want the larger value if the second argument is smaller.



  • @CapitalT said:

    You mean like this?

    int func(int arg1, int arg2){
      return arg1;
    }
    

     

    You don't want warnings for not using all the arguments, so you better throw in a ++arg2 or something.



  • @shakin said:

    Once I came across a reimplimentation of the php function file_exists(). Apparently the programmer didn't like booleans.
     

    Chances are the author was just confused by the PHP comparison operators. I know I always have to think when I see ==, ===, !== and whatever else they threw into that language to make comparisons as hard as possible            . 



  • @Nandurius said:

    Chances are the author was just confused by the PHP comparison operators. I know I always have to think when I see ==, ===, !== and whatever else they threw into that language to make comparisons as hard as possible

    Are you serious?  What's so difficult about == (weakly-typed comparison) and === (strongly-typed comparison)?



  • @morbiuswilters said:

    @Nandurius said:

    Chances are the author was just confused by the PHP comparison operators. I know I always have to think when I see ==, ===, !== and whatever else they threw into that language to make comparisons as hard as possible

    Are you serious?  What's so difficult about == (weakly-typed comparison) and === (strongly-typed comparison)?

     

    That convention also bled over to Flex 2... == is same as java .equals and === is same as java ==. Once you see that the most important thing to do is figure out WTF it means before you make stupid code changes. Its like driving an 18-wheeler and not knowing what gear to switch to. You don't randomly switch gears at 50mph and stall that giant hunk-o-junk or install a mexican worker who does gear switching for you making you have to pay him 5 bucks an hour only to realize you shoula read the damn manual. (sorry no stereotyping intended)



  • Just another one. The coder didn't know all classes extending Collection have a copy constructor. A Collection.copy(List,List) method exists too

    	/**
    
     * Copie le contenu d'une liste dans une autre.
    
     * 
    
     * @param l1
    
     *            liste à copier
    
     * @param l2
    
     *            liste qui va recevoir la copie
    
     */
    
    public static void copieList(List l1, List l2) {
    
    	for (int i = 0; i &lt; l1.size(); i++) {
    
    		l2.add(l1.get(i));
    
    	}
    
    }
    


  • @Aaron said:

    I particularly like the bizarre mix of French and English in the comments.  I wonder if this code was outsourced and commented later. 

     

    Another WTF is that the code wasn't outsourced :) 



  • @KattMan said:

    @Mr_Bad_Example said:

    And would you expect different results if you didn't code this function and just called Min() instead? I mean Min(100, 230) = 100 and Min(100, 23) = 23, right?

     

    I know this may have gone in before my edit.

    This code could have been re-factored down to this, and if that is the case, yes, use min.  But this function is not Math.min, but should rather implement it.  The function is initSizes, which means it could do something else.  Refactoring can add to this or take away from it. 

     

    In the context I found it, the method WAS a Math.min reinvention, with added following constraint  : you must remember the argument order.

    1st WTF : initSize is a generic "don't-say-what-I-really-do" name

    2nd WTF: only a Math.min() was needed

    3th WTF: iniSize was a Math.min in wich argument order was important ; as you saw it, initSize(100,230) isn't the same as iniSize(230,100) but the expected behaviour of initSize(max, value) can actually be done with Math.min(max, value) and Math.min(value, max).



  •  He could have done l1.clone()

     

    Also copyList is misspelled... 



  • Here is one for you:

    namespace ASCII
    {
        class ASCII
        {
            public static byte ASCII_NUL = 0x00;
            public static byte ASCII_ESC = 0x1b;
            public static byte ASCII_DEL = 0x7f;
            public static byte ASCII_CTRLA = 0x01;
            public static byte ASCII_SOH = 0x01;
            public static byte ASCII_CTRLB = 0x02;
            public static byte ASCII_STX = 0x02;
            public static byte ASCII_CTRLC = 0x03;
            public static byte ASCII_ETX = 0x03;
            public static byte  ASCII_CTRLD = 0x04;
            public static byte  ASCII_CTRLE = 0x05;
            public static byte  ASCII_CTRLF = 0x06;
            public static byte  ASCII_ACK = 0x06;
            public static byte  ASCII_CTRLG = 0x07;
            public static byte  ASCII_CTRLH = 0x08;
            public static byte  ASCII_BS = 0x08;
            public static byte  ASCII_CTRLI = 0x09;
            public static byte  ASCII_TAB = 0x09;
            public static byte  ASCII_CTRLJ = 0x0a;
            public static byte  ASCII_CTRLK = 0x0b;
            public static byte  ASCII_CTRLL = 0x0c;
            public static byte  ASCII_CTRLM = 0x0d;
            public static byte  ASCII_CTRLN = 0x0e;
            public static byte  ASCII_CTRLO = 0x0f;
            public static byte  ASCII_CTRLP = 0x10;
            public static byte  ASCII_DLE = 0x10;
            public static byte  ASCII_CTRLQ = 0x11;
            public static byte  ASCII_CTRLR = 0x12;
            public static byte  ASCII_CTRLS = 0x13;
            public static byte  ASCII_CTRLT = 0x14;
            public static byte  ASCII_CTRLU = 0x15;
            public static byte  ASCII_NAK = 0x15;
            public static byte  ASCII_CTRLV = 0x16;
            public static byte  ASCII_CTRLW = 0x17;
            public static byte  ASCII_CTRLX = 0x18;
            public static byte  ASCII_CTRLY = 0x19;
            public static byte  ASCII_CTRLZ = 0x1a;

          ... snip
        }

     

    edit: just so you know this is from a .NET application 


Log in to reply