Ah, Efficiency: Thou art a goddess who deserves worship, adulation, sacrifice, but thou art also a goddess who gets said worship, adulation, and sacrifice in all the wrong ways. Here's some JDBC code validating a login, trying to be good code (I think!)... that gets it wrong in almost every way possible.

This is real code from a #java person. Thankfully, he was pasting it as an example of the dreck he has to work with... it's not his code!

public boolean validateLogin( String custno, String pincode ) 
    throws Exception {
  if( custno == null || pincode == null )
    return false;

  if( custno.length() == 0 || pincode.length() == 0 )
    return false;

  if( custno.matches(".*[^0-9].*") 
      || pincode.matches(".*[^0-9].*") )
    return false;

  MyResultSet rs = sql.executeQueryTable(
    "select custno from customers where custno = " 
    + custno + " and pincode = " + pincode );

  if( rs.next() ){
    if( rs.next() )
      throw new Exception( "More than one customer "
        + "found with custno " + custno 
        + " and pincode " + pincode  
        + ". Check databasekeys!" );

    return true;
  }

  return false;
}

Now, like I said, this was pasted as real code, not "let me craft some horrible code together" example stuff.

So: let us count the ways...

1. The method throws an Exception. Note that it returns a true or false value, meaning "valid or invalid login" -- if an exception occurs, the login is apparently false! But it throws an Exception anyway... and it's a meaningless exception, forcing methods that call this one to handle generic exceptions.

2. pincode is a String, not a real number. I don't know about you: when I see "pin code," I think of codes like "1234," not "mypassw0rd" -- yet they're using the loosest datatype they can. Bad, bad, bad.

   But wait! It gets worse! The code validates the input... to make sure that it's not null (why not just accept the NullPointerException?), to make sure it's not empty (more on this later), and then... to make sure it's numeric only. If they'd used the right datatypes in the first place, none of the validations would be necessary.

3. But wait! It gets even worser! Note the lack of brackets. Wasting code and CPU time is bad, according to whoever wrote this, but we must save the brackets! In my opinion, brackets - i.e., blocks - are worth using, even if it's a single statement the block contains. This prevents someone sticking a statement at the "end of the block" without realising it's not a block.

   Whoever wrote this - and yes, I'm glad I don't know who you are, really - you wrote a method as defensively as you could, but you wrote it in a defensive way that leaves you wide open for bugs, especially bugs written by lazy maintenance coders. Do better. It'll save you from having to look at this code again later... and let's be real, this is a coding horror, nobody wants to look at it in the first place, much less look at it again.

   On the other hand, if the code is maintained by someone else, maybe they'll clean it up...

4. SQL Injection alert? No, because they do validation of the data before building a SQL query with it... but seeing a statement built this way, with constant fields in it, should really bother anyone. Why "MyResultSet?" Is there some special facility that this requires that ordinary and canonical JDBC cannot handle?

   No.

5. Now, it's not apparent in this code, but this queries a table... that has a primary key based on one of the fields in the query, custno. The code gets the result set, iterates to the first row, and then... checks to see if there's ANOTHER ROW.

   Apparently, in whatever database this code anticipates using, the definition of "primary key" doesn't include the property of uniqueness, which makes it pretty much not a primary key.

Look: if you're doing a check for logins like this, fine. But real code should use a PreparedStatement; it should use the right data types; ... it should look something like this:

/** assume Connection conn is in scope 
 * for the method for clarity
 */
public boolean isValidLogin(int custno, int pincode) {
  PreparedStatement ps=null;
  ResultSet rs=null;
  boolean isValid=false;
  try {
    ps=conn.prepareStatement(
      "select custno from customers "+
      "where custno=? and pincode=?");
    ps.setInt(1, custno);
    ps.setInt(2, pincode);
    rs=ps.executeQuery();
    isValid=rs.next();
  } catch(SQLException e) {
    /* log SQL error on an internal log */
  } finally {
    if(rs!=null) {
      try { rs.close(); } catch(Exception e) {}
    }
    if(ps!=null) {
      try { ps.close(); } catch(Exception e) {}
    }
  }
  return isValid;
}

There's lots of code here, too, but the big difference is that all of it's meaningful in a Java context, instead of wasted CPU time. As it is, it's a terrible idea.

Responses[12]

Posted by Joseph B. Ottinger on November 12, 2008 8:54:00 AM EST #