Simple object oriented design of student system

Question

I have created a simple system to get hands on experience with OOPS design and some features in Java:

The system is:

• A ClassOfStudents contains Students
• A Student contains a list of scores
• A Student can be a PartTimeStudent or FullTimeStudent
• A score is valid only if it is between 20 and 100

I have:

• Used enum for gender as it is a set of constants
• Created a user-defined exception to check for business rule
• Made student as abstract to make it extensible
• Used compare to reverse the natural order of sort and able to sort students

Queries

Please go through the code and feel free to advice all the OOPS design enhancements. I am new to OOPS and want to really understand.How to make code more extensible, reusable, secure. Some of the questions I have encountered while coding are:

1. I don't want to create a object when score is not valid. I can achieve this by checking for score before using creating an object. But is there any other way?
2. I am throwing an user defined exception and catching it immediately. Is it good practice? I don't want to interrupt the flow and continue.
3. I am using logging for the first time. Is it the good way to log?
4. I tired to implement as many as OOPS concepts but am unable to think of an interface. Please suggest a good use case to use an interface.
5. How can I improve the exception handling (robustness)? Is there any other way I can add the student to studentList in the ClassOfStudents whenever a new student is created?
6. Also suggest some new feature I can add to learn more OOPS/Java concepts.

I have poster another question similar and got great response. I haven't implemented some of the feature like return immutable lists, not use throws for main here but grasped the concepts.

import java.util.*;
import java.util.logging.*;


//I am trying to write extensible calss. So I have declared Student as abstact.
//FullTimeStudent and PartTimeStudent sub classes 
//Moreover a Student need to be a FullTime are PartTime so Student object cannot be created.
//I am using protected to scoreList. It is a good way?

class ClassOfStudents
{
  private List<Student> studentList = new ArrayList<Student>();
  public void addStudent(Student student)
  {
    studentList.add(student);
  }

  public List<Student> getStudentList()
  {
    return studentList;
  }
}

abstract class Student implements Comparable<Student>
{
    private String name;
    private Address address;
    private Gender gender;
    protected List<Score> scoreList = new ArrayList<Score>(); //Because the subclass need to access


    Student(String name) 
    {
        this.name = name;
        this.gender = Gender.UNKNOWN;
    }  

    Student(String name, Gender gender) 
    {
        this.name = name;
        this.gender = gender;
    }  

    public void setName(String name)
    {
        this.name = name;
    }

    public String getName()
    {
        return name;
    }

    public String toString()
    {
        return name+" "+gender;
    }

    public void addScore(Score score)
    {
        scoreList.add(score);
    }

    public List<Score> getScores()
    {
        return scoreList;
    }

    // Reverse of natural order of String.
    public int compareTo(Student otherStudent)
    {
      return -1 * this.name.compareTo(otherStudent.getName());
    }

    public abstract boolean checkScores();

}

//Inheritance
class FullTimeStudent extends Student
{
    FullTimeStudent(String name)
    {
      super(name);      
    }

    FullTimeStudent(String name, Gender gender) 
    {
      super( name,  gender);
    }  
    public boolean checkScores()
    {
      for(Score score : scoreList)
      {
        if (score.getStatus() == false)
          return false;
      }  
      return true;
    } 
}

//Inheritance
class PartTimeStudent extends Student
{
    PartTimeStudent(String name)
    {
      super(name);      
    }

    PartTimeStudent(String name, Gender gender) 
    {
      super( name,  gender);
    }  

    public boolean checkScores()
    {
      int countScoreFail = 0;
      for(Score score : scoreList)
      {
        if (score.getStatus() == false)
          countScoreFail++;
      }  
      System.out.println(countScoreFail);
      if (countScoreFail >= 3)
        return false;
      else 
        return true;
    } 
}



class Address
{
    private String streetAdress1;
    private String phoneNumber;
    private String zipCode;
    //Constructor, Setters and getters of Address
}

enum Gender
{
    MALE,FEMALE,OTHER,UNKNOWN;
}

// Score can be between 20 to 100. 
//Score can only be incrmented / decremented by 1.
//If Score < 40 , then status is false. Else true

//I dont want to create a object when score is not valid. I can do this by checking for score before using new. But is there any other way?
//I am throwing an user defined exception and catching it immediately, is it a good practice. I dont want to disturb the flow and continue?
//I am using logging for the first time. Is it the good way to write this?
class Score
{
    private int score;
    private boolean status = false;
    Score(int score) throws scoreException
    {
        setScore(score);
    }

    public void setScore(int score) throws scoreException
    {
        if(score < 20 || score > 100)
        {
          try{
            System.out.println("Invalid Score!!!");
            throw new scoreException();
          }
          catch(scoreException e)
          {
            Logger logger = Logger.getLogger("myLogger");
            logger.log( Level.FINE,"Hello logging");
          }
        }
        else
        {
            this.score = score;
            if(score >= 40)
                status = true;
        }
    }

    public int getScore()
    {
        return score;
    }

    public boolean getStatus()
    {
      return status;
    }

    public String toString()
    {
        return score+" "+status;
    }
}

class scoreException extends Exception
{
    public String toString()
    {
        return "Entered Marks are not valid";
    }
}


public class Test{

