How does this code break the Law of Demeter?

Question

The following code breaks the Law of Demeter:

public class Student extends Person {
  private Grades grades;

  public Student() {
  }

  /** Must never return null; throw an appropriately named exception, instead. */
  private synchronized Grades getGrades() throws GradesException {
    if( this.grades == null ) {
      this.grades = createGrades();
    }

    return this.grades;
  }

  /** Create a new instance of grades for this student. */
  protected Grades createGrades() throws GradesException {
    // Reads the grades from the database, if needed.
    //
    return new Grades();
  }

  /** Answers if this student was graded by a teacher with the given name. */
  public boolean isTeacher( int year, String name ) throws GradesException, TeacherException {
    // The method only knows about Teacher instances.
    //
    return getTeacher( year ).nameEquals( name );
  }

  private Grades getGradesForYear( int year ) throws GradesException {
    // The method only knows about Grades instances.
    //
    return getGrades().getForYear( year );
  }

  private Teacher getTeacher( int year ) throws GradesException, TeacherException {
    // This method knows about Grades and Teacher instances. A mistake?
    //
    return getGradesForYear( year ).getTeacher();
  }
}

public class Teacher extends Person {
  public Teacher() {
  }

  /**
   * This method will take into consideration first name,
   * last name, middle initial, case sensitivity, and
   * eventually it could answer true to wild cards and
   * regular expressions.
   */
  public boolean nameEquals( String name ) {
    return getName().equalsIgnoreCase( name );
  }

  /** Never returns null. */
  private synchronized String getName() {
    if( this.name == null ) {
      this.name == "";
    }

    return this.name;
  }
}

Questions

1. How is the LoD broken?
2. Where is the code breaking the LoD?
3. How should the code be written to uphold the LoD?

Answer 1

Person.isTeacher "reaches through" according to the wikipedia article you mention.

I was surprised to find the list of grades a property of the student. Shouldn't that be something the school knows about and manages? I'd ask the school, which teacher graded a student in which year...

Answer 2

I think that here are two problems:

1. Grades logic is too much mixed with Student. It should be done in Grades class
2. Teacher's logic is placed into Student.

Conclusion: Student knows too much about inner structure and logic of Teacher and Grades and that breaks LoD

Answer 3

By having these two private functions breaks LoD.

private Grades getGradesForYear( int year )
private Teacher getTeacher( int year )

Students shouldn't need the logic to to perform such tasks.

The way I would redesign this is to separate data from logic. Student should purely be a data only. It should contain information about the student and student only. Therefore this does not include Grades as that concept requires others such as subject, and teacher.

The same goes for teacher. I would then create a place to store grade information and another place for subject information.

To perform similar tasks I would do this:

gradesDatabase.getGrade(subject, student);
subjectDatabase.getTeacher(subject, student);

Where subject is also a data only object.

Answer 4

Most problems such as this can be solved by revisiting your domain model.

It looks like the Student has way more responsibility than it should. It should have only one reason to change.

I would refactor this by adding a ReportCard object.

public class ReportCard
{
  public Student Student...
  public int Year...
  public ReportCardItem[] ReportCardItems...

  getGrades()...
  createGrades()...
}

public class ReportCardItem
{
  public Grade Grade...
  public string Subject...
  public Teacher Teacher...
}

Answer 5

Methods in class Student which break the Law of Demeter are

private Grades getGradesForYear( int year )
private Teacher getTeacher( int year )

because these expose domain objects Grades and Teacher to the application.

Assuming that you wish to continue to hide the Grades inside a Student and a Teacher inside Grades, one way to remedy this problem is to define proxy methods (also called delegate methods) in class Student that operate on the internal Grades and Teacher objects on behalf of the application, similar to method Student.isTeacher(int, String). This solution may lead to duplication of methods in Grades and Teacher in Student which is a disadvantage of a class design which respects the LofD.

A better solution would be to remove the Grades and Teacher from Student and put them all in another class, say Transcript:

class Transcript {
  Student student;
  Teacher teacher;
  Grades grades;
  Integer year;
}