6
\$\begingroup\$

The point of the application is to create a school, create courses for that school, create students and register them to the school, assigning them courses based off the courses they prefer.

There are quite a few things I find messy, and hope to get some seasoned advice on it. I put comments where I found they were needed, but I'll accept criticism and opinions on ANY part of this program. The main topics I wanted seasoned advice on:

General Programming Practices

  1. Usage of arrays, lists and sets. Do the ones I picked fit in correctly? Or should I use some other form of storing for certain parts?

  2. I've heard constructors shouldn't contain logic; it's best to have constructors do nothing more than initialize fields (I've heard you shouldn't even start a thread in a constructor, only initialize it if needed). What are some clean workarounds to this?

  3. I'm using class literals to specify which courses the students prefer. This is because there could be multiple NetworkCourses, same with the other courses. Is this proper usage? Is there a better, more practiced way to manage this?

Specific to Application

  1. If the student does not have any preferred classes, or if the courses they want are full, a course will be given to them at random. For this, I have a while loop, grabbing a random course and assigning it to the student. the way I'm doing it seems pretty intense for the goal I'm trying to achieve. Is there a cleaner way to do it?

  2. School#register(Student[]) is pretty messy, with all the loops and calling field variables using this to avoid shadowing, but allow conventional parameter names. I would love to hear opinions on how this should be done

  3. My encapsulation fell off. I attempted to keep things hidden by making them package private, but it's hard to create sub-packages for certain things without changing the visibility of a lot of things. My idea of how things should be packaged changes quite often. I tend to put subclasses in a package, while I leave the superclass in the original package. (so school would contain Entity superclass, while school.entity would contain the subclasses). If someone could point me in a good direction of how I should encapsulate this (all opinions welcome), that'll mean a lot

  4. All other least-important questions are commented in the code, but I'm still hoping to get opinions on them.

Quick note: I don't use import wildcards. I wrote this using AIDE on my phone, which uses wildcards when you auto-import.

Starting with Main.java, where I tested everything:

import school.*;
import school.courses.*;

public class Main {
    public static void main(String[] args) {
        Teacher phil = new Teacher("Phil");
        Teacher bill = new Teacher("Bill");
        Teacher lil = new Teacher("Lil");
        Teacher joe = new Teacher("Joe");

        Course[] courses = {
            new NetworkCourse(15, phil),
            new SwingCourse(30, bill),
            new APIDesignCourse(50, lil),
            new PerformanceCourse(5, joe)
        };
        School school = new School(courses); 

        Student ludwig = new Student("Ludwig");
        Student cam = new Student("Cam");
        Student daniel = new Student("Daniel");     
        ludwig.setPreferredCourses(NetworkCourse.class, SwingCourse.class); //give students preferred classes if they have them
        cam.setPreferredCourses(APIDesignCourse.class, PerformanceCourse.class, NetworkCourse.class);

        school.register(ludwig, cam, daniel);

        /*
         * Below is where we test by printing things out
         */
        test(school);
    }

    static void test(School school) {
        /*
         * Prints all the students in the school, all the courses in the school, and which course each student has
         */
        System.out.println("Students and their courses:");
        for(Student student : school.getStudents()) {
            if(student != null) {
                String message = student.getName() + " is taking"; //message will reset for each new student, since we do = and not += here

                for(Course course : student.getCourses())
                    message += " - " + course.getName();

                System.out.println(message);
            }
        }

        System.out.println("\nCourses and their students:");
        for(Course course : school.getCourses()) {
            String message = course.getName() + " is taken by";

            for(Student student : course.getStudents()) {
                if(student != null)
                    message += " - " + student.getName();
            }
            System.out.println(message);
        }
    }
}

School.java

package school;

import java.util.*;

public class School {
    private Course[] courses;
    private Student[] students;

    public School(Course[] courses) {
        this.courses = courses;

        //ive been told that constructors shouldnt contain logic. is there any other way to handle this?
        int numOfStudents = 0;
        for(Course course : courses)
            numOfStudents += course.getStudents().length;

        students = new Student[numOfStudents];
    }

    public void register(Student...students) { //this method is pretty messy, and loops quite a few times. any suggestions?
        if(isFull())
            throw new IllegalStateException("Cannot register anymore students at this time");

        for(Student student : students) {
            if(Arrays.asList(this.students).contains(student)) //wrapping the array every loop. is there any better way to do this, without creating my own private contains method for students?
                throw new IllegalArgumentException("You cannot add the same student to a school twice"); //should I be throwing a runtime exception here? or should i just continue with the rest of the students

            for(Course course : courses) {
                if(student.prefersCourse(course) && !course.isFull())
                    student.assignCourse(course);
            }

            verifyStudent(student); //make sure the student is ready for school
            student.setSchool(this);
            for(int i = 0; i < this.students.length; i++) {
                if(this.students[i] == null) {
                    this.students[i] = student;
                    break;
                }
            }
        }
    }

    private void verifyStudent(Student student) {
        verifyCourses(student);
        //more will be added here later
    }