     public static void main(String []args)throws scoreException
     {
       //Polymorphism
        ClassOfStudents c1 = new ClassOfStudents();
        Student s1 = new FullTimeStudent("John");
        Student s2 = new PartTimeStudent("Nancy",Gender.FEMALE);
        c1.addStudent(s1);
        c1.addStudent(s2);

        List<Student> studentList = c1.getStudentList();
        Collections.sort(studentList);


        for(Student student : studentList)
        {
          System.out.println(student);
        }

     //*************************            

        s1.addScore(new Score(10));
        s1.addScore(new Score(50));
        s1.addScore(new Score(30));


        System.out.println("Student is "+s1);
       //Even for invalid score objects are created. I dont want them to be created.
        System.out.println("Printing content of all the scores of student");
        for(Score score : s1.getScores())
        {
            System.out.println(score);
        }

       System.out.println("Are all scores greater than 40?? ::"+s1.checkScores());

        //****************************
       System.out.println("Student is "+s2);       
        s2.addScore(new Score(10));
        s2.addScore(new Score(50));
        s2.addScore(new Score(30));        

       //Even for invalid score objects are created. I dont want them to be created.
        System.out.println("Printing content of all the scores of student");
        for(Score score : s2.getScores())
        {
            System.out.println(score);
        }

       System.out.println("Are all scores greater than 40?? ::"+s2.checkScores());
     }
}

Answer 1

Bracing style

You have used mostly the Allman-style bracing approach, but there are momentary lapses in your try-catch and the Test class. In fact, I will also suggest introducing braces for the if-statements. Regardless of the styles you choose, please be consistent on this front. :)

Constructor chaining

Student(String name) 
{
    this.name = name;
    this.gender = Gender.UNKNOWN;
}  

Student(String name, Gender gender) 
{
    this.name = name;
    this.gender = gender;
}  

One of these two constructors should be chained to the other, which will facilitate in making your fields final as well:

class Student
{

    private final String name;
    private final Gender gender;

    Student(String name) 
    {
        this(name, Gender.UNKNOWN);
    }  

    Student(String name, Gender gender) 
    {
        this.name = name;
        this.gender = gender;
    }

    // ...
}  

Reverse comparisons

If you happen to be on Java 8, you can make use of Comparator.reverseOrder() to do this for you:

private static final Comparator<String> REVERSE_COMPARATOR = Comparator.reverseOrder();

public int compareTo(Student otherStudent)
{
    // return -1 * this.name.compareTo(otherStudent.getName());
    return REVERSE_COMPARATOR.compare(name, otherStudent.name);
}

Can a full-time students become part-time, and vice versa?

Your current implementations for FullTime and PartTime students are fine, but you may also want to consider the relationships between the types of students, the common fields/methods/properties of students, and how the scores are 'checked'. An alternative solution is to instead consider full-timers or part-timers as only a status to any Student, similar to what you are doing for gender now:

// switching to Java bracing convention for illustration
enum StudentType {
    FULL_TIME {
        @Override
        public boolean checkScores(Student student) { ... }
    },
    PART_TIME {
        @Override
        public boolean checkScores(Student student) { ... }
    };

    public abstract boolean checkScores(Student student);
}

In this case, a Student (which is now non-abstract) can be toggled between full-time and part-time status, and arguably any Collection of students can be easily filtered by checking student.getType() == StudentType.FULL_TIME, instead of FullTimeStudent.class.isInstance(student). Of course, if you start to have more specific methods for each type of students, then the inheritance way will then become a better modeling approach.

What's in a Score?

As it stands, I'm not too sure about the usefulness of your Score class. It is nothing but a wrapper over an int now, and even the boolean status can be easily derived from the score. In any case, MAG's answer offers some suitable enhancements to the class, so I'll suggest taking a look at that.

//I am throwing an user defined exception and catching it immediately, 
//is it a good practice. I dont want to disturb the flow and continue?
//I am using logging for the first time. Is it the good way to write this?

Throwing a specific Exception and catching that does seem... a little odd. It is as-if you are trying to control execution flow... Anyways, ScoreException (note: PascalCase for the class name) also seems to be a redundant class, as the built-in IllegalArgumentException ought to be good enough to convey the same exception cause.

As for logging, you may want to take a look at logging frameworks such as SLF4J to handle logging in your codebase. On a related note, this StackOverflow question provides some useful insight regarding for-and-against the java.util.logging.* classes that you have adopted.

Answer 2

You should make your Score class immutable. That way you just need to enforce invariants in the constructor. For more information on immutability you should read Item 15: Minimize Mutability from the book Effective Java by Joshua Bloch. Also your constructor is package private so Score can only be created inside the same package in which it is define. There are other classes that would benefit from immutability (for example Address).

This would be my version on Score :

public final class Score
{

    private final static int  THRESHOLD =40;

    private final int score;
    // There is no point on catching an exception after throwing it
    Score(int score)
    {
        if(score < 20 || score > 100)
            throw new IllegalArgumentException("invalid score");

        //Always initialize non-static members in the constructor
        this.score= score;      
    }

    public Score changeScore(int score)
    {
        return new Score(score);
    }

    public int getScore()
    {
        return score;
    }

    // There is no point in having a boolean member 
    public boolean getStatus()
    {
      return score >= THRESHOLD;
    }

    //Always use override
    @Override
    public String toString()
    {
        return score+" "+getStatus();
    }
}

Also never do this:

if (countScoreFail >= 3)
        return false;
      else 
        return true;

This always can be condense to single boolean expression:

return countScoreFail < 3;

Finally I would also recommend you to use more interfaces to minimize coupling. (Look up SOLID principles).