Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 enter image description here

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 from a properties file via AppConfig.getStudentsDataFile()
 * 
 */
public class DataLoader {
    private final static Logger LOGGER = Logger.getLogger(DataLoader.class .getName()); 

    public static ArrayList<Student> loadStudentsData() {

        ArrayList<Student> studentsCollection = new ArrayList<Student>(100);

            try (BufferedReader br = new BufferedReader(new FileReader( AppConfig.getStudentsDataFile()) ) )    {

                String  recordLine = new String();
                String lineItems[] = new String[3];


                 while  ((recordLine = br.readLine()) != null) {

                     Student student = new Student();
                     StudentTest studentTest = new StudentTest();

                     lineItems = recordLine.split(",")   ;
                     System.out.println( lineItems[0]  + " " + lineItems[1] + " " + lineItems[2] );
                     student.setStudentID( Integer.parseInt(lineItems[1]) ); // second item is ID

                     int index ;

                     if (  (index = studentsCollection.indexOf(student) ) > -1  ) {

                         student = studentsCollection.get(index);

                     }

                     studentTest.setTestDate( Utility.convertStringIntoDate( lineItems[0]  )  ); // first  item is Date
                     studentTest.setTestScore( Float.parseFloat(lineItems[2] )  ); // first  item is Score

                     student.addStudentTest(studentTest);


                     if ( index == -1)  {
                         studentsCollection.add(student);
                     }
                }



            } catch (IOException e) {
             LOGGER.warning( e.getMessage() );
          } 


        return studentsCollection;
    }




    public static String[] getStudentRecordInParts(String record) {


        if (record == null || record.length() < 1 )
            throw new IllegalArgumentException("");

        String items[] = new String[3];
        int index =0;

        StringBuffer item =new StringBuffer();
        for(int i=0; i < record.length() ; i++  ) {  //one record looks like 10-04-2014,1,60

            if (record.charAt(i) != ',') {
                item.append( record.charAt(i) );
            }else {

                items[index] = item.toString() ; //add into array

                item.delete(0, item.length()); //clear previous item
                 index++;
            }

        }
        if (item.length() > -1 ) { //Add last item into array
            items[index] = item.toString() ;            

        }

        //LOGGER.info(items.toString());
        return items;
    }
}

DateSearchController class ( 41 lines )

package com.students.controller;

import java.util.ArrayList;
import java.util.Date;
import java.util.Iterator;
import java.util.TreeSet;

import com.students.model.Student;
import com.students.model.StudentTest;

/*
 * This class works as a controller to search records by date and populate Search Results collection
 * 
 */
public class DateSearchController {



    public static TreeSet<SearchResult> searchStudentScore(  Date frmDate, Date toDate) {
        ArrayList<Student> students = DataLoader.loadStudentsData();

        TreeSet<SearchResult> results =new TreeSet<SearchResult>() ;

        for(Iterator<Student> iterator = students.iterator() ; iterator.hasNext(); ) {
             Student st = iterator.next();
             SearchResult  rs =new  SearchResult();
            for( Iterator<StudentTest> testItrator = st.getStudentTestsCollection().iterator(); testItrator.hasNext(); ) {
                StudentTest stest = testItrator.next();
                 if ( stest.getTestDate().compareTo(frmDate ) >=0  &&  stest.getTestDate().compareTo(toDate) <= 0 ) {

                    rs.setStudentID(st.getStudentID());
                    rs.addScore( stest.getTestScore()  );

                 }
            }
            results.add(rs);
        }

        return results;
    }
}

SearchResult class ( 71 lines )

package com.students.controller;

import java.util.TreeSet;

/*
 * This class will hold all results score and will return final score
 * 
 */

public class SearchResult implements Comparable{

    private int student;
    private TreeSet<Double> scores = new TreeSet<Double>();


    public void setStudentID(int student) {
        this.student = student;
    }

    public int getStudentID() {
        return this.student  ;
    }

    public double getFinalScore() {

        Object[] arr = scores.toArray();
        double finalScore = 0;
        int index = 0;
        for(int i=arr.length - 1; i >= 0; i--) {
            finalScore += (double)arr[i];
            index++;
            if( index == 5 ) break;
        }
        return finalScore / index;//
    }

