Java code smells — examples & fixes (staff / principal)

01What staff & principal smell hunting grades

02Severity: when a smell is a blocker

1. Lead every review comment with severity + failure mode.
2. Bundle related smells into one systemic fix when possible.
3. Refuse to approve blockers; negotiate high items with an explicit follow-up.

03Money, quantities, and time

BigDecimal? No — this shipped:
double total = line.price * line.qty;
total = total * 1.08; // tax
return round(total, 2); // still wrong under load of SKUs

Money price = Money.of(line.unitPrice(), line.currency());
Money subtotal = price.multiply(line.qty());
Money tax = taxPolicy.apply(subtotal, shipTo);
return subtotal.plus(tax); // Money uses BigDecimal + currency + scale rules

@PostMapping("/refunds")
Refund refund(@RequestParam String amount) {
  BigDecimal a = new BigDecimal(amount); // NFE / locale / scale surprises
  return refunds.issue(a);
}

record MoneyDto(@NotNull BigDecimal amount, @NotNull @Size(min=3,max=3) String currency) {}

@PostMapping("/refunds")
Refund refund(@Valid @RequestBody MoneyDto body) {
  return refunds.issue(Money.of(body.amount(), Currency.getInstance(body.currency())));
}

boolean isTrialActive(User u) {
  return u.getTrialEnd().isAfter(Instant.now()); // untestable + multi-node skew stories
}

boolean isTrialActive(User u, Clock clock) {
  return u.getTrialEnd().isAfter(clock.instant());
}
// Spring: @Bean Clock systemClock() { return Clock.systemUTC(); }
// tests: Clock.fixed(Instant.parse("2026-01-01T00:00:00Z"), ZoneOffset.UTC)

LocalDate due = LocalDate.now().plusDays(2); // which "now"?

LocalDate due = LocalDate.now(clock.withZone(businessZone)).plusDays(2);
// or: LocalDate.ofInstant(clock.instant(), businessZone).plusDays(2);

long cents = 1999;
double rate = 0.029;
long fee = Math.round(cents * rate); // banker's vs half-up fights later

Money principal = Money.ofCents(1999, USD);
Money fee = feePolicy.feeFor(principal); // BigDecimal rate, explicit RoundingMode, scale=0 cents

04Concurrency & shared mutable state

Item item = repo.findById(id);
if (item.getQty() < 1) throw new SoldOut();
item.setQty(item.getQty() - 1);
repo.save(item); // lost update under concurrency

@Transactional
public void reserve(String id) {
  int updated = repo.decrementIfAvailable(id); // UPDATE … SET qty=qty-1 WHERE id=? AND qty>=1
  if (updated != 1) throw new SoldOut();
}
// or @Version optimistic lock + retry with clear SoldOut semantics

@Service
class RateLimiter {
  private final Map<String, Long> hits = new HashMap<>(); // shared mutable
  boolean allow(String key) {
    hits.merge(key, 1L, Long::sum);
    return hits.get(key) < 100;
  }
}

@Service
class RateLimiter {
  private final ConcurrentHashMap<String, AtomicLong> hits = new ConcurrentHashMap<>();
  // better: Redis / bucket4j / API gateway — process-local maps don't work multi-instance
  boolean allow(String key) {
    long n = hits.computeIfAbsent(key, k -> new AtomicLong()).incrementAndGet();
    return n <= 100;
  }
}

public synchronized Order checkout(Cart cart) {
  payment.charge(cart);   // remote I/O under lock
  return orders.save(...);
}

public Order checkout(Cart cart) {
  // keep critical section tiny — or better, rely on DB uniqueness / idempotency key
  PaymentIntent intent = payment.createIntent(cart.idempotencyKey());
  return orders.savePaid(cart, intent);
}

class Client {
  private static Client instance;
  static Client get() {
    if (instance == null) {
      synchronized (Client.class) {
        if (instance == null) instance = new Client(); // may publish partially constructed
      }
    }
    return instance;
  }
}

enum Client {
  INSTANCE;
  // or Spring @Bean / constructor injection — don't hand-roll singletons
}

05Collections, equality, and API surfaces

class Order {
  private final List<Line> lines;
  List<Line> getLines() { return lines; } // caller can clear()
}

class Order {
  private final List<Line> lines;
  List<Line> getLines() { return List.copyOf(lines); } // or UnmodifiableList
  // mutations only via addLine(...) that enforces invariants
}

