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

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> studentRecord;

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

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

  public Map> db() {
    return Collections.unmodifiableMap(studentRecord);
  }

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

  public Map> sort() {
    Map> studentGradeMap = new HashMap<>();
    for (Map.Entry> 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("

Comments about the code itself first:
Use Java 8 constructs

public void add(String name, int grade) {
    Set 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 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 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> 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> 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.)

StackExchange Code Review Q#127783, answer score: 4