3
\$\begingroup\$

This is a Spring Framework-based datasource instantiator which reads its properties from the Spring Framework context, i.e. the values are initially all String from properties files and numbers have to be tested for presence and validity.

Normally it would be totally superfluous in Spring Framework because the purpose of Spring Framework is to allow Java developers to by-pass it all. However sometimes as in this case, something is different and it has to be done manually.

My main concern with this method is the repeated use of the couldBe variable which is an attempt to make it more readable.

couldBe is the possibly hopefully valid number retrieved from the properties file, which could in fact be invalid.

This is all that is in the class:

private final Pattern testForInt = 
    Pattern.compile(GeneralUtils.REGEX_TEST_FOR_INT);

@Bean
@ConfigurationProperties(prefix = "tardis.datasource")
public Map<String, String> dataSourceProperties() {
    // Spring populates the map with properties prefixed as above
    return new HashMap<>();
}

@Bean
public DataSource dataSource() {
    Map<String, String> props = dataSourceProperties();
    HikariConfig dataSourceConfig = new HikariConfig();
    dataSourceConfig.setDriverClassName(props.get("driver"));
    dataSourceConfig.setUsername(props.get("username"));
    dataSourceConfig.setPassword(props.get("password"));
    dataSourceConfig.setJdbcUrl(props.get("url"));
    String couldBe = props.get("maximumPoolSize");
    if (couldBe != null && testForInt.matcher(couldBe).find()) {
        dataSourceConfig.setMaximumPoolSize(Integer.valueOf(couldBe));
    }
    couldBe = props.get("maxLifeTime");
    if (couldBe != null && testForInt.matcher(couldBe).find()) {
        dataSourceConfig.setMaxLifetime(Long.valueOf(couldBe));
    }
    couldBe = props.get("leakDetectionThreshold");
    if (couldBe != null && testForInt.matcher(couldBe).find()) {
        dataSourceConfig.setLeakDetectionThreshold(Long.valueOf(couldBe));
    }
    couldBe = props.get("connectionTimeout");
    if (couldBe != null && testForInt.matcher(couldBe).find()) {
        dataSourceConfig.setConnectionTimeout(Long.valueOf(couldBe));
    }
    return new HikariDataSource(dataSourceConfig);
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! This question lacks context. Please tell us what this code does, and why it exists. Also, the title should state the purpose of the code, rather than your main concern about it. (See How to Ask.) \$\endgroup\$ – 200_success May 17 '17 at 14:13
  • \$\begingroup\$ OK, that was unexpected! Just goes to show you should always rtfm, or in this case the How To Ask a Good Question. Lucky I got 100 points free :O \$\endgroup\$ – Adam May 17 '17 at 15:00
  • \$\begingroup\$ It's a common thing to provide a separate Interface for a specific Configuration (so you can switch implementation and mock it easier in test cases). So I recommend to provide a DatasourceConfiguration interface with int getMaxLifeTime() and do the magic within the implementation (e.g. a private getInt(String property, Integer default) method). \$\endgroup\$ – slowy May 17 '17 at 16:53
  • \$\begingroup\$ That would certainly clean up this code, and I'm wondering why your comment isn't an answer (I'm new here). Also, it occurs to me that I already have a specific Configuration - HikariConfig - so I need to let Spring initialise that, instead of the map, by changing all my property names - as long as that bit of magic with the property prefix annotation works. \$\endgroup\$ – Adam May 18 '17 at 9:08
  • \$\begingroup\$ Actually if I let Spring automatically initialise HikariConfig I suspect it will silently fail to initialise int values when the property value is invalid. \$\endgroup\$ – Adam May 19 '17 at 8:35

Your Answer

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

Browse other questions tagged or ask your own question.