How do you decide between different Java collection or concurrency choices during code review?
Quick Answer
Ask what operations dominate (lookup vs. iteration vs. insertion order vs. sorted order) to pick the right collection, whether the code is genuinely accessed by multiple threads (favoring java.util.concurrent types over ad hoc synchronization), and whether the simplest option (a plain ArrayList/HashMap) is sufficient before reaching for something more specialized. In review, flag concrete failure scenarios (a race condition, an O(n) lookup in a hot loop, a synchronized block that's broader than needed) rather than stylistic preferences.
Detailed Answer
A practical framework for evaluating collection/concurrency choices in review:
Collections:
- What operation dominates? Frequent lookups by key →
HashMap/HashSet; frequent indexed access →ArrayList; frequent insert/remove at both ends →ArrayDeque; need sorted iteration or range queries →TreeMap/TreeSet; need predictable insertion-order iteration →LinkedHashMap/LinkedHashSet. - Is the simplest option actually sufficient? A plain
ArrayList/HashMapis almost always the right starting point; reach for something more specialized only once a real, measured need (not a hypothetical one) justifies the added complexity. - Watch for anti-patterns in review, like
list.contains()inside a loop (O(n) each call, O(n²) overall — should be aSet), or building aTreeMapwhen insertion order (not sorted order) was actually what was needed.
Concurrency:
- Is the data genuinely shared across threads? If not, don't pay for synchronization at all — a plain
ArrayList/HashMapused by a single thread needs no concurrent collection. - If it is shared, is fine-grained concurrent access actually needed, or would coarser external synchronization (a single lock around a whole operation) be simpler and just as correct?
ConcurrentHashMap/CopyOnWriteArrayListearn their complexity when contention or read/write ratios genuinely benefit from it. - Look for classic review red flags: a
synchronizedblock scoped wider than necessary (holding a lock during I/O or another blocking call), a check-then-act race condition (if (!map.containsKey(k)) map.put(k, v);instead ofcomputeIfAbsent), or manualwait/notifywhere ajava.util.concurrentutility would be both simpler and less error-prone.
In review comments specifically, it's most effective to point at a concrete failure scenario ("if two threads call this concurrently, X can happen") rather than a stylistic preference — it makes the reasoning verifiable and the fix obviously worth the effort to the code's author.