1
\$\begingroup\$

I have two methods which are very similar apart from the method call inside and the return type. I wondered if there was a way to make it neater, i.e not duplicate the boiler plate connection code. I considered putting a flag in the method signature to switch, something like isMultiple but in the end I didn't like this and as the return type is different that won't work. I wondered if there was something nice I could do with Java 8.

Methods:

protected String getSingleFilePathResult(Query query) throws UwsException {
      JdbcPooledConnection conn = createConnection();
      try {
         return conn.getSingleFilePathResult(query);
      } catch (SQLException sqle) {
         throw new UwsException(SQL_EXCEPTION + sqle.getMessage(), sqle);
      } finally {
         closeConnection(conn);
      }
   }

protected List<String> getMultipleFilePathResults(Query query) throws UwsException {
      JdbcPooledConnection conn = createConnection();
      try {
         return conn.getMultipleFilePathResults(query);
      } catch (SQLException sqle) {
         throw new UwsException(SQL_EXCEPTION + sqle.getMessage(), sqle);
      } finally {
         closeConnection(conn);
      }
   }
\$\endgroup\$
  • \$\begingroup\$ Please show the relevant JdbcPooledConnection methods as well. \$\endgroup\$ – 200_success Aug 4 '17 at 5:42
2
\$\begingroup\$

You can factor out the boilerplate like so:

protected String getSingleFilePathResult(Query query) throws UwsException {
    return doWithConnection(conn -> conn.getSingleFilePathResult(query));
}

protected List<String> getMultipleFilePathResults(Query query) throws UwsException {
    return doWithConnection(conn -> conn.getMultipleFilePathResults(query));
}


protected <T> T doWithConnection(Function<conn, T> callback) throws UwsException {
    try (JdbcPooledConnection conn = createConnection()){
        return callback.Invoke(conn);
    } catch (SQLException sqle) {
        throw new UwsException(SQL_EXCEPTION + sqle.getMessage(), sqle);
    }
}
\$\endgroup\$
1
\$\begingroup\$

As Mario already wrote it is hard to make that code less duplicate without increasing complexity, for example you can do it like this:

interface PathResult {
List<String> getResult(JdbcPooledConnection conn, Query query) throws SQLException;
}

class MultiplePathResult implements PathResult {

    @Override
    public List<String> getResult(JdbcPooledConnection conn, Query query) throws SQLException {
        return conn.getMultipleFilePathResults(query);
    }
}

class SinglePathResult implements PathResult {

    @Override
    public List<String> getResult(JdbcPooledConnection conn, Query query) throws SQLException {
        return conn.getSingleFilePathResult(query);
    }
}

And then you would have one method getPathResults(Query query, PathResult pathResult)

protected List<String> getPathResults(Query query, PathResult pathResult) throws UwsException {
    JdbcPooledConnection conn = createConnection();
    try {
        return pathResult.getResult(conn, query);
    } catch (SQLException sqle) {
        throw new UwsException(SQL_EXCEPTION + sqle.getMessage(), sqle);
    } finally {
        closeConnection(conn);
    }
}
\$\endgroup\$
0
\$\begingroup\$

I think your code is pretty clear and straight forward. That a good thing for whom have to read and understand it.

Without a bit of more context I could just assume your class is a wrapper of the database, and it's used inside your code to perform queries just avoiding the database dependencies.

Made that code less duplicate is pretty hard, without increasing his complexity and readability, I think.

What you could sacrifice is just a bit of memory, but wrapping the getMultipleFilePathResults(Query query) in your getSingleFilePathResult(Query query) like:

protected String getSingleFilePathResult(Query query) throws UwsException {
    List<String> result = getMultipleFilePathResults(Query query);
    if (result.size() == 1) {
        return result.get(0);
    } else {
        // have to handle this...
    }
}

But as you see things are not so clear, as now you use much memory to allocate a List and you have a new case to handle.

\$\endgroup\$
0
\$\begingroup\$

Hmm... I'm missing a think

Is it realy important to get a single result and not a list? If it is, an exception should be thrown when you get more than one result and you asked for only one.

If it is not important - simply use the list function.

And if you just need any of the results in the list, than write a method that gets the list and returns the first one of the list. All the Exception handling stays in the list function.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.