    private void verifyCourses(Student student) {
        boolean verified = false;

        //assigns a random course. is there a cleaner way to handle this?
        while(!verified) { 
            for(Course course : student.getCourses()) {
                if(course == null) {
                    int index = (int) (Math.random() * courses.length);
                    student.assignCourse(courses[index]);
                }
            }

            verified = !Arrays.asList(student.getCourses()).contains(null);
        }
    }

    public Student[] getStudents() {
        return Arrays.copyOf(students, students.length);
    }

    public Course[] getCourses() {
        return Arrays.copyOf(courses, courses.length);
    }

    public boolean isFull() {
        boolean full = true;
        for(Student student : students)
            if(student == null)
                return full = false;

        return full;
    }
}

Course.java

package school;

import java.util.*;

public abstract class Course {
    private Teacher teacher;
    private Student[] students;
    private UUID id;

    protected Course(int maxStudents, Teacher teacher) { //might allow multiple teachers later
        students = new Student[maxStudents];
        this.teacher = teacher;

        id = UUID.randomUUID();
    }

    void addStudent(Student student) {
        for(int i = 0; i < students.length; i++) {
            if(student == students[i])
                continue;

            if(students[i] == null) {
                students[i] = student;
                return;
            }
        }
    }   

    public Teacher getTeacher() {
        return teacher;
    }

    public Student[] getStudents() {
        return Arrays.copyOf(students, students.length);
    }

    public boolean isFull() {
        boolean full = true;

        for(Student student : students)
            full = student != null;

        return full;
    }

    public abstract String getName();
}

Student.java

package school;

import java.util.*;

public class Student extends Entity {
    private School school;
    private Course[] courses;
    private Set<Class<? extends Course>> preferredCourses;

    public Student(String name) {
        super(name);
        courses = new Course[2];
        preferredCourses = new HashSet<>();
    }

    public void setPreferredCourses(Class<? extends Course>...courses) {        
        for(Class<? extends Course> course : courses) {
            preferredCourses.add(course);
        }
    }

    void assignCourse(Course course) {
        for(int i = 0; i < courses.length; i++) {
            if(course == courses[i])
                continue;

            if(courses[i] == null) {
                course.addStudent(this);
                courses[i] = course;
                return;
            }
        }
    }

    void setSchool(School school) {
        this.school = school;
    }

    public School getSchool() {
        return school;
    }

    public Course[] getCourses() {
        return Arrays.copyOf(courses, courses.length);
    }

    public boolean prefersCourse(Course course) {
        return preferredCourses.contains(course.getClass());
    }

    public boolean isTakingCourse(Course course) {
        boolean contains = false;
        for(Course c : courses)
            return contains = (c == course);

        return contains;
    }
}

Teacher.java

package school;

public class Teacher extends Entity {
    public Teacher(String name) {
        super(name);
    }
}

Entity.java

package school;

public abstract class Entity {
    private String name;

    protected Entity(String name) {
        this.name = name;
    }

    public String getName() {
        return name;
    }
}

APIDesignCourse.java (the Course subclasses are similar)

package school.courses;

import school.*;

public class APIDesignCourse extends Course {

    public APIDesignCourse(int numOfStudents, Teacher teacher) {
        super(numOfStudents, teacher);
    }
    public String getName() {
        return getTeacher().getName() + "'s API Design Course";
    }
}

NetworkCourse.java

package school.courses;

import school.*;

public class NetworkCourse extends Course {
    public NetworkCourse(int numOfStudents, Teacher teacher) {
        super(numOfStudents, teacher);
    }

    public String getName() {
        return getTeacher().getName() + "'s Network Course";
    }
}

PerformanceCourse.java

package school.courses;

import school.*;

public class PerformanceCourse extends Course {
    public PerformanceCourse(int numOfStudents, Teacher teacher) {
        super(numOfStudents, teacher);
    }

    public String getName() {
        return getTeacher().getName() + "'s Performance Course";
    }
}

SwingCourse.java

package school.courses;

import school.*;

public class SwingCourse extends Course {

    public SwingCourse(int numOfStudents, Teacher teacher) {
        super(numOfStudents, teacher);
    }

    public String getName() {
        return getTeacher().getName() + "'s Swing Course";
    }
}
\$\endgroup\$
4
\$\begingroup\$

Arrays are troublesome in many ways. Unless you have a specific need for arrays, it's better to use lists.


