Skip to content

Latest commit

 

History

History
469 lines (366 loc) · 16.5 KB

File metadata and controls

469 lines (366 loc) · 16.5 KB
name clean-code-java-idioms
description Apply when fixing Java idiom findings from the Clean Code plugin — J1 (avoid wildcard imports), J2 (don't inherit constants), J3 (constants versus enums), G1 (multiple languages in one source file), G4 (overridden safeties), G25 (replace magic numbers with named constants), G26 (be precise). Use when the user asks to extract constants, convert to enums, narrow types, remove suppressions, or move embedded CSS/SQL/HTML.

Skill: Java Idioms

When to use this skill

  • When fixing a J1, J2, J3, G1, G4, G25, or G26 finding identified by the plugin
  • When writing new code that introduces constants, enums, imports, or numeric/type-sensitive values
  • When reviewing a class that implements an interface solely to inherit its constants

This skill does not apply to test classes, except for J1 (wildcard imports) and G25 (magic numbers in assertions are acceptable — do not extract assertion expected values to constants).

Note on examples: All class names in code examples are illustrative only. Use the action rules below to determine the correct change for your context.


Before you act

If fixing existing code:

  • Identify the finding: match it to one row in the action table below
  • Check scope: each finding targets a single site — do not refactor surrounding code unless the finding requires it
  • Search callers: for inherited constants and enum conversions, search all references to the constants being moved; update every call site in the same change

If writing new code:

  • Do not introduce any anti-pattern listed in the action table — write the correct idiom from the start
  • Every constant must have a name derived from its business meaning, never from its value or type

Action table

Finding Action Notes
Wildcard import Replace each * import with explicit imports for every used type If more than 8 types are imported from a single package, keep the wildcard and flag for human review — the class may have too many dependencies
Inherited constants Remove implements ConstantsInterface. Add import static for each constant used. If the interface defines non-constant methods too, only remove the implements if the class does not override any of those methods
Constants vs enums Extract related static final fields to an enum See enum extraction rules below
Overridden safeties Depends on the safety being overridden — see Overridden safeties section below Never silently delete a safety override
Magic number Extract to a named static final constant Name after business meaning, not value
Imprecise type Narrow the type to the most specific correct choice See type narrowing table below

Wildcard imports

Replace every wildcard import with explicit imports for the types actually used in the file.

// Illustrative only
// BEFORE
import java.util.*;
import org.springframework.web.bind.annotation.*;

// AFTER
import java.util.List;
import java.util.Map;
import java.util.Optional;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;

Inherited constants

Replace implements ConstantsInterface with static imports. The class must not inherit an interface solely to use its constants.

// Illustrative only
// BEFORE
public class ReportController implements ReportConstants {

    public Report generate() {
        return reportService.generate(DEFAULT_PAGE_SIZE, MAX_COLUMNS);
    }
}

// AFTER
import static com.example.report.ReportConstants.DEFAULT_PAGE_SIZE;
import static com.example.report.ReportConstants.MAX_COLUMNS;

public class ReportController {

    public Report generate() {
        return reportService.generate(DEFAULT_PAGE_SIZE, MAX_COLUMNS);
    }
}

Constants vs enums

When two or more static final fields share a common prefix or represent values from the same domain concept, extract them to an enum.

Enum extraction rules:

  • Name the enum after the shared concept, not the prefix: STATUS_ACTIVE, STATUS_INACTIVE becomes enum Status { ACTIVE, INACTIVE }
  • If the constants carry values (strings, ints), add a field to the enum
  • Move the enum to its own file
// Illustrative only
// BEFORE
public class OrderService {
    public static final String STATUS_PENDING = "PENDING";
    public static final String STATUS_SHIPPED = "SHIPPED";
    public static final String STATUS_DELIVERED = "DELIVERED";
    public static final String STATUS_CANCELLED = "CANCELLED";
}

// AFTER — enum in its own file
public enum OrderStatus {
    PENDING("PENDING"),
    SHIPPED("SHIPPED"),
    DELIVERED("DELIVERED"),
    CANCELLED("CANCELLED");

    private final String value;

    OrderStatus(final String value) {
        this.value = value;
    }

    public String value() {
        return value;
    }
}

Overridden safeties

Each overridden safety has its own fix. Do not silently delete the override — understand why it exists first.

Empty catch block: apply the exception-handling skill. Choose the correct pattern (wrap and propagate, boundary translation, genuine recovery, or batch collection). An empty catch is never acceptable.

@SuppressWarnings("unchecked"): extract the unchecked cast into a single-purpose method annotated with @SuppressWarnings("unchecked"). The annotation must not appear on a method that does anything else.

// Illustrative only
// BEFORE
@SuppressWarnings("unchecked")
public List<Widget> loadWidgets(final Object rawData) {
    final var widgets = (List<Widget>) rawData;
    widgets.forEach(this::validate);
    return widgets;
}

// AFTER
public List<Widget> loadWidgets(final Object rawData) {
    final var widgets = castToWidgetList(rawData);
    widgets.forEach(this::validate);
    return widgets;
}

@SuppressWarnings("unchecked")
List<Widget> castToWidgetList(final Object rawData) {
    return (List<Widget>) rawData;
}

Other @SuppressWarnings values: flag for human review. Do not remove the annotation without fixing the underlying warning.

// TODO: G4 — requires human review: @SuppressWarnings("deprecation") on method X

System.out.println, System.err.println, e.printStackTrace(): Console output bypasses log routing, filtering, and alerting. Replace with structured logging via Lombok @Slf4j.

Anti-pattern Fix
System.out.println(...) log.info(...) or log.debug(...)
System.err.println(...) log.warn(...) or log.error(...)
e.printStackTrace() log.error("context message", e) or wrap-and-propagate
// Illustrative only
// BEFORE
System.out.println("Fetching page: " + cursor);
catch (Exception e) { e.printStackTrace(); }

// AFTER
@Slf4j
public class PetClient {
    log.debug("Fetching page (cursor={})", cursor);
    log.error("GraphQL request failed", exception);
}

Always use parameterised logging with {} placeholders — never concatenate strings in log calls.


Magic numbers

Extract numeric and string literals to named static final constants. Name the constant after its business meaning, not its value.

// Illustrative only
// BEFORE
client.variable("first", 50);
if (retryCount > 3) { ... }

// AFTER
private static final int PAGE_SIZE = 50;
private static final int MAX_RETRIES = 3;

client.variable("first", PAGE_SIZE);
if (retryCount > MAX_RETRIES) { ... }

Acceptable magic numbers — do not extract:

  • 0, 1, -1 when used as loop bounds, index offsets, or identity values
  • Assertion expected values in test classes
  • Enum ordinals defined in the enum itself

Imprecise types

Narrow types to the most specific correct choice. Using a broad type when a narrow one exists invites bugs.

Broad type Narrow type When
double / float BigDecimal Money, financial calculations, any value requiring exact decimal precision
String (date) LocalDate Calendar dates without time
String (timestamp) Instant Points in time, UTC timestamps
String (duration) Duration Elapsed time, timeouts
Object The actual type Any situation where the concrete type is known
Map<String, Object> A record or typed class Structured data with known fields
List (raw) List<SpecificType> Always parameterise collections
java.io.File java.nio.file.Path File paths, directory references
FileInputStream Files.newInputStream(path) Reading file bytes
FileOutputStream Files.newOutputStream(path) Writing file bytes
FileReader Files.newBufferedReader(path) Reading text files
FileWriter Files.newBufferedWriter(path) Writing text files
// Illustrative only
// BEFORE
final double price = 19.99;
final String createdAt = "2024-03-15";
final Map<String, Object> config = loadConfig();

// AFTER
final BigDecimal price = new BigDecimal("19.99");
final LocalDate createdAt = LocalDate.parse("2024-03-15");
final DashboardConfig config = loadConfig();

The java.io.File family returns boolean for failures instead of throwing, cannot represent non-default filesystems, and mixes path representation with I/O operations. Always use java.nio.file.


StringBuilder — when to use, when not to, what to name it

StringBuilder has a narrow role. Most Java string construction that looks like it needs one actually doesn't, and the cases that do usually use the wrong name.

Positive patterns

Hot-path loops accumulating ≥ 1KB. Building a large response body, a CSV stream, or a bulk SQL statement inside a loop that runs many times. Here, the allocator pressure of repeated += concatenation is real. Declare the builder locally and name it after what it contains.

final StringBuilder html = new StringBuilder(pages.size() * 512);
for (final Page page : pages) {
    html.append("<section>").append(page.render()).append("</section>");
}
return html.toString();

Short-lived per-call assembly. Same rule — local, descriptive name, never a parameter.

Negative patterns

Don't name it sb. The plugin's detection recipe flags every local StringBuilder sb / StringBuffer sb. Name it after the content: html, markdown, csv, buffer, message. sb is an encoding of the type, not a description of the value (see N6: avoid encodings).

Don't thread a StringBuilder parameter through helpers. Passing a StringBuilder into a helper that calls .append() on it is the output-argument anti-pattern (F2) in disguise. Two fixes:

// Bad — threaded output argument
private void renderRow(StringBuilder out, Row row) {
    out.append(row.label()).append(": ").append(row.value());
}

// Good — return the value
private String renderRow(Row row) {
    return row.label() + ": " + row.value();
}

// Also good when there are many pieces — return a List, join at the top
private List<String> renderLines(Section section) {
    return section.items().stream()
            .map(this::renderRow)
            .toList();
}

Don't reach for StringBuilder when a text block works. For mostly literal content with a few interpolations, prefer """<text-block>""".formatted(...) — it reads as the thing it produces.

// Bad — every reader has to mentally execute the appends
final StringBuilder sb = new StringBuilder();
sb.append("<table>\n");
sb.append("  <tr><td>").append(name).append("</td></tr>\n");
sb.append("</table>\n");
return sb.toString();

// Good — the shape of the output is visible in the source
return """
        <table>
          <tr><td>%s</td></tr>
        </table>
        """.formatted(name);

Don't build a sub-10-line constant with StringBuilder. Plain + concatenation is fine and the JIT fuses it. The builder adds noise without saving anything.

Decision rule

  1. Content is mostly literal with ≤ 3 interpolations → text block.
  2. Content is a fixed list of lines with no branching → String.join("\n", ...) over a List<String>.
  3. Content is built across branching logic in one method → local builder with a descriptive name (never sb).
  4. Content is built by helpers collaborating through an accumulator → refactor each helper to return its piece; caller joins them.

Only pattern 3 legitimately uses StringBuilder. Patterns 1, 2, and 4 are the ones the detection recipe is catching.


Do not

  • Name a constant after its value: FIFTY = 50, THREE = 3, FIVE_HUNDRED = 500 — name it after what it means
  • Create an enum for unrelated constants that happen to share a type — enums represent a single domain concept with a closed set of values
  • Remove @SuppressWarnings without fixing the underlying warning that caused it
  • Extract assertion expected values in tests to constants — test literals are documentation
  • Introduce wildcard imports in new code
  • Use implements on an interface solely to access its constants
  • Fix multiple findings in a single task — one finding per task keeps each fix independently reviewable and revertable
  • Expand scope beyond the identified location without explicit instruction
  • Apply this skill to test classes except for wildcard imports and magic numbers (see exemptions at top)

G1 — Multiple Languages in One Source File

A Java file that builds HTML, CSS, or SQL by concatenating strings is hard to read, hard to syntax-check, and hard to edit (no IDE support inside a string). Move the other language out.

Pattern: extract to a classpath resource

Before:

class HtmlReportWriter {
    void renderStyles(StringBuilder sb) {
        sb.append("<style>\n");
        sb.append("  body { font-family: sans-serif; }\n");
        sb.append("  .error { color: red; }\n");
        sb.append("</style>\n");
    }
}

After:

src/main/resources/html-report/styles.css          (the real CSS file, editable in any editor)
src/main/java/.../HtmlReportWriter.java            (loads and emits it)
class HtmlReportWriter {
    private static final String STYLES = loadResource("/html-report/styles.css");

    void renderStyles(StringBuilder sb) {
        sb.append("<style>\n").append(STYLES).append("</style>\n");
    }

    private static String loadResource(String path) {
        try (InputStream in = HtmlReportWriter.class.getResourceAsStream(path)) {
            if (in == null) {
                throw new IllegalStateException("Missing resource: " + path);
            }
            return new String(in.readAllBytes(), StandardCharsets.UTF_8);
        } catch (IOException e) {
            throw new UncheckedIOException(e);
        }
    }
}

Rules

  • Put resources under src/main/resources/<domain>/<name>.<ext>. The <domain> folder groups related assets (e.g. html-report/styles.css, html-report/template.html).
  • Load once into a private static final field so you don't re-read on every call.
  • Fail loudly if the resource is missing — a null InputStream is a deployment bug, not a runtime condition to tolerate.
  • For small templated sections (a <tr> with substitutions), String.format or a tiny replace("{name}", value) helper is enough. Do not pull in Handlebars / Mustache for one template.
  • For SQL specifically, use Files.readString / classpath resources the same way; do not concatenate SQL with user values — use PreparedStatement parameters.

Do not

  • Move CSS/HTML/SQL into a constant String STYLES = "..." inside the same Java file. That doesn't fix G1 — the other language is still embedded in Java source.
  • Split the Java into many methods (appendFooter, appendHeader, …) that each inline a different slice of the other language. The finding still applies; you've just spread it around.

Traceability: Clean Code J1, J2, J3, G1, G4, G25, G26