2
\$\begingroup\$

Following approach uses more Functional approach (non OOP), each method is passed DataSource as dependency.

I've written this piece and I'm wondering if there's something that can be improved. I wish to add ResultSet to try block but not finding any option. Is there a way?

Also please review the class MySQL.

public static Iterator<StateEntity> findAllPrepared(DataSource dataSource, int page, int size) throws SQLException {
    List<StateEntity> list = new ArrayList<>();
    ResultSet rs = null;
    String sql = "SELECT * FROM states LIMIT ?, ?";
    try (
        Connection connection = dataSource.getConnection();
        PreparedStatement statement = connection.prepareStatement(sql, ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_READ_ONLY);
    ) {
        statement.setInt(1, page);
        statement.setInt(2, size);
        rs = statement.executeQuery();
        while(rs.next()) {
            list.add(new StateEntity(rs.getString("stateId"), rs.getString("stateName")));
        }
    } finally {
        if(rs != null) rs.close();
    }
    return list.iterator();
}

StateEntity

public class StateEntity {

    private String id;
    private String name;

    public StateEntity(String id, String name) {
        this.id = id;
        this.name = name;
    }

    public String getId() {
        return id;
    }

    public String getName() {
        return name;
    }
}

SQL Connection

public class MySQL {

    public static MysqlDataSource dataSource = new MysqlDataSource();

    private static Map<String, String> parameters = new HashMap<String, String>(){
        {
            put("authReconnect", "true");
            put("useSSL", "false");
            put("allowMultiQueries", "true");
            put("useUnicode", "true");
            put("useJDBCCompliantTimezoneShift", "true");
            put("useLegacyDatetimeCode", "false");
            put("serverTimezone", "UTC");
        }
    };

    private static Map<String, String> credentials = new HashMap<String, String>(){
        {
            put("host", System.getenv("HOST") != null ? System.getenv("HOST") : "localhost");
            put("schema", System.getenv("SCHEMA") != null ? System.getenv("SCHEMA") : "usa");
            put("username", System.getenv("USERNAME") != null ? System.getenv("USERNAME") : "root");
            put("password", System.getenv("PASSWORD") != null ? System.getenv("PASSWORD") : "mypass");
        }
    };

    static {
        String query = parameters.entrySet().stream()
                .map(p -> p.getKey() + "=" + p.getValue())
                .reduce((p1, p2) -> p1 + "&" + p2)
                .orElse("");

        dataSource.setURL("jdbc:mysql://" + credentials.get("host")+":3306/" + credentials.get("schema") + "?" + query);
        dataSource.setUser(credentials.get("username"));
        dataSource.setPassword(credentials.get("password"));
    }
}

Test

@Test
public void testGetAllPrepared() throws SQLException {
    Iterator<StateEntity> states = State.findAllPrepared(MySQL.dataSource, 0, 5);
    long count = 0;
    while (states.hasNext()) {
        count++;
        StateEntity stateEntity = states.next();
        System.out.println(stateEntity.getId());
        assertNotNull(stateEntity);
        assertNotNull(stateEntity.getId());
        assertNotNull(stateEntity.getName());
        assertEquals(2, stateEntity.getId().length());
    }
    assertEquals(5, count);
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Starting out with your first question: no, there isn't. But you may put the result set into another nested try block. Still not super-elegant, but a bit better in my opinion:

try (
    Connection connection = dataSource.getConnection();
    PreparedStatement statement = connection.prepareStatement(sql, ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_READ_ONLY);
) {
    statement.setInt(1, page);
    statement.setInt(2, size);
    try (ResultSet rs = statement.executeQuery()) {
        while(rs.next()) {
            list.add(new StateEntity(rs.getString("stateId"), rs.getString("stateName")));
        }
    }

Regarding the method itself: why do you return an iterator instead of the list itself? This severely limits the capabilities of the return value without any benefit. (E.g. you cannot use size() or stream() which you could do with the list.)

Regarding the MySql class now:

Are you aware that you actually create an anonymous subclass with the map construction using curly braces and an object initializer? I never found this on par with the problem solved here. Fortunately, Java 9 brought us Map.of, so I recommend replacing this construct with

private static Map<String, String> parameters = Map.of(
    "authReconnect", "true",
    ...
);

Regarding the query = ...stream...reduce...orElse: you can achive the same by simply using a joining collector:

String query = parameters.entrySet().stream()
    .map(p -> p.getKey() + "=" + p.getValue())
    .collect(Collectors.joining("&"));

The MySql class in itself is just global variables. This is "somewhat ok" as long as you don't employ advanced dependency injection possibilities like spring or a CDI container, but keep in mind that these globals are generally regarded as bad. Advice: read up on dependency injection, and see what options there are.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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