Chapter 5: Debugging & Code Review
Objective
These exercises simulate interview questions where you are presented with buggy or poorly written code and asked to find the problems and fix them. This is a very popular format at large companies (Sonar/Codility style).
Exercise 1 — Find and fix 4 bugs
"This code has 4 bugs. Find them and fix them."
The buggy code
public class DiscountCalculator {
public double calculateDiscount(List<Product> products, String coupon) {
double total = 0;
for (int i = 0; i <= products.size(); i++) { // BUG 1
Product p = products.get(i);
if (p.getPrice() > 0) {
total =+ p.getPrice(); // BUG 2
}
}
double discount = 0;
if (coupon == "SAVE20") { // BUG 3
discount = total * 0.20;
}
return total - discount; // BUG 4: no null check
}
}
The fixed code
public class DiscountCalculator {
public double calculateDiscount(List<Product> products, String coupon) {
if (products == null || products.isEmpty()) return 0; // FIX 4
double total = 0;
for (int i = 0; i < products.size(); i++) { // FIX 1: < instead of <=
Product p = products.get(i);
if (p.getPrice() > 0) {
total += p.getPrice(); // FIX 2: += instead of =+
}
}
double discount = 0;
if ("SAVE20".equals(coupon)) { // FIX 3: .equals() instead of ==
discount = total * 0.20;
}
return total - discount;
}
}
Explanation of the bugs
| Bug | Problem | Consequence | Fix |
|---|---|---|---|
| 1 | <= instead of < | ArrayIndexOutOfBoundsException | Change to < |
| 2 | =+ instead of += | =+ assigns +value (positive), does not accumulate | Change to += |
| 3 | == compares references | Two identical String may have different references | Use .equals() |
| 4 | No null check | NullPointerException if products is null | Add a guard clause |
Interviewers' favorite pitfalls
=+vs+=: One assigns, the other adds. Very subtle visually.==vs.equals(): In Java,==compares memory addresses, not content.- Off-by-one (
<=vs<): The most classic error in programming.
Interview phrase
"I found four bugs: off-by-one error in the loop, assignment instead of addition, reference comparison instead of .equals(), and missing null safety. These are classic Java pitfalls."
Exercise 2 — Complete code review
"Review this code. What would you improve?"
The code to review
public class DataProcessor {
public static ArrayList<String> process(ArrayList<String> data) {
ArrayList<String> result = new ArrayList<>();
for (int i = 0; i < data.size(); i++) {
String s = data.get(i);
if (s != null) {
if (s.length() > 0) {
if (!s.equals("SKIP")) {
result.add(s.toUpperCase());
}
}
}
}
return result;
}
}
The improved code
public class DataProcessor {
public List<String> process(List<String> data) {
if (data == null) return List.of();
return data.stream()
.filter(Objects::nonNull)
.filter(s -> !s.isEmpty())
.filter(s -> !"SKIP".equals(s))
.map(String::toUpperCase)
.toList();
}
}
All identified problems
| # | Problem | Fix | Principle |
|---|---|---|---|
| 1 | ArrayList in signature | Use List (interface) | Program to interface |
| 2 | static method | Remove static | Testability (mocking) |
| 3 | No guard clause for null | Add if (data == null) return List.of() | Null safety |
| 4 | Nested if (3 levels) | Stream pipeline | Readability |
| 5 | s.equals("SKIP") | "SKIP".equals(s) | Avoid NPE |
| 6 | s.length() > 0 | s.isEmpty() | Idiomatic code |
| 7 | Mutable ArrayList returned | toList() returns an immutable list | Immutability |
Interview phrase
"I'd refactor to use streams for readability, program to interfaces, and add null safety. This is exactly the kind of code Sonar would flag — nested conditionals and mutable state."
Exercise 3 — Find the memory leak
"This code causes a memory leak. Find it."
The problematic code
@Service
public class CacheService {
private final Map<String, Object> cache = new HashMap<>();
public void put(String key, Object value) {
cache.put(key, value);
}
public Object get(String key) {
return cache.get(key);
}
// No method to clear the cache!
// The Map grows indefinitely → OutOfMemoryError
}
The fixed code
@Service
public class CacheService {
private final Map<String, Object> cache = new LinkedHashMap<>() {
@Override
protected boolean removeEldestEntry(Map.Entry<String, Object> eldest) {
return size() > 1000; // Limit to 1000 entries
}
};
// Or better, use Caffeine/Redis
}
Even better: use Spring Cache
@Service
public class ProductService {
@Cacheable(value = "products", key = "#id")
public Product findById(Long id) {
return productRepository.findById(id).orElseThrow();
}
@CacheEvict(value = "products", key = "#id")
public void delete(Long id) {
productRepository.deleteById(id);
}
}
Interview phrase
"A HashMap cache without eviction policy is a memory leak waiting to happen. I'd use Spring Cache with Caffeine or Redis, which handle TTL and max size automatically."
Exercise 4 — Find the concurrency issue
"Is this code thread-safe? Find the problem."
The problematic code
@Service
public class CounterService {
private int count = 0;
public void increment() {
count++; // NOT thread-safe!
}
public int getCount() {
return count;
}
}
Problem: count++ is not atomic. In reality, it's 3 operations:
- Read
count - Add 1
- Write the new value
If two threads do count++ at the same time, an increment can be lost.
The solutions
// Solution 1: AtomicInteger
@Service
public class CounterService {
private final AtomicInteger count = new AtomicInteger(0);
public void increment() {
count.incrementAndGet();
}
public int getCount() {
return count.get();
}
}
// Solution 2: synchronized
@Service
public class CounterService {
private int count = 0;
public synchronized void increment() {
count++;
}
public synchronized int getCount() {
return count;
}
}
Interview phrase
"count++ is not atomic — it's read-modify-write. I'd use AtomicInteger for lock-free thread safety, or synchronized if I need to protect a larger critical section."
Exercise 5 — Fix poor exception handling
"What's wrong with this exception handling?"
The bad code
@Service
public class PaymentService {
public void processPayment(PaymentRequest req) {
try {
gateway.charge(req.getAmount());
orderService.updateStatus(req.getOrderId(), "PAID");
} catch (Exception e) {
// PROBLEM 1: catch too broad
// PROBLEM 2: swallows the exception silently
// PROBLEM 3: useless log
System.out.println("Error: " + e.getMessage());
}
}
}
The fixed code
@Service
@Slf4j
public class PaymentService {
@Transactional
public void processPayment(PaymentRequest req) {
try {
gateway.charge(req.getAmount());
} catch (PaymentDeclinedException e) {
log.warn("Payment declined for order {}: {}", req.getOrderId(), e.getMessage());
throw new BusinessException("Payment was declined", e);
} catch (GatewayTimeoutException e) {
log.error("Gateway timeout for order {}", req.getOrderId(), e);
throw new ServiceUnavailableException("Payment gateway is down", e);
}
orderService.updateStatus(req.getOrderId(), "PAID");
}
}
Identified problems
| # | Problem | Why it's serious |
|---|---|---|
| 1 | catch (Exception e) | Catches everything, including NullPointerException, OutOfMemoryError |
| 2 | Swallowed exception | Caller never knows payment failed |
| 3 | System.out.println | No structured log, no stack trace, lost in production |
| 4 | No rollback | If charge succeeds but updateStatus fails, inconsistent data |
Interview phrase
"Never catch generic Exception. Catch specific exceptions, log with proper severity (warn vs error), re-throw as business exceptions, and use @Transactional for data consistency."
Exercise 6 — Identify code smells
"List all the problems in this code."
The code to analyze
public class Utils {
public static String x(String s) {
String r = "";
for (int i = s.length() - 1; i >= 0; i--) {
r = r + s.charAt(i);
}
return r;
}
public static boolean check(String email) {
if (email.contains("@") == true) {
if (email.contains(".") == true) {
return true;
} else {
return false;
}
} else {
return false;
}
}
public static int calc(int a, int b, String op) {
if (op == "+") return a + b;
if (op == "-") return a - b;
if (op == "*") return a * b;
if (op == "/") return a / b;
return 0;
}
}
The problems
| Method | Problems |
|---|---|
x() | Unclear name, concatenation in loop (O(n²)), no null check |
check() | == true redundant, if/else return true/false replaceable by expression |
calc() | == instead of .equals(), division by zero, silently returns 0 |
The fixed code
public class StringUtils {
public static String reverse(String input) {
if (input == null) return null;
return new StringBuilder(input).reverse().toString();
}
}
public class EmailValidator {
public static boolean isValid(String email) {
return email != null && email.contains("@") && email.contains(".");
}
}
public class Calculator {
public static int calculate(int a, int b, String operator) {
return switch (operator) {
case "+" -> a + b;
case "-" -> a - b;
case "*" -> a * b;
case "/" -> {
if (b == 0) throw new ArithmeticException("Division by zero");
yield a / b;
}
default -> throw new IllegalArgumentException("Unknown operator: " + operator);
};
}
}
Code review checklist
When asked to "review" code in an interview, systematically check:
| Category | Points to check |
|---|---|
| Null safety | Null checks on inputs? Optional used? |
| Naming | Explicit names? Java conventions (camelCase)? |
| Exceptions | Specific catch? No swallowed exceptions? |
| Strings | .equals() not ==? StringBuilder for loop concatenation? |
| Collections | Interfaces (List, not ArrayList)? Immutable when possible? |
| SOLID | Single responsibility? Depends on abstractions? |
| Thread safety | Shared variables protected? |
| Performance | No N+1? No hidden O(n²) loop? |
| Security | No SQL injection? Inputs validated? |