    public void addScore(double dl) {
        scores.add(dl);
    }

    @Override
    public String toString() {

        return this.student + " - " + getFinalScore();

    }

    @Override
    public boolean equals(Object v) {

        return this.student == ((SearchResult)v).student ;
    }

    @Override
    public int hashCode() {
        int hash = 5;
        hash = 89 + hash + (this.scores != null ? this.scores.hashCode() : 0);
        hash += 89 + hash + this.student;
        return hash;
    }




    @Override
    public int compareTo(Object v) {

        return (this.student - ((SearchResult)v).student);
    } 

}

The Model Package (com.students.model) contains two classes:

Student class (50 lines)

package com.students.model;
import java.util.ArrayList;
import java.util.TreeSet;

/*
 * This is the main student class to hold each record entry
 * 
 */
public class Student {

        private int studentID;
        private ArrayList<StudentTest> testsCollections ;  

        public Student() {
            testsCollections  = new ArrayList<StudentTest>();
        }
        public void setStudentID(int id) {
            this.studentID = id;
        }

        public int getStudentID() {
            return this.studentID  ;
        }

        public void addStudentTest(StudentTest test) {

            this.testsCollections.add(test);
        }

        public ArrayList<StudentTest> getStudentTestsCollection() {
            return this.testsCollections  ;
        }

        @Override
        public boolean equals(Object v) {

            return this.studentID == ((Student)v).studentID ;
        }

        @Override
        public int hashCode() {
            int hash = 5;
            hash = 89 + hash + (this.testsCollections != null ? this.testsCollections.hashCode() : 0);
            hash += 89 + hash + this.studentID;
            return hash;
        }



}

StudentTest class (31 lines )

package com.students.model;

import java.util.Date;
import java.util.TreeSet;

/*
 * This student Test class hold record details by date and score
 * 
 */
public class StudentTest {

    private Date date;
    private double score;

    public void setTestDate(Date date) {
        this.date = date;
    }

    public Date getTestDate() {
        return this.date  ;
    }

    public void setTestScore(double score) {
        this.score = score;
    }

    public double getTestScore() {
        return this.score  ;
    }

}

The View Package (com.students.view) contains only one class:

Command class (29 lines)

package com.students.view;


import java.util.Iterator;
import java.util.TreeSet;

import com.students.controller.DateSearchController;
import com.students.controller.SearchResult;
import com.students.util.AppConfig;

/*
 * Here is the main Class with main method , it is just going to search all loaded recorded and show final results
 * 
 */
public class Command {

    public static void main(String[] arg) {


        TreeSet<SearchResult> sr = DateSearchController.searchStudentScore(  AppConfig.getFromDate(), AppConfig.getToDate());

        for(Iterator<SearchResult> it = sr.iterator(); it.hasNext();) {
            SearchResult st = it.next();
            System.out.println( st  );
        }


    }
}

The Util Package (com.students.util) contains two classes:

AppConfig class (55 lines)

package com.students.util;

import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.util.Date;
import java.util.Properties;
import java.util.logging.Logger;

/*
 * This class loads Application configuration details form properties file
 * 
 */
public class AppConfig {

    public static final Properties prop = new Properties();
    private static AppConfig appConfig;
    private final static Logger LOGGER = Logger.getLogger(AppConfig.class .getName()); 


    static {
        getInstance();
    }

    private  AppConfig() {
        try (InputStream  appConfig= new FileInputStream("config.properties") ) {
           // load properties
            prop.load(appConfig );

        } catch (IOException io) {
            io.printStackTrace();
        }  
    }

    public static synchronized AppConfig getInstance() {
        if (appConfig == null) {
            appConfig = new AppConfig();
        }
        LOGGER.info("loaded instance");
        return appConfig;
      }

    public static String getStudentsDataFile() {
        return prop.getProperty("filepath");
    }

    public static Date getFromDate() {
        return Utility.convertStringIntoDate( prop.getProperty("FromDate") );
    }

    public static Date getToDate() {
        return Utility.convertStringIntoDate( prop.getProperty("ToDate") );
    }

}

Utility class (27 lines)

package com.students.util;

import java.text.DateFormat;
import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.util.Date;
import java.util.logging.Logger;

public class Utility {

