Write a small archiving program that stores students' names along with the grade that they are in

Question

Problem:

Write a small archiving program that stores students' names along with the grade that they are in.

In the end, you should be able to:

• Add a student's name to the roster for a grade.
• Get a list of all students enrolled in a grade.
• Get a sorted list of all students in all grades. Grades should sort as 1, 2, 3, etc., and students within a grade should be sorted
  alphabetically by name.

Note that all our students only have one name. (It's a small town, what do you want?)

Code:

import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.TreeSet;
import java.util.ArrayList;
import java.util.Collections;


public class School {
  private final Map<Integer, Set<String>> studentRecord;

  public School() {
    studentRecord = new HashMap<>();
  }

  public void add(String name, int grade) {
    Set<String> names = Optional.ofNullable(
        studentRecord.get(grade)).orElse(new TreeSet<>());
    names.add(name);
    studentRecord.put(grade, names);
  }

  public Map<Integer, Set<String>> db() {
    return Collections.unmodifiableMap(studentRecord);
  }

  public Set<String> grade(int grade) {
    return Collections.unmodifiableSet(Optional.ofNullable(
        studentRecord.get(grade)).orElse(new TreeSet<>()));
  }

  public Map<Integer, List<String>> sort() {
    Map<Integer, List<String>> studentGradeMap = new HashMap<>();
    for (Map.Entry<Integer, Set<String>> entry : studentRecord.entrySet()) {
      studentGradeMap.put(entry.getKey(), new ArrayList<>(entry.getValue()));
    }
    return Collections.unmodifiableMap(studentGradeMap);
  }
}

Test suite:

import static org.assertj.core.api.Assertions.assertThat;

import org.junit.Test;

import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

public class SchoolTest {
  private final School school = new School();

  @Test
  public void startsWithNoStudents() {
    assertThat(school.db()).isEmpty();
  }

  @Test
  public void addsStudents() {
    school.add("Aimee", 2);
    assertThat(school.db().get(2)).contains("Aimee");
  }

  @Test
  public void addsMoreStudentsInSameGrade() {
    final int grade = 2;
    school.add("James", grade);
    school.add("Blair", grade);
    school.add("Paul", grade);

    assertThat(school.db().get(grade)).hasSize(3).contains("James", "Blair", "Paul");
  }

  @Test
  public void addsStudentsInMultipleGrades() {
    school.add("Chelsea", 3);
    school.add("Logan", 7);

    assertThat(school.db()).hasSize(2);
    assertThat(school.db().get(3)).hasSize(1).contains("Chelsea");
    assertThat(school.db().get(7)).hasSize(1).contains("Logan");
  }

  @Test
  public void getsStudentsInAGrade() {
    school.add("Franklin", 5);
    school.add("Bradley", 5);
    school.add("Jeff", 1);
    assertThat(school.grade(5)).hasSize(2).contains("Franklin", "Bradley");
  }

  @Test
  public void getsStudentsInEmptyGrade() {
    assertThat(school.grade(1)).isEmpty();
  }

  @Test
  public void sortsSchool() {
    school.add("Jennifer", 4);
    school.add("Kareem", 6);
    school.add("Christopher", 4);
    school.add("Kyle", 3);
    Map<Integer, List<String>> sortedStudents = new HashMap<Integer, List<String>>();
    sortedStudents.put(6, Arrays.asList("Kareem"));
    sortedStudents.put(4, Arrays.asList("Christopher", "Jennifer"));
    sortedStudents.put(3, Arrays.asList("Kyle"));

    assertThat(school.sort()).isEqualTo(sortedStudents);
  }
}

Note:

As personal opinion I found this exercise horrible, the implementation was not that difficult but the API expected in the test suite were horrible. The client knows more than expected of the internal date structure used and of course Law of Demeter.

Design Decisions:

The question expected some kind of mapping of grades with names so, the choice for Map seemed obvious to me. Now, the expectation was to have a sorted list of names so, I had two choices:

• Have a unique list of names and keep them sorted if any modifications are made.
• Have a sorted set which takes care of the above problem. But as the client code expected the List in the sort method I have to do the conversion there.

Questions:

Today while coding I got one weird issue, I was confused about the data structure for studentRecord so I toggled between Set and List couple of times, but every time that change was propagated everywhere in the code for eg: return type and all, is it some code smell or bad OO design?

Last but not least I kept the studentRecord field final and every public methods returns immutable view of it to prevent any unintentional modifications.

Reference: Exercism

Answer 1

Comments about the code itself first:

Use Java 8 constructs

public void add(String name, int grade) {
    Set<String> names = Optional.ofNullable(studentRecord.get(grade)).orElse(new TreeSet<>());
    names.add(name);
    studentRecord.put(grade, names);
}

can simply be written

public void add(String name, int grade) {
    studentRecord.computeIfAbsent(grade, k -> new TreeSet<>()).add(name);
}

The method computeIfAbsent will return the current mapping for the given key. If there is no mapping, it will store as value for the key the result of invoking the given mapping function and return it.

---

In the same way, your grade method is:

public Set<String> grade(int grade) {
    return Collections.unmodifiableSet(Optional.ofNullable(studentRecord.get(grade)).orElse(new TreeSet<>()));
}

You could just use the method getOrDefault to return a default value:

public Set<String> grade(int grade) {
    return Collections.unmodifiableSet(studentRecord.getOrDefault(grade, Collections.emptySet()));
}

In this case, the default value is Collections.emptySet() which is a pre-existent (and pre-allocated) empty set.

About unmodifiable constructs

Your db() method currently does:

public Map<Integer, Set<String>> db() {
    return Collections.unmodifiableMap(studentRecord);
}

It returns an unmodifiable view of the current map. This is great as it prevents the caller from adding entries inside it and modifying the internal data structure. However... the set it still modifiable. Consider:

School school = new School();
school.add("James", 2);
school.db().get(2).add("Blair");
System.out.println(school.db());

This will modify the students stored for the grade 2: the map was never modified but its entries were.

This tells us that you will be better off by returning a new map completely, containing a new list:

public Map<Integer, Set<String>> db() {
    return studentRecord.entrySet()
                        .stream()
                        .collect(Collectors.toMap(Map.Entry::getKey, e -> new TreeSet<>(e.getValue())));
}

---

Now, about the design.

There are indeed two choices about having a List or a Set for the names:

• List: we know from the problem description that there won't be duplicate names (Note that all our students only have one name) so this isn't a problem. We know also that our School has two methods to dump its data: one in "raw" form db() and one in sorted form sort(). If we use a list, we'll explicitely need to sort in the latter method.
• SortedSet: This makes sure that there won't be any dupes but this was already a pre-requisite. It internally keeps the data sorted so both method db() and sort() will return exactly the same data.

All in all, I don't think there is a "best" choice. From the public methods exposed by School, I would maybe prefer a List: this way, the db() method can directly return its unsorted data, very quicky; whereas the sort() method will need to do more work to actually sort and return that.

However, you're right that juggling between a Set and a List changes the method return type. However, there is a way to prevent that: return an Iterable. It sends the client the signal that all it can do with it is iterate over the results, and nothing else. And since both Set and List are Iterable, it directly solves the problem: you won't need to change the public API if you decide to switch.

(And in case the ugly client still wants to downcast the Iterable, we still need make sure to return true unmodifiable constructs.)