@Entity
class User {
  @Id Long id;
  String email;
  public boolean equals(Object o) { return id != null && id.equals(((User)o).id); }
  public int hashCode() { return Objects.hash(id); } // null id → collapses; then id assigned → lost in Set
}

// Prefer business key for equality when needed, or don't put managed entities in Sets.
// Common staff pattern: equals/hashCode on stable natural key (email) OR identity (==) only in aggregates.
@Override public boolean equals(Object o) {
  if (this == o) return true;
  if (!(o instanceof User u)) return false;
  return email != null && email.equalsIgnoreCase(u.email);
}
@Override public int hashCode() { return Objects.hash(email == null ? null : email.toLowerCase()); }

List<String> allowed = loadAllowed(); // ArrayList
return items.stream().filter(allowed::contains).toList(); // O(n*m)

Set<String> allowed = Set.copyOf(loadAllowed());
return items.stream().filter(allowed::contains).toList(); // O(n)

List<String> out = new ArrayList<>();
items.parallelStream().forEach(out::add); // race

List<String> out = items.parallelStream().map(this::map).toList();
// or Collectors.toList() / concurrent collector intentionally

06Resources, threads, and async

InputStream in = client.getObject(key).getObjectContent();
byte[] bytes = in.readAllBytes();
in.close(); // skipped on exception → leak

try (InputStream in = client.getObject(key).getObjectContent()) {
  return in.readAllBytes();
}

List<CompletableFuture<Result>> futs = ids.stream()
  .map(id -> CompletableFuture.supplyAsync(() -> fetch(id))) // unbounded
  .toList();
return futs.stream().map(CompletableFuture::join).toList();

Executor exec = Executors.newFixedThreadPool(32); // or Spring TaskExecutor bean with bounds + queue
Semaphore lim = new Semaphore(32);
List<CompletableFuture<Result>> futs = ids.stream().map(id ->
  CompletableFuture.supplyAsync(() -> {
    lim.acquireUninterruptibly();
    try { return fetch(id); } finally { lim.release(); }
  }, exec)
).toList();

@EnableAsync
@Service
class Mailer {
  @Async
  void send(Email e) { smtp.send(e); } // unbounded threads
}

@Configuration
class AsyncConfig {
  @Bean(name = "mailExecutor")
  Executor mailExecutor() {
    ThreadPoolTaskExecutor t = new ThreadPoolTaskExecutor();
    t.setCorePoolSize(4); t.setMaxPoolSize(16); t.setQueueCapacity(500);
    t.setRejectedExecutionHandler(new CallerRunsPolicy()); // or abort + metric
    t.initialize(); return t;
  }
}
@Async("mailExecutor") void send(Email e) { ... }

try {
  queue.take();
} catch (InterruptedException e) {
  // ignore
}

try {
  queue.take();
} catch (InterruptedException e) {
  Thread.currentThread().interrupt();
  throw new IllegalStateException("interrupted waiting on queue", e);
}

07Nullability, Optional, and stringly types

class User {
  private Optional<String> nickname; // smell
  void setNickname(Optional<String> n) { this.nickname = n; }
}

class User {
  private @Nullable String nickname; // or omit field when absent
  void setNickname(@Nullable String n) { this.nickname = n; }
  Optional<String> nickname() { return Optional.ofNullable(nickname); } // at API boundary if needed
}

User u = repo.findById(id).get();

User u = repo.findById(id)
  .orElseThrow(() -> new NotFound("user", id));

void transition(String status) {
  if (status.equals("ACTIVE")) ...
  else if (status.equals("active")) ... // duplicate
}
boolean cancelled; boolean refunded; // cancelled&&!refunded vs ...

enum OrderStatus { PENDING, ACTIVE, CANCELLED, REFUNDED }
void transition(OrderStatus next) { stateMachine.assertTransition(current, next); }

User find(String id) {
  try {
    return repo.load(id);
  } catch (Exception e) {
    log.error("fail"); return null;
  }
}

User find(String id) {
  return repo.findById(id).orElseThrow(() -> new NotFound("user", id));
}
// map infra failures at the boundary; don't swallow

08Structure smells that amplify change

@Service
class OrderService { // 2000 lines
  Order checkout(...) {
    // price, tax, inventory, payment, email, metrics, audit...
  }
}