This doesn't look great:

    for(Student student : school.getStudents()) {
        if(student != null) {

In reality, would a school tell you that they have "null" students? No, they would tell you they have no students (= empty set). Redesign to make school return an empty collection when there are no students. Such null checks are not ergonomic, noise.

The School constructor

public School(Course[] courses) {
    this.courses = courses;

    //ive been told that constructors shouldnt contain logic. is there any other way to handle this?
    int numOfStudents = 0;
    for(Course course : courses)
        numOfStudents += course.getStudents().length;

    students = new Student[numOfStudents];
}

A couple of things to consider here:

  • If you use a list for students instead of an array, the counting of numOfStudents becomes unnecessary here, you can drop the loop, and your dilemma about logic in the constructor naturally goes away. More specifically, it seems it will make sense to keep the students in a Set, to ensure they are unique.
  • A bigger problem I see is the concept of initializing a school from courses and the students in those courses. That is, that students belong primarily to courses instead of the school. That seems unnatural as a model. I would imagine a bit differently: a School has Courses, Students and Enrollments, where an Enrollment is to hold the link between students and courses.
  • I recommend to consistently use braces always, even when there is a single statement in the for or if statement

School.register

public void register(Student...students) { //this method is pretty messy, and loops quite a few times. any suggestions?
    if(isFull())
        throw new IllegalStateException("Cannot register anymore students at this time");

It might be better to use a custom checked exception and give callers a way to handle gracefully. In real life, if a school rejects your application because they are full, you would try a different school, or if all schools are full then create your startup in the garage and become a millionaire.

Also, you check isFull here, but not again later. What if the school can take N students, there are N-1 students now so it's not full and the method receives 10 new students?

So instead of checking .isFull up front, you should check it before adding each student.

        // wrapping the array every loop. is there any better way to do this,
        // without creating my own private contains method for students?
        if(Arrays.asList(this.students).contains(student)) 
            throw new IllegalArgumentException("You cannot add the same student to a school twice");
            //should I be throwing a runtime exception here?
            // or should i just continue with the rest of the students?

As mentioned earlier, it would be good to keep students in a Set. Then this problem naturally goes away.

        for(Course course : courses) {
            if(student.prefersCourse(course) && !course.isFull())
                student.assignCourse(course);
        }

        verifyStudent(student); //make sure the student is ready for school

This made me think this at first:

It is strange that you first assign a student to a course, and "verify" it after. Shouldn't it be the other way around?

After I read the implementation of verifyStudent I saw that it assigns to a random course. So it's a very misleading name.

The for loop would be good to extract to another method called assignToPreferredCourses, and rename verifyStudent to assignToRandomCourse.

As you wrote in a comment, if you want to "make sure the student is ready for school", create a separate method for that, and check it before accepting the student to the school, not when assigning to courses.

        for(int i = 0; i < this.students.length; i++) {
            if(this.students[i] == null) {
                this.students[i] = student;
                break;
            }
        }

What an awkward code, just to add a student to the school. Use a Set and replace this loop with a single .add operation.

More...

The rest of the code has similar issues. I suggest to replace all the arrays in your code with sets or lists, and look for the opportunities of simplification it will give you. Also think about the overall design of classes, their responsibilities and relationships. Creating a structure that resembles reality (students belong to schools first, courses second) may help in designing something that's natural and easy to understand.

Once rewritten, you could post another question for another round of review.

\$\endgroup\$
  • \$\begingroup\$ So courses in School should also be a Set, right? I create the school requiring courses because a school isn't really a school without courses to take, which makes requiring the courses for construction necessary in my eyes. It allows me to see how many students will be in the school, also allowing each course to specify how many students can be handled. With that said, is it still illogical to do it that way? The only way to register the students is through a School instance, so I felt the logic is alright. If not, mind elaborating on the model you speak of above? (with Enrollments) \$\endgroup\$ – Vince Emigh Nov 23 '14 at 9:31
  • 1
    \$\begingroup\$ The courses and students should be sets in a school. Courses and students don't need to be required to create a school. Courses and students can be registered later after construction. Managing the mapping of students to courses is a tricky topic: which class should be the one authority? How to check and enforce integrity? Probably you'll need to try different ways to organize and compare them to see which one is the best both in terms of logic and ergonomy. There may many equally good solutions. \$\endgroup\$ – Stop ongoing harm to Monica Nov 23 '14 at 10:13
  • 1
    \$\begingroup\$ Your suggestions are golden, thank you so much! I have one last question before my rewrite: I currently have assignCourse encapsulated in the Student class (package-private method). I hid it so courses must be assigned through the school. If I put Student in a sub-package school.entity, and make a private assignCourse(Student, Course) method in School, the only way to assign courses properly would be to switch Set<Course> from student to Entity (which is in school package), and make the set package-private so I can access it from School. Would you recommend this? \$\endgroup\$ – Vince Emigh Nov 23 '14 at 19:27
  • 1
    \$\begingroup\$ It's a bit difficult to understand your last question. But I can say a few things. 1. I agree with putting School in charge of assigning students to courses. Centralize that logic in one place. 2. I wouldn't worry yet about fine tuning the class/method visibility. Work on the overall organization first, try a few alternatives, and when you find something that you like and feels natural, logical and easy to use, then tune the visibility settings (make everything as private as you can). \$\endgroup\$ – Stop ongoing harm to Monica Nov 23 '14 at 22:27
  • 1
    \$\begingroup\$ You're right, I shouldn't spend so much time worrying about little things til after I'm finished. Thank you once again :) \$\endgroup\$ – Vince Emigh Nov 23 '14 at 22:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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