GetAllCastingWTFs()



  • public static Member[ ] getAllMembersProject(int project_id) throws DBException
    {
    Vector v = new Vector();
    Member m = null;

    try
    {
    String query = "SELECT UserInfo.id, UserProject.user_id, UserInfo.first_name FROM UserProject, UserInfo WHERE project_id = " + project_id + " AND UserProject.user_id = UserInfo.id";
    RecordSet rs = DB.DoQuery(query);

    while (rs.next())
    {
    m = getMember(rs.getInt("user_id"));
    if (m != null)
    v.add(m);
    }
    return (Member[ ])v.toArray();

    }
    catch (Exception e)
    {
    Debugger.log(Debugger.ERROR, "getAllMembersProject", e);
    throw new DBException("getAllMembersProject " + e.toString());
    }
    }

    Found in an internal issue-tracking system originally written in Java 1.3. Upon migration of the project to 1.6, this threw an exception about being unable to cast Object[] to Member[] (line 17).

    RecordSet is a custom extension of ResultSet. Yes, it does have a .size() method (or, technically, .getNumOfRecords()).

    Oh, and while I'm thinking of it, why yes, getMember() does happen to execute another query for each member the initial query returns.



  • Re: getAllCastingWTFs

    @Mr. DOS said:

    public static Member[ getAllMembersProject(int project_id) throws DBException
    {
    Vector v = new Vector();
    Member m = null;

    try
    {
    String query = "SELECT UserInfo.id, UserProject.user_id, UserInfo.first_name FROM UserProject, UserInfo WHERE project_id = " + project_id + " AND UserProject.user_id = UserInfo.id";
    RecordSet rs = DB.DoQuery(query);

    while (rs.next())
    {
    m = getMember(rs.getInt("user_id"));
    if (m != null)
    v.add(m);
    }
    return (Member[)v.toArray();

    }
    catch (Exception e)
    {
    Debugger.log(Debugger.ERROR, "getAllMembersProject", e);
    throw new DBException("getAllMembersProject " + e.toString());
    }

    }

    Found in an internal issue-tracking system originally written in Java 1.3. Upon migration of the project to 1.6, this threw an exception about being unable to cast Object[ to Member[ (line 17).

    RecordSet is a custom extension of ResultSet. Yes, it does have a .size() method (or, technically, .getNumOfRecords()).

    Oh, and while I'm thinking of it, why yes, getMember() does happen to execute another query for each member the initial query returns.

    I initially assumed that "Member[" was a typo, but then you repeated it 3 more times throughout the post, so now I have to wonder...is "[" a valid character in a Java class identifier now?

    Or did CS fuck things up?



  • @toth said:

    I initially assumed that "Member[" was a typo, but then you repeated it 3 more times throughout the post, so now I have to wonder...is "[" a valid character in a Java class identifier now?

    Or did CS fuck things up?

    That would be CS, and I've no idea how to fix it, especially seeing as it looks just dandy in the editor.

    Edit: Have some spaces between your brackets.



  • @Mr. DOS said:

    public static Member[ ] getAllMembersProject(int project_id) throws DBException
    {
    Vector v = new Vector();
    Member m = null;

    try
    {
    String query = "SELECT UserInfo.id, UserProject.user_id, UserInfo.first_name FROM UserProject, UserInfo WHERE project_id = " + project_id + " AND UserProject.user_id = UserInfo.id";
    RecordSet rs = DB.DoQuery(query);

    while (rs.next())
    {
    m = getMember(rs.getInt("user_id"));
    if (m != null)
    v.add(m);
    }
    return (Member[ ])v.toArray();

    }
    catch (Exception e)
    {
    Debugger.log(Debugger.ERROR, "getAllMembersProject", e);
    throw new DBException("getAllMembersProject " + e.toString());
    }
    }

    Found in an internal issue-tracking system originally written in Java 1.3. Upon migration of the project to 1.6, this threw an exception about being unable to cast Object[ to Member[ (line 17).

    RecordSet is a custom extension of ResultSet. Yes, it does have a .size() method (or, technically, .getNumOfRecords()).

    Oh, and while I'm thinking of it, why yes, getMember() does happen to execute another query for each member the initial query returns.

    So, is your problem that the original developer used a while instead of a for?  Using .next in a while allows the system to stream the records in small batches, any access to .getNumOfRecords will block until the entire result set has been loaded.

    As for the one-query-per-member issue, this pattern is sometimes a good tradeoff of performance for maintainability.  This allows the developer to avoid creating a version of getMember that operates on multiple members as well as avoiding leaking the member data impementation into this method.

    My biggest problem with this method would be that the invalid cast exception was re-thrown as a DBException, but the problem wasn't in the database.



  •  Actually, my problem was shoving it all into a Vector then casting it to an array as opposed to creating a Member array right off the bat based on the size of the ResultSet.



  • It's the freaky BBcode parser. An empty BBcode, i.e []] is the way to tell CS to echo a "[]". So, use []]] to get a []] in the post.



  •  TRWTF: Community Server “supporting” BBCode even though it also supports HTML formatting.



  • The true wtf is: Using statement instead of preparedStatement.

    Calling getMember with a new query, instead of just using a join.

    Implementing your own getMember methods(And all the other similary methods) instead of using an oo mapper such as torque or Hibernate.

    (And yes, returning an array instead of a List is stupid too).



  • @tiller said:

    The true wtf is: Using statement instead of preparedStatement.

    Not a WTF.

    @tiller said:


    Calling getMember with a new query, instead of just using a join.

    I don't know about you, but I'm a Java programmer, not an SQL one. You don't know what kind of process goes under getMember.

    @tiller said:


    Implementing your own getMember methods(And all the other similary methods) instead of using an oo mapper such as torque or Hibernate.

     

    Haven't you read the part that says written in Java 1.3 ? There was no Hibernate.

    @tiller said:

     

    (And yes, returning an array instead of a List is stupid too).

     

    Why is that?

    I can understand most of the errors this person made (probably a junior programmer) since I also made most of them when being a junior programmer, but no WTFs here. Except for the Exception rethrow, which I keep seeing every day even when working with many years experienced programmers.



  •  The real wtf is

     

    1. Using Vector.  Which is unecessarily synchronized and would have long been marked deprecated if it werent in ME specs...   -> use ArrayList  instead and if you need synchronizations there is a Collections method for creating that..

     

    2. be aware Collections don't have a type for their content at runtime... this means an Vector/ArrayList will contain an Object[]] array... you can't cast that one simply up  to a Member arra. (Java is strong typed!)

     use .toArray(new Member[]]{})   instead



  • @ubersoldat said:

    I can understand most of the errors this person made (probably a junior programmer) since I also made most of them when being a junior programmer, but no WTFs here. Except for the Exception rethrow, which I keep seeing every day even when working with many years experienced programmers.
    Not even that is a real WTF, since the original exception is being logged. Why would this method's caller bother with anything more than DBExceptions?



  • @Zecc said:

    @ubersoldat said:
    I can understand most of the errors this person made (probably a junior programmer) since I also made most of them when being a junior programmer, but no WTFs here. Except for the Exception rethrow, which I keep seeing every day even when working with many years experienced programmers.
    Not even that is a real WTF, since the original exception is being logged. Why would this method's caller bother with anything more than DBExceptions?
    I agree.  Logging and rethrowing the same exception is pretty questionable (you should instead just check the stack trace where it's dealt with), but catching exceptions and wrapping them as API-specific exceptions is a very reasonable thing to do.  Mind you, the exception here isn't fully wrapped, but Java 1.3 didn't support chained exceptions.  Logging it (with the stack trace) and appending the message to a new exception would be the next best thing.

    (Not that "method name"+e.toString() is all too great, the method name is already in the stack trace; but it's still 1000x better to declare the method as throwing MySpecificException rather than any old Exception.)



  • @Zecc said:

    @ubersoldat said:

    I can understand most of the errors this person made (probably a junior programmer) since I also made most of them when being a junior programmer, but no WTFs here. Except for the Exception rethrow, which I keep seeing every day even when working with many years experienced programmers.
    Not even that is a real WTF, since the original exception is being logged. Why would this method's caller bother with anything more than DBExceptions?

    They probably wouldn't bother handling anything more than a DBException, but don't you think that if the method encountered, say, an IndexOutOfRangeException (not saying I see that happening in this particular case, just an example), it should go to the caller as an IndexOutOfRangeException, rather than a DBException with a message saying "Index was out of range" or whatever? I think the issue here is that the method is catching a generic exception and coercing it to a DBException, whereas it would be better to catch an exception that actually indicated an error in the DB interaction and throw that as a DBException, letting any other exception go through unhandled to either be handled by the caller or be investigated and fixed? Isn't that basic exception practice?



  • Since the native java.sql.ResultSet interface doesn't have a size() method then it's fairly standard practice to use a collection to store the results until the entire set has been retrieved and then convert that collection to an array. Perhaps the original developer simply wasn't aware that the custom extension made available a size()/getNumRecords() method.

    The throwing of the exception upon trying to cast the results of the Vector.toArray() to an array of Member[] would be expect. You simply need to initialize an empty array of Member objects and pass it into the Vector.toArray(Object[]) method (example: v.toArray(new Member[v.size()])). The WTF is that this was standard beharior with Java 1.3, so I would expect a ClassCastException to have been thrown with each call to this method in all cases.

    The use of a PreparedStatement would have been preferable to a Statement in this case. Unless a developer has a real good reason for not using a PreparedStatement, then they really should be using PreparedStatement objects for a variety of reasons - security and performance being the two main benefits.



  • @DeepThought said:

    Perhaps the original developer simply wasn't aware that the custom extension made available a size()/getNumRecords() method.
    The original developer wrote the custom extension. That's probably a whole 'nother set of WTFs, but I'm a little scared of peeking inside it.

    @DeepThought said:

    The throwing of the exception upon trying to cast the results of the Vector.toArray() to an array of Member[]] would be expect.
    The use of a Vector was the original WTF I was trying to point out. It seems to have gone largely ignored, though...

    @DeepThought said:

    The use of a PreparedStatement would have been preferable to a Statement in this case.
    It would've been preferable here, as it would've been through most of the rest of the 5,288-line DBUtil class.



  • @Mr. DOS said:

    @DeepThought said:

    The throwing of the exception upon trying to cast the results of the Vector.toArray() to an array of Member[] would be expect.
    The use of a Vector was the original WTF I was trying to point out. It seems to have gone largely ignored, though...

    It's gone largely ignored because it isn't that big of a WTF.  As mentioned above, it's pretty standard practice to build a collection and then convert it to an array.  Memory usage is a non-issue for several reasons:

    1. Building a collection and converting it to an array creates two memory structures, both with n elements, where n is the number of members returned.  However, directly creating an array and populating it also create 2n memory allocations.  One for the array, and another for the RecordSet that has to be fully populated in memory so it can be counted.  With the collection strategy, the RecordSet can be streamed (assuming it is created as a forward-only RecordSet, which is the default ResultSet).
    2. Returning an array of members isn't the pattern you'd use when you planned on the number of members being large enough to cause memory problems.

    So, the lack of memory optimization in the method actually matches the lack of memory optimization in the RecordSet design and the method signature.  Personally, I'm OK with it.  A lot of people like to think about performance above all else, but I'm a big fan of avoiding premature optimization.  The code, as written, is a good example of how to write readable and maintainable code if memory constraints are not an issue.  Of course the code isn't perfect, like the strange choice of collection type and other minor issues mentioned in this thread.


Log in to reply