@Service
class CheckoutService {
  private final Pricing pricing;
  private final Inventory inventory;
  private final Payments payments;
  private final OrderPublisher publisher;

  @Transactional
  Order checkout(CheckoutCmd cmd) {
    Money total = pricing.quote(cmd.cart());
    inventory.reserve(cmd.cart());
    Payment p = payments.charge(cmd.idempotencyKey(), total);
    Order order = Order.paid(cmd, total, p);
    publisher.ordered(order); // outbox preferred
    return order;
  }
}

void transfer(String from, String to, String amount) { ... } // which is user? account?

void transfer(AccountId from, AccountId to, Money amount) { ... }
record AccountId(UUID value) {
  static AccountId of(String raw) { return new AccountId(UUID.fromString(raw)); }
}

// Controller A
if (!doc.getOwnerId().equals(userId)) throw new Forbidden();
// Controller B forgot the check → IDOR

@Component
class DocumentAccess {
  Document requireOwned(String docId, String userId) {
    Document d = repo.findById(docId).orElseThrow(...);
    if (!d.ownerId().equals(userId)) throw new Forbidden();
    return d;
  }
}
// all entry points call requireOwned — or method security / query filters

order.setStatus("PAID");
order.setPaidAt(Instant.now());
order.setTotal(total);

order.markPaid(payment, clock.instant()); // enforces: pending→paid, total matches, paidAt set once

09Spring / persistence boundary smells

@Transactional(readOnly = true)
Order get(String id) { return repo.findById(id).orElseThrow(); }

// later in controller / DTO mapper — no TX
order.getLines().size(); // LazyInitializationException

@Transactional(readOnly = true)
OrderResponse get(String id) {
  Order o = repo.findWithLines(id); // join fetch / entity graph
  return OrderResponse.from(o);     // map inside TX
}

@Service
class Billing {
  public void run() { charge(); } // this.charge() — no proxy
  @Transactional void charge() { ... }
}

@Service
class Billing {
  private final Billing self; // constructor-inject self via @Lazy, or split collaborator
  public void run() { self.charge(); }
  @Transactional public void charge() { ... }
}
// cleaner: extract ChargingService and inject it

@Transactional
Order checkout(Cart c) {
  Order o = orders.save(...);
  payment.charge(o);      // HTTP inside TX — pool exhaustion
  mailer.send(o);         // more I/O
  return o;
}

@Transactional
Order checkout(Cart c) {
  Order o = orders.savePending(c);
  outbox.enqueue("OrderPaid", o.id()); // same TX
  return o;
}
// payment + mail happen in consumers with idempotency — not inside the DB TX

10Logging, errors, and operability smells

log.info("charge user={} token={} pan={}", userId, stripeToken, pan);

log.info("charge user={} paymentIntent={} last4={}", userId, intentId, last4);
// structured logging + scrubbing filters on the logger MDC pipeline

try {
  return payment.charge(...);
} catch (PaymentException e) {
  log.error("payment failed", e);
  throw e;
}

// log once at the boundary (controller advice / consumer) with correlation id
return payment.charge(...); // let it bubble; enrich context with exception fields

try {
  analytics.track(event);
} catch (Exception ignored) {
  // TODO
}

try {
  analytics.track(event);
} catch (Exception e) {
  metrics.increment("analytics.track.fail");
  log.warn("analytics failed eventType={}", event.type(), e);
  // decide: fail the request vs degrade — make it explicit
}

11Test smells that hide production bugs

Order o = service.checkout(cart);
assertNotNull(o.getId());

Order o = service.checkout(cart);
assertEquals(Money.of("21.60", "USD"), o.total());
assertEquals(OrderStatus.PAID, o.status());
assertTrue(inventory.reserved(cart));

publisher.publish(evt);
Thread.sleep(2000);
assertEquals(1, repo.count());

publisher.publish(evt);
Awaitility.await().atMost(2, SECONDS).untilAsserted(() ->
  assertEquals(1, repo.count())
);
// better: unit-test the handler directly; use @Transactional outbox assertions

@Spy OrderService service;
doReturn(price).when(service).computeTax(...); // testing nothing real

// Prefer fakes for slow I/O; real domain objects for rules.
// Integration test: @DataJpaTest / Testcontainers for reserve UPDATE semantics.

12Principal lens: systemic smells

