...words fail me.



  • I found this little gem in a J2ME app here (method and var names have been altered slightly to protect the guilty):



    public void renderNumber(int number, Graphics g, int x, int y, int anchor)
    {
    int w = NumberWidth;
    int h = NumberHeight;
    g.setClip(x,y,w,h);
    g.drawImage(numberImg,(number * (-NumberWidth))+x,y,Graphics.TOP | Graphics.LEFT);
    g.setClip(0,0,ScreenWidth,ScreenHeight);
    }

    public void renderNumber2(int number, Graphics g, int x, int y, int anchor)
    {
    int w = NumberWidth;
    int h = NumberHeight;
    g.setClip(x,y,w,h);
    g.setColor(0x00b7e3fb);
    g.fillRect(x-1,y-1,w+2,h+2);
    g.drawImage(numberImg2,(number * (-NumberWidth))+x,y,Graphics.TOP | Graphics.LEFT);
    g.setClip(0,0,ScreenWidth,ScreenHeight);
    }


    It's supposed to draw a numeral, either highlighted or unhighlighted, depending on which one's called.



    I'm still new at this, but I count at least four WTFs:


    1. The second method is a copypaste of the first with the addition of a yellow rectangle; they could have just been one method with a parameter that controlled whether the fillRect() happened or not. (DRY rule)
    2. The `anchor' parameter is never used.
    3. The creation of the temp variables 'w' and 'h' is unnecessary, since all they do is get copies of NumberWidth and NumberHeight. and these never get changed.
    4. The colour really should be declared as a static final int somewhere up top (or, if you like, a ${to_be_filled_in_by_ant_at_compile_time}) but not a magic number.


    Maddening, but not terribly surprising for this app (when one's code has a 2000-line commandAction() and reloads the background on every call to paint(), anything can happen).


  • @untalented_newbie said:

    I found this little gem in a J2ME app here (method and var names have been altered slightly to protect the guilty):

    1. The second method is a copypaste of the first with the addition of a yellow rectangle; they could have just been one method with a parameter that controlled whether the fillRect() happened or not. (DRY rule)
    2. The `anchor' parameter is never used.
    3. The creation of the temp variables 'w' and 'h' is unnecessary, since all they do is get copies of NumberWidth and NumberHeight. and these never get changed.
    4. The colour really should be declared as a static final int somewhere up top (or, if you like, a ${to_be_filled_in_by_ant_at_compile_time}) but not a magic number.

    1) Yes, this is somewhat of a wtf.  It might have also been ok to have two functions with the second one calling the first.  I think two-three functions would have been best - one for the text, one for the rectangle, and optionally one to do both.  This way you can always make rectangles and text, heh...

     2) I don't really see this as a wtf at all - sorta forward looking in a way (assuming that the values are already defined).

     3) This is definitely not a WTF.  You may know that NumberWidth and NumberHeight simply return an int, but in theory they could be doing tons of processing also, which you wouldn't want to have happen several times.

    4) probably should be static, yeah.



  • Oh, I don't understand how this leads you to words failing you, though...

     This is hardly a wtf, really (not that it doesn't have anything wrong, but it's hardly a wtf)



  • I think the worst part is the naming convention  for constants (or are they instance variables? doesn't matter, it's still wrong), that doesn't follow the standard naming convention (SCREEN_WIDTH or screenWidth const/instance).



  • Thus spake Morbii:Oh, I don't understand how this leads you to words failing you, though...

    Oh, that was just the tip of the iceberg. I didn't (can't, really) post any of the truly terrible code from this app, as it would give too much away. There are some real monsters lurking in here, like the game board being able to be saved in one thread, but loaded from another (and yes, they do collide, and there is data corruption as java makes a copy of the game board that can get out of sync with the real one) or the aforementioned 2000-line commandAction() that does everything itself and doesn't delegate functionality when it should.

    Here are two more tidbits (again, var and method names anonymised a bit, indenting smoothed, logic is intact, though):

           displayXpos = displayXpos + SpriteWidth + displayTopXSpacing;
           displayXpos = displayStackBorder;
           displayYpos = displayStackYOffset;
    

    Oh really? So displayXpos is supposed to be set to what now?

    Or this one:

    // Create display for Tile Match Game
       public void createTileMatchDisplay(Graphics g) {
    // create sprites
          Image image = Image.createImage(SpriteWidth,SpriteHeight);
    

    This is something that gets called every repaint, which happens every time the tile puzzle changes (user clicks on a tile to move it, etc) or the app is paused and resumed.

    1. First line: most helpful comment in history, no?
    2. Creating a new image EVERY REPAINT - probably not wise (I can imagine devices with very buggy garbage collection getting bogged down here). (in addition, the background was getting reloaded every repaint, as was the cursor, etc...)

    So, erm, yes...I sometimes have to wonder what this person was doing...



  • @untalented_newbie said:


    // Create display for Tile Match Game

    Ah, games. They usually have horrible code. It's a combination of game designers doubling up as programmers, and a high rate of burn-out taking away all the people who are really good at it. I hope you are well paid.



  • WTF's are supposed to be about accidentally malignant horrible code. This code is benign...sure it does some things that are hard to keep track of, and some things that are just unnecessary, but  I don't see anything dangerous.

     

    We need more real WTFs. 



  • @Spacecoyote said:

    We need more real WTFs. 

    ...

    Please don't say things like that. 



  • @Spacecoyote said:

    We need more real WTFs. 

    What about this great piece of code I noticed in some open source project to draw to numbers above eachother:

                            //write the numbers one by one, first, the number of beats
                            QString curBeats = QString::number(timeSignature()->beats());
                            QString curBeat = QString::number(timeSignature()->beat());
                            int curX = s.x;
                            while (!curBeats.isEmpty() || !curBeat.isEmpty()) {
                                    if (!curBeats.isEmpty())                                
                                            p->drawText(curX, (int)(s.y + 0.5*drawableContext()->height()*s.z), QString(curBeats[0]))
    ;
                                    if (!curBeat.isEmpty())
                                            p->drawText(curX, (int)(s.y + drawableContext()->height()*s.z), QString(curBeat[0]));
    
                                curX += (int)(14*s.z + 0.5);
                                curBeats = curBeats.mid(1);     //trim-off the left-most character
                                curBeat = curBeat.mid(1);       //trim-off the left-most character
                        }
    


  • @Mek said:

    ... to draw to numbers above eachother:

    To draw what to numbers above each other?

    But the code ... Wow. WTF?



  • @Morbii said:

    @untalented_newbie said:
    I found this little gem in a J2ME app here (method and var names have been altered slightly to protect the guilty):

    1. The second method is a copypaste of the first with the addition of a yellow rectangle; they could have just been one method with a parameter that controlled whether the fillRect() happened or not. (DRY rule)
    2. The `anchor' parameter is never used.
    3. The creation of the temp variables 'w' and 'h' is unnecessary, since all they do is get copies of NumberWidth and NumberHeight. and these never get changed.
    4. The colour really should be declared as a static final int somewhere up top (or, if you like, a ${to_be_filled_in_by_ant_at_compile_time}) but not a magic number.

    1) Yes, this is somewhat of a wtf.  It might have also been ok to have two functions with the second one calling the first.  I think two-three functions would have been best - one for the text, one for the rectangle, and optionally one to do both.  This way you can always make rectangles and text, heh...

     2) I don't really see this as a wtf at all - sorta forward looking in a way (assuming that the values are already defined).

     3) This is definitely not a WTF.  You may know that NumberWidth and NumberHeight simply return an int, but in theory they could be doing tons of processing also, which you wouldn't want to have happen several times.

    4) probably should be static, yeah.

    Read a bit more the code, for your (3). NumberWidth and NumberHeight are fields, not methods. So they surely don't do any processing. It they where, they would be methods (getNumberWidth()) and as such it would be ok to store them as locale variables.

     Anyway, this code is clearly far from perfect. The thing that hurts me the most is not using java convention to name fields (lowerCamelCase) and constants (UPPER_CASE). But this is however reasonable code.
     


Log in to reply