Calculate final test score from a list of test results

Recently I have given an interview to a widely well-known technology firm. The first interview was general and it went well but they rejected based on some technical things. They did not mention it so I expect may be due to my Coding Exercise that I submitted is not well designed.

It would be great if someone point out me my mistakes in the coding exercise. I have uploaded complete project code at git. https://github.com/aahad/Interviews/

The Coding Exercise description was:


Given a list of test results (each with a test date, Student ID, and
the student’s Score), return the Final Score for each student for a
user inputted date range. A student’s Final Score is calculated as the
average of his/her 5 highest test scores. You can assume each student
has at least 5 test scores.


We will evaluate your answer on:

• Correctness, does it work?

• Is it tested?

• Is it well designed?

• Is the code easy to comprehend?

• Would this code be easy to extend or maintain?

Your implementation must be your own, making use of your standard
library. Please feel free to consult any relevant documentation.

The code works fine as per the given requirements and it does not have any issues.

I only expect that it might be possible that it is not designed well OR it could be designed in much better way, OR its not fast, scalable etc. These might be the reasons that's why got rejected.

Code

The code can also be found on https://github.com/aahad/Interviews/

Project Structure

The Controller Package (com.students.controller) has three classes:

DataLoader ( 102 lines )

```
package com.students.controller;

import java.io.BufferedReader;
import java.io.FileReader;
import java.io.IOException;
import java.util.ArrayList;
import java.util.logging.Logger;
import com.students.model.Student;
import com.students.model.StudentTest;
import com.students.util.AppConfig;
import com.students.util.Utility;

/*
* This class is loading Student Data fro

General

• Your formatting isn't very clean. You have random spaces, empty lines and indentations everywhere.

• Use interfaces instead of concrete types where possible.

• You use a logger, but either hardly use it, or write unhelpful information. Either use it properly and consistently everywhere with helpful messages, or drop it entirely.

Class Student

• EDIT: Since the ID is integral part of the student class you should set it in the constructor and remove setStudentID since it makes no sense to change it after the fact.

• testsCollections isn't a good field name. It's a single collection of test so testsCollection or even better just tests.

• getStudentTestsCollection (better: getTests) shouldn't return your internal list representation otherwise external code could modify your list. Use collections.unmodifiableList to protect from that.

• Don't include the tests into the hash code. Two Student objects with the same ID represent the same student irregardless of the assigned tests. Also keep in mind that two objects that are equal() must return the same hash code.

class StudentTest

-
StudentTest is a bad name. It hasn't anything to do with students. Test is fine or - if you think could be confused with development tests - maybe TestScore.

-
Drop Test from all the getters and setters.

class AppConfig

AppConfig may be a bit over-engineered for such a simple project. Representing each setting as it's own getter leads to a lot of maintenance every time you add a new setting. Using Properties directly or just a thin wrapper around it would suffice IMHO. Actually in this case I'd drop it all together and get the parameters from the command line, since theses aren't settings, but data.
class DateSearchController

• Loading the data doesn't belong in the search method. It should be done before.

• Iterators? Seriously? It's (check watch) 2014. Use the enhanced for loop.

class DataLoader

• Consider using a third party CSV file reader.

• Prefilling recordLineand lineItems makes no sense what so ever since they are overwritten. You are aware that Strings are immutable?

• Also lineItems is only used inside the while loop, so it be declared there.

• Now it's getting confusing. You first create a Student object, set its ID, but then search your list of Students for an one with the same ID and if you find one, throw the one you just created away without using it? Or looking at the bigger picture: Each line in the file represents a Test, yet you create a Student object for each line.

• What is getStudentRecordInParts? You don't seem to use it.

EDIT: Added more stuff.

StackExchange Code Review Q#48320, answer score: 6