Chondromalacia of the Soul



  • Count the WTFs:

    public Collection analyzeData(List frobnitzList, List filfreList) {
    

    ArrayList ret = new ArrayList();
    ArrayList keyList = null;
    ArrayList otherKeys = new ArrayList();
    ArrayList dataList = null;
    ArrayList otherList = null;
    StringBuffer queryString = new StringBuffer();

    if (frobnizList.size() > 0) {
    queryString.append("SELECT * FROM frobnitz_table WHERE frobnitz_id in (");
    keyList = frobnitzList;
    else if (filfreList.size() > 0) {
    queryString.append("SELECT * FROM filfre_table WHERE filfre_id in (");
    keyList = filfreList;
    else {
    return ret;
    }

    for (int x = 0; x < keyList.size(); x++) {
    if (x != 0) {
    queryString.append(",");
    }
    queryString.append(keyList.get(x));
    }

    queryString.append(")");

    try {
    dataList = getDataFromMagicalPlace(queryString);
    ret.add(dataList);
    } catch (NullPointerException npe) {
    ret.add(new ArrayList());
    ret.add(new ArrayList());
    return ret;
    }

    if (dataList == null || dataList.size() <= 0) {
    ret.add(new ArrayList());
    return ret;
    }

    for (int x = 0; x < data.size(); x++) {
    Item datum = (Item) dataList.get(x);
    if (datum.getKey() > 0) {
    otherKeys.add(new Integer(datum.getKey());
    }
    }

    if (otherKeys.size() > 0) {
    otherData = getDataFromOtherMagicalPlace(otherKeys);
    otherKeys.clear();
    } else {
    ret.add(new ArrayList());
    return ret;
    }
    ret.add(otherData);

    return ret;
    }

    Do you still believe/not believe in single entrance/exit?

    [Note: I use the term in the title to mean 'grinding' as the patella grinds against the femur when the soft underlying cartilage wears away ... and please forgive any typos, as I try to sanitize on-the-fly ...]


  • ♿ (Parody)

    @zelmak said:

    Do you still believe/not believe in single entrance/exit?

    As a strict rule? No. Anyways, would a single exit look any better? Actually, you could probably refactor this to make it look reasonable, but really, there's no helping some code.

    OBBlakey: TRWTF is java



  • My problem is that the whole codebase is rife with befuddlement like this.

    o Using a try/catch block instead of checking for null-ness of a return value?
    o This method returns different datasets depending on the provided arguments (and the wrong thing? if you give it two arguments with stuff in them.) The caller must know how to properly use/fill the arguments to get the data they want.
    o Users of this method invariably cast the return value as ArrayList (but the method returns a Collection) but since the underlying Collection IS an ArrayList, Nothing Bad Happens(tm).
    o All sorts of wacky handling of this return Collection to fill it with two ArrayLists, whether they happen to have data or not.
    o Using two queries to perform this lookup instead of one (possibly not a WTF, but a join would be simpler, no?)
    o Why is otherKeys getting clear()ed and keyList is not?
    o When can an ArrayList's size() be < 0?
    o No check for null-ness on the input arguments
    o No upper bound set/checked for the 'in' argument (my understanding is that there is a limit -- both logical and practical -- to the number of items in an "in" list)

    All these things conspire to grind away my soul as I work in this codebase ... I don't have a problem fixing it, but I'm not (yet?) allowed to ...



  • Oh, and my biggest pet peeve --

    o Returning null instead of an empty collection (WRT the actual pieces doing the querying and returning data)



  • @zelmak said:

    This method returns different datasets depending on the provided arguments (and the wrong thing? if you give it two arguments with stuff in them.) The caller must know how to properly use/fill the arguments to get the data they want.

    In my Java-noviceness, I would think that WRT this point it would be worth creating an empty interface for each type of input that this method requires. The "core" logic of the original method could be broken out into a protected method that would then be shared by the original method (and its respective overloads) to minimize code duplication. This would add extensibility if the need ever arose to support a third, fourth, or fifth list argument, while providing self-documenting for IDEs (developers) for how to use the method (and its return value) in question.

    How would you handle this situation if/when you get to rewrite it?



  • @dohpaz42 said:

    @zelmak said:
    This method returns different datasets depending on the provided arguments (and the wrong thing? if you give it two arguments with stuff in them.) The caller must know how to properly use/fill the arguments to get the data they want.

    In my Java-noviceness, I would think that WRT this point it would be worth creating an empty interface for each type of input that this method requires. The "core" logic of the original method could be broken out into a protected method that would then be shared by the original method (and its respective overloads) to minimize code duplication. This would add extensibility if the need ever arose to support a third, fourth, or fifth list argument, while providing self-documenting for IDEs (developers) for how to use the method (and its return value) in question.

    How would you handle this situation if/when you get to rewrite it?

    Similarly, I think... find the common stuff, put it into a private method that the other ones call. Sometimes, it depends on the context ... I might find a way to make the calls even more generic (we have a lot of down-to-the-gnat's-ass-hair methods which perform very specific queries ... which are VERY similar to other queries ...

    I generally push the common stuff lower down the 'stack' and make building blocks from it.

    What I despise is that I'm finding methods which require a fully generated query and then run it and then return all sorts of java.sql objects to the caller (Connection, (Prepared)Statement, and ResultSet) instead of converting the ResultSet into 'domain' objects so the calling programmer can use them immediately.



  • Some other nitpicks, admittedly less severe than what you said:

    • It indexes into the Lists instead of using their iterators. If someone happens to pass a LinkedList, this becomes quadratic.
    • Being vulnerable to SQL injection if the Lists contain something unexpected, like Strings.
    • Building a new empty ArrayLists instead of using Collections.emptyList(). (May not be a WTF if the caller wants to modify the result, though.)


  • @zelmak said:

    Do you still believe/not believe in single entrance/exit?

    I still believe that single exit rule is total bullshit.

    I not only still believe, that single exit rule is total bullshit, but this this function actually proves it. If it was written to the single exit rule, it would have ended up a mile to the right and be another order of magnitude less readable. There are many things wrong with the function, the abuse of ArrayList to make up for lack of tuples1 in Java, but the control flow returning empty result early if something does not exist is not one of them.


    1 This function returns two collections. Many languages have special support for that either using built in tuples (Python, Haskell, ...), library provided tuples (C++ using boost or C++11) or list context (Perl). In any case they allow writing a, b, c = f() or that with a little extra verbiage. Some more languages have output arguments. It's much less readable, but would work here. Now Java has neither, so you either have to create auxiliary type for the return (which is readable, but lot of typing) or resort to returning ArrayList or Object[] like here, which is less typing when creating the function, but more typing when using it (casts needed) and unreadable.



  • @Bulb said:

    This function returns two collections. Many languages have special support for that either using built in tuples (Python, Haskell, ...), library provided tuples (C++ using boost or C++11) or list context (Perl). In any case they allow writing a, b, c = f() or that with a little extra verbiage. Some more languages have output arguments. It's much less readable, but would work here. Now Java has neither, so you either have to create auxiliary type for the return (which is readable, but lot of typing) or resort to returning ArrayList or Object[ like here, which is less typing when creating the function, but more typing when using it (casts needed) and unreadable.

    Tuples are nice, but it's not much effort to create a Pair class, and it's usually useful.

    As to casts being needed: the issue here is that the code is still using Java 1.4 a full 7 years after Java 1.5 was released. The first WTFs I saw were the ungenericised collections.



  •  IIRC single entry/exit referred to Fortran 77 and prior where you could have stuff like this:

     call Subra(101, 102)

     return

    101 do something

     goto 103

    102 do something else

    103

    call Subrb()
    return

    end

     subroutine subra(*,*)

    integer fred

    if (some validation fails) return 1

    if (some other validation fails) return 2

    C This now drops through to subrb

    fred = 1

    goto 20

    entry subrb()

    fred = 2

    20 continue

     do what you wanted in the first place but fred is 1 or 2 depending on which entry point you used

    return

    end

     

    where there's 2 entry points to the code in subra (or is it subrb) and theres about 3 places to where a call to subra can return.

    thats not the same as having multiple return statements at all.



  • @Bulb said:

    I not only still believe, that single exit rule is total bullshit, but this this function actually proves it. If it was written to the single exit rule, it would have ended up a mile to the right and be another order of magnitude less readable.

    All that proves is that they're a bad designer.  Let's rewrite it (I'll use C#):

    public ICollection AnalyzeData(IList frobnitzList, IList filfreList)

    {

       List<Item> data = new List<Item>();

       List<OtherItem> resultData = new List<OtherItem>();

       if(frobtnitzList.Any())

          data = GetFrobnitzData(frobnitzList);

       else if(filfreList.Any())

          data = GetFilfreData(filfreList);

       if(data.Any())

          resultData = GetDataFromMagicalPlace(data);

       return resultData;

    }

      



  •  @Bulb said:

    @zelmak said:

    Do you still believe/not believe in single entrance/exit?

    I still believe that single exit rule is total bullshit.

    ...

    Assuming that a coding standard devised for a particular coding paradigm is magically best practice for all coding paradigms is bullshit; single exits is only bullshit when removed from its context.  I am fairly certian that the single exit rule was designed for C, especially when mannaging memory with malloc()/free() where returing early may cause you to leak memory by skipping the call(s) to free(). A common pattern was to use goto foo_exit; to facilitate a single exit where memory is then freed. In any other context the single exit rule is bullshit.


     



  • @esoterik said:

     @Bulb said:

    @zelmak said:

    Do you still believe/not believe in single entrance/exit?

    I still believe that single exit rule is total bullshit.

    ...

    Assuming that a coding standard devised for a particular coding paradigm is magically best practice for all coding paradigms is bullshit; single exits is only bullshit when removed from its context.  I am fairly certian that the single exit rule was designed for C, especially when mannaging memory with malloc()/free() where returing early may cause you to leak memory by skipping the call(s) to free(). A common pattern was to use goto foo_exit; to facilitate a single exit where memory is then freed. In any other context the single exit rule is bullshit.


     

    The "single exit rule" is also to improve readability. I'm no fan of it (readability that is).

    In general I have 3 types of exits:
    -At the end
    -In the beginning (error checking)
    -When you've found what you where looking for


  • @Daid said:

    -At the end
    -In the beginning (error checking)
    -When you've found what you where looking for
     

    Agreed.



  • @dhromed said:

    @Daid said:

    -At the end

    -In the beginning (error checking)

    -When you've found what you where looking for
     

    Agreed.

    _rand = new Random();

    if( _rand.Next() < 0.5 )

    {

        // what the hell, why not?

        return;

    }



  • @blakeyrat said:

    @dhromed said:

    @Daid said:

    -At the end
    -In the beginning (error checking)
    -When you've found what you where looking for
     

    Agreed.

    _rand = new Random();

    if( _rand.Next() < 0.5 )

    {

        // what the hell, why not?

        return;

    }

     

    Agreed.

     



  • @blakeyrat said:

     _rand = new Random();
    if( _rand.Next() < 0.5 )
    {
        // what the hell, why not?
        return;
    }

    <nagesh>ty for teh codez. you are tru java wizord</nagesh>


  • Discourse touched me in a no-no place

    @zelmak said:

    <nagesh>ty for teh codez. you are tru java wizord</nagesh>

    When I eventually saw it (quoted,) I was thinking 'F-, must troll harder' but your comment seems to embody the thought, plus some...



    That, and 'where would that construct even make any sense in any real-world codebase?' And I gave up coming up with one that didn't fit
    sleep(rand())
    and forgot/couldn't be arsed to post.


Log in to reply