Review a Java Order Service

1. JDBC resources are never closed

Critical, line 14-16

The connection, statement and result set are not closed, even on success. Each call leaks a pooled connection; under load the pool is exhausted within minutes and every request hangs waiting for a connection.

Fix: Use try-with-resources for Connection, PreparedStatement and ResultSet.

2. HashMap shared across threads

Major, line 3, 12, 18

HashMap is not thread-safe. Concurrent puts can corrupt it (lost entries, and in old JDKs infinite loops). containsKey then get is also a check-then-act race. The cache also grows forever and never sees updates to orders.

Fix: Use ConcurrentHashMap.computeIfAbsent, or better a bounded cache with expiry (Caffeine), and invalidate on writes.

3. Shared SimpleDateFormat

Major, line 4, 26

SimpleDateFormat keeps internal state and is not thread-safe; concurrent calls produce wrong dates or exceptions.

Fix: Use java.time with the immutable, thread-safe DateTimeFormatter.ISO_LOCAL_DATE.

4. Double-checked locking without volatile

Major, line 5, 30-35

Without volatile, another thread can see a non-null prices reference before the object is fully constructed (the Java Memory Model allows the reordering).

Fix: Declare the field volatile, or use the holder-class idiom, or load it eagerly in the constructor.

5. Exceptions swallowed

Major, line 20-22

Every failure (SQL error, pool timeout, bug) becomes null, which looks the same as "order not found" and is often cached. Nothing is logged.

Fix: Catch SQLException, log it, and rethrow a meaningful exception; do not cache failures.

6. String-built SQL and SELECT *

Minor, line 16

Concatenation prevents statement caching and is an injection habit, even with a long. SELECT * breaks when columns change.

Fix: Use a PreparedStatement with ? and name the columns.

Java reviews for senior roles focus on concurrency in singletons: shared mutable collections, non-thread-safe utility objects, safe publication (volatile), and resource management. Leaked connections are the bug most likely to cause an outage, so lead with it.

public class OrderService {
    private static final DateTimeFormatter FORMAT = DateTimeFormatter.ISO_LOCAL_DATE;
    private static final Logger log = LoggerFactory.getLogger(OrderService.class);

    private final DataSource dataSource;
    private final Cache<Long, Order> cache = Caffeine.newBuilder()
        .maximumSize(10_000).expireAfterWrite(Duration.ofMinutes(5)).build();
    private volatile PriceTable prices;

    public OrderService(DataSource dataSource) {
        this.dataSource = dataSource;
    }

    public Optional<Order> find(long id) {
        return Optional.ofNullable(cache.get(id, this::load));
    }

    private Order load(long id) {
        String sql = "SELECT id, customer_id, total, created_at FROM orders WHERE id = ?";
        try (Connection conn = dataSource.getConnection();
             PreparedStatement st = conn.prepareStatement(sql)) {
            st.setLong(1, id);
            try (ResultSet rs = st.executeQuery()) {
                return rs.next() ? Order.from(rs) : null;
            }
        } catch (SQLException e) {
            log.error("Could not load order {}", id, e);
            throw new DataAccessException("Could not load order " + id, e);
        }
    }

    public String label(Order order) {
        return order.getId() + " / " + FORMAT.format(order.getCreatedAt());
    }

    public PriceTable prices() {
        PriceTable p = prices;
        if (p == null) {
            synchronized (this) {
                p = prices;
                if (p == null) prices = p = PriceTable.load();
            }
        }
        return p;
    }
}