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:
- How many people in the list are male?
- In years what is the average age of the people in the list?
- 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 inManipulateDataApplication
?- 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.
Clearly I failed because the recruitment agent is avoiding me.
Not necessarily true. \$\endgroup\$System.out.print
and you useSystem.out.println
;-) !!! ??? ;-) \$\endgroup\$