    private final static DateFormat formatter =  new SimpleDateFormat("yyyy-MM-dd");
    private final static Logger LOGGER = Logger.getLogger(Utility.class .getName()); 


    public static Date convertStringIntoDate(String date) {

         Date dt= new Date();
          try{
               dt = formatter.parse(date);
          }catch(ParseException e){
              LOGGER.info("Exam Date format Exception: " + e.getStackTrace() ) ;
          }

          return dt;

    }
}

The project has one configuration (config.properties) file that has three things:

  1. The path and name of a data file that contains students records that will be processed in the application and will be shown as per requirements i.e.

    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.

  2. From Date

  3. To Date

The students data is saved in a CVS file under data folder inside the project and its path is set in the config.properties file. Data file can be placed on any path but config.properties path needs to be pointed out correctly.

Each student record will look like ( YYYY-MM-DD , StudentID , Exam Score )

enter image description here

share|improve this question
    
I don't have time for a full review and might do it later, but just a first thing I noticed: Object should not have ArrayList as a type, but only use the interface List. Similarly, use Set as a type instead of TreeSet etc. –  Ingo Bürk Apr 27 at 14:32
    
To add one more thing: Don't use a static DateFormatter field –  Ingo Bürk Apr 27 at 14:45
    
The first thing that came to my mind when reading the exercise: why reinvent the wheel? I'd just drop the CSV into a database and write a single query. Done. –  Prinzhorn Apr 27 at 14:56
1  
@Prinzhorn Not every application can use databases, besides it's an interview question and third-party libraries were disallowed – I think databases would be disallowed as well. –  Ingo Bürk Apr 27 at 14:57

4 Answers 4

up vote 6 down vote accepted

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.

share|improve this answer
    
Thanks alot for detailed reply. –  AAhad Apr 27 at 15:27

Adding to @RoToRa's points:

  • Unused methods DataLoader.getStudentRecordInParts() and SearchResult.compareTo()

  • I don't quite get why AppConfig is specifically constructed to be a singleton, since there really isn't a global state to speak of in your application, much less a good use case for a singleton. I concur with RoToRa, it's perhaps better to read them in and pass to your other methods accordingly.

  • I can't help but feel that your solution is over-engineered for the simple question provided... You document two 'model' classes but SearchResult sounds like one as well. DataLoader creates a new Student class when reading every line, but they get discarded if you can retrieve a previous entry from your List of Students. StudentTest does nothing other than to wrap two values, it's not even properly used by SearchResult since that takes in the numeric test scores straight. All these mean that you have effectively employed three different classes just to read three values into a List and to perform the required computation.

  • Your equals() and hashcode() implementations in both Student and SearchResult is inconsistent. This is confusing to other programmers at best, and will introduce bugs at worst because modifying these classes may unknowingly alter whether instances will still be equal to each other or not.

share|improve this answer
    
Thanks for pointing out mistakes. –  AAhad Apr 27 at 17:10

You could have used a Map instead of ArrayList for students collection. Here in the below code you are searching for student for each record. If you have 100 students and average 6 records per student then you have worst case complexity of O(600). So using Map makes sense here.

 if (  (index = studentsCollection.indexOf(student) ) > -1  ) {

                     student = studentsCollection.get(index);

                 }

Other thing is for the exception you are throwing here. You can definately add some meaningful message to this exception. Makes no sense in hiding that record was null or empty.

if (record == null || record.length() < 1 )
        throw new IllegalArgumentException("");

Yes I also feel that the whole application is over engineered. This task shouldn't be this much verbose. If I am given authority of judging a guy who writes this much code for simple task then I would be a little worried. The reason is that I am maintaing an application which is way too much over engineered intentionally and now we are facing a hard time maintaing it. I shared my thoughts so that you can see the other aspect as well . Hope this helps.

share|improve this answer
    
Thanks for sharing your thoughts, appreciated. –  AAhad Apr 27 at 17:12

Something nobody else noticed is the Utility class is not thread safe since it re-uses an existing SimpleDateFormat see Synchronization section in the SimpleDateFormat API. SimpleDateFormat is not thread safe and must be synchronized externally if an instance is re-used. The Utility class can also hide the parse exception (logging it as INFO which may also be missed) resulting in an incorrect date being returned.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.