Spotting Encapsulation Violations

Two things are wrong, and both are about the class losing control of its own state. The lines field is public, so any caller can add, remove or clear it, and the invoice has no idea. The taxRate field is public and mutable, so a caller can set a rate that breaks any pricing rule the invoice is supposed to enforce. The class computes a total, but it cannot promise the total is right, because its inputs are editable from outside.

public class Main {
    public static void main(String[] args) {
        Invoice invoice = new Invoice();
        invoice.lines.add("Widget");
        invoice.lines.add("Gadget");
        invoice.taxRate = 0.2;
        System.out.println(invoice.total());
    }
}

class Invoice {
    final java.util.List<String> lines = new java.util.ArrayList<>();
    double taxRate = 0.0;
    double total() {
        double sum = 0;
        for (String line : lines) {
            sum += 10.0;
        }
        return sum * (1 + taxRate);
    }
}

The output shows the price being assembled from outside. The caller adds the lines and sets the tax rate itself, and the invoice faithfully computes 24.0 from that. But nothing stops a caller setting the rate to a negative number, or clearing the list and then asking for a total, or adding a line and never paying for it in a consistent way.

The fix follows the shape of the class. Make both fields private, replace direct list manipulation with an addLine method, and move the tax rate into the constructor so it is set once and validated once. If the rate genuinely varies, give it a method that validates the value. The call sites then read invoice.addLine("Widget") and new Invoice(0.2), which is barely different from what they did before, so the change is cheap.

The general lesson to draw out is that a public mutable field is the same as no encapsulation at all, and a public mutable collection is worse because the damage arrives through the back door.

Start with a mechanical sweep rather than an opinion. List every public field, every public collection reference, and every setter. A public field is an immediate encapsulation violation, because the class has declared that anybody may write its state. A public collection is nearly as bad, because the caller gets write access through the return value.

The subtler signs take reading. A class whose setters do nothing but assign is usually a class whose invariants live somewhere else, often in a service that operates on it. A constructor that assigns fields directly while a setter next to it validates is a class with two rules and one of them is bypassed. Getter methods that return the internal collection or array are leaks, because the caller gets a mutable handle on the state.

Another smell is a method that takes an object and mutates it on the caller's behalf, such as a method that takes a list and sorts it in place. The class has taken on responsibility for state it does not own, and the caller has lost track of who may change what.

Then judge by intent rather than by pattern. The question is not whether a setter exists but whether the class is the authority for the value it holds. If two different parts of the codebase can put the same object into different states, the class is not the authority, and the fix is to move the decision behind its methods.

The closing advice for a review is to fix the ordering: start with the classes that other code depends on most, because every fix there removes the largest number of illegal paths at once.