School which registers students by preferred courses

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

-
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?

-
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?

-
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

-
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?

-
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

-
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, w

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.

StackExchange Code Review Q#70630, answer score: 4