Load and analyze a list of people from a file

Question

I recently completed a programming task in Java for a job that was javascript heavy but the hiring company wanted some Java knowledge. I've been using Java since the turn of the year. Other than learning on the job I've read Clean Code and thought I could have a go at this task. Clearly I failed because the recruitment agent is avoiding me. I thought my design was ok but I'm going wrong somewhere/everywhere so want to learn from this.

The task was:

INSTRUCTIONS

Please carefully read these instructions before attempting to write your solution.

This is an exercise to write some java code to read in a text file and answer three simple questions. The questions are at the bottom of this file. Once you have finished the exercise, please zip this directory and email it back to us. Please note that we:

• are looking for code that is written in a modular way that demonstrates thought about reusability, maintainability and your understanding of object oriented programming.

• are looking for you to showcase your code design skills, so feel free to create the necessary classes, avoid long procedural classes, overuse of static members, messy code, etc.

• have provided IntelliJ IDEA & Eclipse project files for your convenience, but you can ignore, delete or replace them.

QUESTIONS

Please read in the data contained within the file manipulate-data.txt

Manipulate the data to programmatically answer the following questions:

1. How many people in the list are male?
2. In years what is the average age of the people in the list?
3. How many days older is Jeff Briton than Tom Soyer?

Please print your answers out to the screen by using the 'System.out.print' function.

My code:

This class was provided with some code in and a private method. I wanted to keep this really clean (which might have been where I went wrong - see my own critique of my code below).

public class ManipulateDataApplication {

    /**
     * Main method
     * Throwing exceptions because custom exception types
     * print to stdout which is sufficient for this exercise
     * @param args
     * @throws FailedToBuildPeopleListException
     * @throws FailedToFindPersonException
     * @throws InvalidPeopleListException
     */
    public static void main(String[] args) throws FailedToBuildPeopleListException, FailedToFindPersonException, InvalidPeopleListException {

        PeopleList peopleList = new PeopleList();
        peopleList.buildPeopleListFromFile("manipulate-data.txt");

        System.out.println("Answers to questions:");

        System.out.println("1. There are " + peopleList.countBySex().get("male") + " males in the list.");
        System.out.println("2. In years, the average age of the people in the list is " + peopleList.averageAge() + ".");
        System.out.println("3. " + peopleList.ageDifferenceInDays("Jeff Briton", "Tom Soyer") + ".");
    }   
}

I created a class for the list of Person objects but looking back I didn't separate the data from the object and instead thought that this class will include everything for working with the list so probably breaking the single responsibility principle.

/**
 * This class enables the creation of List from a file that 
 * can contain instances of {@link Person} with methods that operate
 * on that list.
 */
public class PeopleList {
    private List<Person> peopleList = new ArrayList<Person>();


    public PeopleList() {

    }

    public List<Person> buildPeopleListFromFile(String fileName) throws FailedToBuildPeopleListException {
        File file = new File(fileName);
        System.out.println(file.getName() + " file exists = " + file.exists());
        try (BufferedReader bufferedReader = Files.newBufferedReader(Paths.get(fileName))) {
            int lineNumber = 0;
            String line;
            while ((line = bufferedReader.readLine()) != null) {
                if (lineNumber > 0) {
                    Person person = new Person(line);
                    peopleList.add(person);
                }
                lineNumber++;
            }
        } catch (IOException e) {
            throw new FailedToBuildPeopleListException(e);
        }

        return peopleList;
    }

    public Map<String, Long> countBySex() throws InvalidPeopleListException {
        if (!peopleList.isEmpty()) {
            Map<String, Long> counted = peopleList.stream()
                    .collect(Collectors.groupingBy(Person::getSex, Collectors.counting()));

            return counted;
        } else {
            throw new InvalidPeopleListException("PeopleList is empty");
        }
    }

    public int averageAge() throws InvalidPeopleListException {
        if (!peopleList.isEmpty()) {
            Double average = peopleList.stream()
                    .collect(Collectors.collectingAndThen(Collectors.summarizingInt(Person::getAge), 
                            dss -> dss.getAverage()));
            return average.intValue();
        } else {
            throw new InvalidPeopleListException("PeopleList is empty");
        }
    }

    public String ageDifferenceInDays(String person1, String person2) throws FailedToFindPersonException {
        LocalDate person1Birthdate = findPerson(person1).getBirthdate();
        LocalDate person2Birthdate = findPerson(person2).getBirthdate();

        long daysBetween = ChronoUnit.DAYS.between(person1Birthdate, person2Birthdate);
        if (daysBetween < 0) {
            daysBetween = ChronoUnit.DAYS.between(person2Birthdate, person1Birthdate);
            return person2 + " is " + daysBetween + " days" + " older than " + person1;
        } else {
            return person1 + " is " + daysBetween + " days" + " older than " + person2;
        }
}

