Java Code Review - Identify Bugs and Smells in a Concurrent Data Structure
Interview Experience
Problem
Review the following Java code for a thread-safe counter. Identify all bugs, concurrency issues, and code quality problems. For each issue, explain the fix.
java
public class SharedCounter {
private int count = 0;
private List<Integer> history = new ArrayList<>();
public void increment() {
count++; // (1)
history.add(count); // (2)
}
public int getCount() {
**return** count; // (3)
}
public List<Integer> getHistory() {
**return** history; // (4)
}
public void reset() {
count = 0;
history.clear(); // (5)
}
}
Issues to find:
- (1) Non-atomic read-modify-write on count.
- (2) ArrayList is not thread-safe; concurrent adds cause data corruption.
- (3) Stale read - no visibility guarantee without volatile or lock.
- (4) Returning mutable reference leaks internal state.
- (5) Non-atomic compound reset allows torn reads between count=0 and history.clear().
Follow-ups
- Rewrite using
AtomicIntegerandCopyOnWriteArrayList. What are the tradeoffs? - When would you use
synchronizedvsReentrantLockvsAtomicInteger? - How would you write a unit test that reliably exposes the race condition in the original code?
- Describe a scenario where
CopyOnWriteArrayListperforms poorly.
Full Details
Problem
Review the following Java code for a thread-safe counter. Identify all bugs, concurrency issues, and code quality problems. For each issue, explain the fix.
java
public class SharedCounter {
private int count = 0;
private List<Integer> history = new ArrayList<>();
public void increment() {
count++; // (1)
history.add(count); // (2)
}
public int getCount() {
**return** count; // (3)
}
public List<Integer> getHistory() {
**return** history; // (4)
}
public void reset() {
count = 0;
history.clear(); // (5)
}
}
Issues to find:
- (1) Non-atomic read-modify-write on count.
- (2) ArrayList is not thread-safe; concurrent adds cause data corruption.
- (3) Stale read - no visibility guarantee without volatile or lock.
- (4) Returning mutable reference leaks internal state.
- (5) Non-atomic compound reset allows torn reads between count=0 and history.clear().
Follow-ups
- Rewrite using
AtomicIntegerandCopyOnWriteArrayList. What are the tradeoffs? - When would you use
synchronizedvsReentrantLockvsAtomicInteger? - How would you write a unit test that reliably exposes the race condition in the original code?
- Describe a scenario where
CopyOnWriteArrayListperforms poorly.
About This Question
This is a candidate experience report from a grammarly interview during the onsite round.
It covers the following topics: Code Review, Coding, Onsite .