The instanceof Grade Book: Inheritance Used for Roles
The Shevach municipal high school has computed Bagrut averages for eight years with a Java system whose hierarchy is Person → Student → ExcellentStudent. GradeBook.average() opens with a chain: if (p instanceof ExcellentStudent) bonus = 5; else if (p instanceof Student) bonus = 0; else if (p instanceof ExternalCandidate) ... External candidates (נבחנים אקסטרניים), who are not enrolled in any school, were added by making ExternalCandidate extend Student and leaving its school field null, so every caller now guards against null. Last month a teacher who is also the mother of a student was entered twice - once as Teacher, once as Parent - and her phone number and address now disagree in the two records. A new rule grants a 10% scholarship discount to students in the Mechina programme; it cuts across the hierarchy, and the principal wants it live in three weeks. The developer's proposal is to add a class MechinaExcellentStudent.
1. Name the design problems in this code and explain precisely why ExternalCandidate extends Student is a violation of the Liskov Substitution Principle rather than merely an inconvenience.
Read the worked answer
Smells, in order of damage:
- Type-code dispatch.
average()asks what a person is instead of asking the person to compute its own contribution. Every new category edits this one method - a textbook open/closed violation. The method also grows a branch per category squared once categories combine. - Broken substitutability.
ExternalCandidateis declared aStudentbut cannot honour Student's contract: a school that is non-null, enrolment, report-card delivery, homeroom assignment. An object of the subtype placed where the supertype is expected throws a NullPointerException. Every caller must add a guard that the type system promised was unnecessary - that is the observable symptom of an LSP violation, not a style issue. The subtype has weakened the precondition ("student without a school") while the supertype's postconditions still advertise a school. - Inheritance used for roles. Teacher and Parent are not kinds of a person; they are roles one person holds simultaneously, and roles change over time (a parent joins the staff). Modelling them as separate root objects duplicates identity, which is exactly how one human ended up with two conflicting phone numbers. Identity must be one record, ideally keyed by ID number.
- Policy in the type lattice. The 10% Mechina rule is a policy that varies by year and programme. Encoding it as a subclass predicts the next three classes: MechinaExcellentStudent, MechinaExternalStudent, MechinaAdultStudent - a combinatorial explosion for a rule that is one line of arithmetic.
2. Propose a replacement design. Show how an external candidate, a teacher who is also a parent, and the Mechina discount are each handled without new subclasses.
Read the worked answer
Identity once, roles by composition.
Personholds the identity fields (ID number, name, contact details) exactly once, and aPersonRegistrykeyed on ID number rejects duplicates at entry time.- Roles become small interfaces implemented by role objects held in a list on
Person:TeacherRole,ParentRole,StudentRole. A teacher who is also a parent is onePersonwith two roles; the phone number exists in one place, so drift is impossible.
External candidates. Stop inheriting. Introduce Examinee { List<BagrutResult> results(); } implemented by both EnrolledStudent (which owns a non-null School) and ExternalCandidate (which simply has no school-related methods). Callers that only need results depend on Examinee and cannot reach for a school at all, so the null guards disappear rather than being centralised.
Bonus and discount as policy objects. GradePolicy { double bonus(StudentProfile p); double discount(StudentProfile p); } is composed into the grade calculator. The Mechina rule is a new implementation of GradePolicy, combined with the existing ones (decorator or a list of rules), tested in isolation. The calculator is closed for modification and open for extension.
Getting there safely. Keep the old classes as thin façades delegating to the new model for one release; add characterisation tests that capture today's averages for ~20 real report cards; only then delete the instanceof chain. The migration is a sequence of small shippable steps, not a rewrite.
3. The principal wants the discount live in three weeks, and the developer wants to do the full refactor first. Advise on sequencing, and say what could go wrong with each option.
Read the worked answer
Split behaviour from structure. These are two changes with different risk profiles and must not ship in the same release.
Ship now (days, not weeks): the discount as a GradePolicy implementation plugged into the existing calculator. It touches one call site, is testable against hand-computed examples, and gives the principal the deadline. If the surrounding code is still ugly, that is acceptable - the discount does not depend on the identity model.
Defer (after the exam period): the role/identity refactor and the removal of the instanceof chain. This one changes persistence, breaks the two-person-records problem and needs data cleanup (a duplicate-ID report should be run first to size it). Doing it in three weeks, alongside a live regulation change, means that when averages come out wrong nobody can tell whether the cause is the new discount or the new object model.
Failure modes. Refactor-first: the deadline slips, and the school either ships untested code or misses the rule; the two changes become entangled in one diff and one rollback. Discount-only forever: the null guards and duplicate records keep generating bugs, and the next rule (adult candidates, retakes) multiplies classes again.
Concrete guardrails. Freeze a regression set of last year's averages before touching anything; make each step independently revertible; require the duplicate-ID report and a written migration plan as the entry ticket for the refactor; and put the policy rule behind a configuration value so next year's percentage is not a code change.