    private Person findPerson(String personName) throws FailedToFindPersonException {
        Optional<Person> matching = peopleList.stream()
            .filter(person -> person.getFullName().equals(personName))
            .findFirst();

        Person person = matching.get();
        if (person != null) {
            return person;
        } else {
            throw new FailedToFindPersonException("Person not in list");
        }
    } 
}

And finally the Person object (I'm not really confident about the way I've used multiple constructors here):

public class Person {
    private String fullName;
    private String sex;
    private int age;
    private LocalDate birthdate;

    public Person(String fullName, String sex, int age, LocalDate birthdate) {
        this.fullName = fullName;
        this.sex = sex;
        this.age = age;
        this.birthdate = birthdate;
    }

    public Person (String line) {
        String[] columns = line.split(",");
        this.fullName    = columns[0].trim();
        this.sex         = columns[1].trim();
        this.age         = Integer.parseInt(columns[2].trim());
        this.birthdate   = parseDate(columns[3].trim());
    }

    private LocalDate parseDate(String date) {
        DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd/MM/yyyy");
        LocalDate localDate = LocalDate.parse(date, formatter);
        return localDate;
    }

    public String getFullName() {
        return fullName;
    }

    public void setFullName(String fullName) {
        this.fullName = fullName;
    }

    public String getSex() {
        return sex;
    }

    public void setSex(String sex) {
        this.sex = sex;
    }

    public int getAge() {
        return age;
    }

    public void setAge(int age) {
        this.age = age;
    }

    public LocalDate getBirthdate() {
        return birthdate;
    }

    public void setBirthdate(LocalDate birthdate) {
        this.birthdate = birthdate;
    }

    @Override
    public String toString() {
        return "Person [fullName=" + fullName + ", sex=" + sex + ", age=" + age + ", birthdate=" + birthdate + "]";
    }
}

All the custom exceptions are the same. Here's an example of one:

public class FailedToFindPersonException extends Exception {
    private static final long serialVersionUID = 1L;

    public FailedToFindPersonException(Throwable cause) {
        super(cause);
        System.out.println("Failed to find person: " + cause.getMessage());
    }

    public FailedToFindPersonException(String cause) {
        super(cause);
        System.out.println("Failed to find person: " + cause);
    }

    public FailedToFindPersonException() {

    }
}

Looking at my code the areas I don't like / I'm not sure about are:

• PeopleList doing too much: should I have just kept that class simple and added the methods for working with the list in the main method in ManipulateDataApplication?
• Multiple constructors in Person
• I wanted to add a generic counter method instead of the specific one I added but I ran out of time
• I like catching exceptions and converting them to custom exceptions but maybe the way I have done it is overkill and I don't like my check for an empty peopleList then throwing a custom exception but I'm just not sure what a better alternative is.

Answer 1

Design Princples and Patterns

Other than learning on the job I've read Clean Code

Uncle Bob would wag his finger at you, then open up that book to SRP. :D

I created a class for the list of Person objects but looking back I didn't separate the data from the object and instead thought that this class will include everything for working with the list so probably breaking the single responsibility principle.

Yes. You are correct. making just the list class which does only list things would have been the correct approach, anything else breaks SRP.

so a bit more of an OO approach would have been to have a factory that takes in a path (be it in string form or other) and returns your person list. Its only responsibility would be reading the file end to end and creating a list with all the people in it.

And finally the Person object (I'm not really confident about the way I've used multiple constructors here):

and that is a good concern to have. Here there are a few approaches, you could again go with the factory pattern that takes in a line from the file from your other factory which is reading the file. You could make a converter/adapter (same concept just coded slightly differently) The factory pattern again seems to fit the bill the best.