• Money as double in >1 service → shared Money library + codec for APIs.
• Ownership checks per controller → DocumentAccess / method security / RLS.
• @Async defaults → org-wide TaskExecutor starter with metrics + rejection policy.
• Remote I/O in TX → outbox starter + review checklist item.
• Instant.now() in domain → Clock bean in the service starter.

13API, DTO, and serialization smells

@PostMapping("/users")
User create(@RequestBody User user) { // entity exposed
  return repo.save(user); // client sets role=ADMIN, verified=true
}

record CreateUserRequest(@Email String email, @NotBlank String name) {}

@PostMapping("/users")
UserResponse create(@Valid @RequestBody CreateUserRequest req) {
  User u = User.register(req.email(), req.name()); // server sets role/defaults
  return UserResponse.from(repo.save(u));
}

@PostMapping("/charges")
Charge charge(@RequestBody ChargeReq req) {
  return payments.charge(req); // every retry = new charge
}

@PostMapping("/charges")
Charge charge(@RequestHeader("Idempotency-Key") String key,
              @Valid @RequestBody ChargeReq req) {
  return payments.chargeOnce(key, req); // store key→result with TTL / unique constraint
}

Page<Order> list(Pageable pageable) {
  return repo.findAll(pageable); // trusts client size
}

@GetMapping
Page<OrderResponse> list(@PageableDefault(size = 20) Pageable pageable) {
  int size = Math.min(pageable.getPageSize(), 100);
  Pageable safe = PageRequest.of(pageable.getPageNumber(), size, pageable.getSort());
  return repo.findAll(safe).map(OrderResponse::from);
}

@GetMapping("/{id}")
Order get(@PathVariable String id) {
  return repo.findById(id).orElseThrow(); // entity + lazy lines
}

@GetMapping("/{id}")
OrderResponse get(@PathVariable String id) {
  return query.findOrderView(id); // DTO / projection query
}

14Hot-path performance smells

String s = "";
for (Line line : lines) s += line.render() + "\n";
return s;

StringBuilder sb = new StringBuilder(lines.size() * 64);
for (Line line : lines) sb.append(line.render()).append('\n');
return sb.toString();
// or String.join / Collectors.joining

boolean valid(String email) {
  return Pattern.compile(".+@.+").matcher(email).matches();
}

private static final Pattern EMAIL = Pattern.compile(".+@.+"); // or use a real validator
boolean valid(String email) { return EMAIL.matcher(email).matches(); }

return repo.findAll().stream()
  .filter(o -> o.status() == PAID)
  .filter(o -> o.createdAt().isAfter(since))
  .toList();

return repo.findByStatusAndCreatedAtAfter(PAID, since);
// indexed columns; pagination if the result set can grow

for (Line line : order.getLines()) {
  Product p = productRepo.findById(line.productId()).orElseThrow();
  line.setName(p.name());
}

Set<String> ids = order.lineProductIds();
Map<String, Product> products = productRepo.findAllById(ids).stream()
  .collect(Collectors.toMap(Product::id, Function.identity()));
order.enrichNames(products);

15War stories: how these smells fail in prod

16Hire bar: staff vs principal on smells

17Review script: how to narrate smells

18Practice drills (staff / principal)

1. Drill A (10m): Find money/time smells in a checkout method — double, Instant.now, string amount.
2. Drill B (10m): Inventory reserve — rewrite check-then-act to conditional UPDATE; explain oversell.
3. Drill C (10m): Spot remote I/O inside @Transactional; propose outbox.
4. Drill D (10m): Principal close — list two platform guardrails for the PR.
5. Drill E (15m): Review a controller that takes an entity body + unbounded Pageable — write blocker comments for mass assignment and DoS.
6. Drill F (15m): Narrate a war story (oversell / penny drift / pool exhaustion) in STAR form as if in a principal interview.

19One-page cheat sheet

• Money → Money/BigDecimal + currency — never double.
• Time → inject Clock — never scattered Instant.now().
• Reserves → conditional write / version — never check-then-act.
• Caches on beans → concurrent + bounded — or shared store.
• Async → bounded executor — never unbounded supplyAsync/@Async default.
• Resources → try-with-resources — always.
• Authz → one guard — never copy-paste ownership.
• TX → no remote I/O — outbox / after-commit.
• Logs → no secrets — ids and redaction.
• Principal → local fix + platform guardrail.