those 4 classes would have gotten you to the point of having all the objects you need. I would not have gone past that. This makes them modular and easy to use else where because they behave much like any other class in the java world. So when it comes down to answering the 3 questions again you would want to create another class that does those things. I think the Builder design pattern would be a good choice here. (sorry for the C# version, but the same principle applies) GOF builder design pattern

I wont implement the code, but show you what structure I would follow. First I would make the builder interface

public interface FindAnswerToQuestion {
    void findAnswer(PeopleList peopleList);
    String getAnswer();
}

then I'd make a class that implements it for one of the questions

public class FindCountOfMalesAnswer implements FindAnswerToQuestion {
    @Override
    public void findAnswer(PeopleList peopleList) {

    }

    @Override
    public String getAnswer() {
        return "";
    }
}

From that point using it is very simple

    FindAnswerToQuestion[] finders = {
            new FindCountOfMalesAnswer(),
    };
    for (FindAnswerToQuestion finder : finders){
        finder.findAnswer(peopleList);
        System.out.println(finder.getAnswer());
    }

it would be debatable in my opinion to make the concrete builders return the answer in English or to just return the numbers and let others format it. The way I see it it would be trivial to switch back and forth because on findAnswer() I would think you'd save the number internally on that class then in the getAnswer() you would either return the number or the formatted string.

Other concerns

public List<Person> buildPeopleListFromFile(String fileName) throws FailedToBuildPeopleListException {
    File file = new File(fileName);
    System.out.println(file.getName() + " file exists = " + file.exists());
    try (BufferedReader bufferedReader = Files.newBufferedReader(Paths.get(fileName))) {
        int lineNumber = 0;
        String line;
        while ((line = bufferedReader.readLine()) != null) {
            if (lineNumber > 0) {
                Person person = new Person(line);
                peopleList.add(person);
            }
            lineNumber++;
        }
    } catch (IOException e) {
        throw new FailedToBuildPeopleListException(e);
    }

    return peopleList;
}

in this you count the line numbers. It isn't part of the requirements to know this, and you don't use it anywhere...so why? You also used Files to get a BufferedReader but you are really only concerned with each line in the file. so using the stream feature of Java 8 you could simplify reading the file and adding to your list like so

Files.lines(Paths.get(fileName)).forEach(line-> peopleList.add(new Person(line)));

---

public Map<String, Long> countBySex() throws InvalidPeopleListException {
    if (!peopleList.isEmpty()) {
        Map<String, Long> counted = peopleList.stream()
                .collect(Collectors.groupingBy(Person::getSex, Collectors.counting()));

        return counted;
    } else {
        throw new InvalidPeopleListException("PeopleList is empty");
    }
}

I like this concept that you make it easy to get a count of either gender... but a string??? what would happen if you asked for "Male", "MALE", "M" or any other combination? What if the file itself is not standardized. This would have been much better as an Enum. then you can parse the file and know that you have a type safe version of it to group by.

---

On all of your methods you test for a condition (!peopleList.isEmpty()) do your business then get out. The problem with this is that I have read all the logic to find out that if it was empty you want to throw an exception. instead reduce the nesting of your methods and throw the exceptions up top or return a default answer. Then you can proceed with your normal flow. To me this just reads a lot easier

public Map<String, Long> countBySex() throws InvalidPeopleListException {
    if (peopleList.isEmpty()) {
        throw new InvalidPeopleListException("PeopleList is empty");
    }

    Map<String, Long> counted = peopleList.stream()
            .collect(Collectors.groupingBy(Person::getSex, Collectors.counting()));

    return counted;

}

public int averageAge() throws InvalidPeopleListException {
    if (peopleList.isEmpty()) {
        throw new InvalidPeopleListException("PeopleList is empty");
    }

    Double average = peopleList.stream()
            .collect(Collectors.collectingAndThen(Collectors.summarizingInt(Person::getAge),
                    dss -> dss.getAverage()));
    return average.intValue();
}

but i can take that a step further and removing the duplicate code

public Map<String, Long> countBySex() throws InvalidPeopleListException {
    verifyPeopleListIsNotEmpty();

    Map<String, Long> counted = peopleList.stream()
            .collect(Collectors.groupingBy(Person::getSex, Collectors.counting()));

    return counted;

}

public int averageAge() throws InvalidPeopleListException {
    verifyPeopleListIsNotEmpty();

    Double average = peopleList.stream()
            .collect(Collectors.collectingAndThen(Collectors.summarizingInt(Person::getAge),
                    dss -> dss.getAverage()));
    return average.intValue();
}

private void verifyPeopleListIsNotEmpty() throws InvalidPeopleListException {
    if (peopleList.isEmpty()) {
        throw new InvalidPeopleListException("PeopleList is empty");
    }
}

So that is all I have. Normally I'd go on a rant about TDD and creating tests and so forth. And i still want to, but I'll let someone else cover it.

Answer 2

Optional.get: Don't do it!

You are breaking the benefits of Optional using this method. The name of this method is a little misleading, it sounds like the method you want to use to get the values, but it is not the case most of the time. orElse to provide a default value or orElseThrow to throw an error if there is no value are usually better alternatives.

In your PeopleList.findPerson method, there is a bug here:

Optional<Person> matching = ...
Person person = matching.get(); // <-- This throws NoSuchElementException if matching has no value.   
if (person != null) { // Person is never null at this point.
    return person;
} else {
    throw new FailedToFindPersonException("Person not in list");
}

Your code would be cleaner, shorter and makes a better use of Optional this way:

Optional<Person> matching = ...
return matching.orElseThrow(() -> new FailedToFindPersonException("Person not in list"));