Adding a course to one of 8 periods

Question

I have this rather ghastly if statement and I'm sure there is a way to condense it, but I don't really know how. I was told a case statement would work but I've never used a case statement and I'm not sure.

public void add(int p, Course c)
{
    if (p == 1){
    if (course1.isFree() == true) {
        course1 = c;
        System.out.println("Course " + c + "added to period " + p +".\n");
    } else {
        System.out.println("Unable to add class to a nonfree period.");
    }
    } else if (p == 2){
        if (course2.isFree() == true) {
        course2 = c;
        System.out.println("Course " + c + "added to period " + p +".\n");
    } else {
        System.out.println("Unable to add class to a nonfree period.");
    }
    } else if (p == 3){
        if (course3.isFree() == true) {
        course3 = c;
        System.out.println("Course " + c + "added to period " + p +".\n");
    } else {
        System.out.println("Unable to add class to a nonfree period.");
    }
    } else if (p == 4){
        if (course4.isFree() == true) {
        course4 = c;
        System.out.println("Course " + c + "added to period " + p +".\n");
    } else {
        System.out.println("Unable to add class to a nonfree period.");
    }
    } else if (p == 5){
        if (course5.isFree() == true) {
        course5 = c;
        System.out.println("Course " + c + "added to period " + p +".\n");
    } else {
        System.out.println("Unable to add class to a nonfree period.");
    }
    } else if (p == 6){
        if (course6.isFree() == true) {
        course6 = c;
        System.out.println("Course " + c + "added to period " + p +".\n");
    } else {
        System.out.println("Unable to add class to a nonfree period.");
    }
    } else if (p == 7){
        if (course7.isFree() == true) {
        course7 = c;
        System.out.println("Course " + c + "added to period " + p +".\n");
    } else {
        System.out.println("Unable to add class to a nonfree period.");
    }
    } else if (p == 8){
        if (course8.isFree() == true) {
        course8 = c;
        System.out.println("Course " + c + "added to period " + p +".\n");
    } else {
        System.out.println("Unable to add class to a nonfree period.");
    }
    }

    totalcredits();
}

Answer 1

    if (course3.isFree() == true) {
    course3 = c;
    System.out.println("Course " + c + "added to period " + p +".\n");
} else {
    System.out.println("Unable to add class to a nonfree period.");
}

...

} else if (p == 8){
    if (course8.isFree() == true) {
    course8 = c;
    System.out.println("Course " + c + "added to period " + p +".\n");
} else {
    System.out.println("Unable to add class to a nonfree period.");
}

Good thing there aren't 20.. 200... 2,000... 20,000 courses!

This code needs to DRY a bit. Don't repeat yourself.™

Programming is about abstractions - this code need one here, badly: there should be a concept of a bunch of courses in your program.

Then it would be possible to only write the block once. Given courses is an array of courses Course... of course:

public void add(int period, Course course) {
|
|---if (courses[period].isFree()) {
|   |
|   |---courses[period] = course; 
|   |---System.out.println("Course " + course + "added to period " + period +".\n");
|   |
|---} else {
|   |
|   |---System.out.println("Unable to add class to a nonfree period.");
|---}
|
}

Notice the position of the scope-closing braces: by lining them up with the indentation of the line that opened the scope, you make the code much easier to follow, and avoid this:

    }
    }

Also notice the names I've used. Avoid single-letter identifiers, like p and c. Code is much easier to read when it's... readable.

---

This is too verbose:

if (course.isFree() == true) {

Since course.isFree() obviously returns a Boolean value, the Boolean value itself can serve as the expression of the condition, which means you don't need to compare it to true:

if (course.isFree()) {

Answer 2

@Mat's Mug has a pretty good answer, and I'll like to go one step further with a couple more questions:

1. What are course1...course8? Global variables? Any reasons why they are not a Collection of courses?
2. What exactly is the relationship between p, c and course1...course8? Is p some form of user input indicating the desired period the course c should go to, to be set for one of course<n>? The reason why I ask this is because clearly you are only checking whether course<n> is free for period <n>.
3. How is calling totalcredits() related to adding a course to one of 8 periods?

If you can post a follow-up question by answering some of the above (say with more code from other parts), then I think there's a lot more that we can help you to improve on.

edit:

Let's attempt to break down the actions into some steps.

1. We need to get the course for the specified period value, so we may have something like this:

   private Course getCourse(int period) {
       return courses.get(period - 1); // assuming period is not zero-based
   }
   

2. We can now call getCourse(period) inside your add method:

   public final void add(int period, final Course course) {
       final Course selectedCourse = getCourse(period);
       if (selectedCourse.isFree()) {
           // do something about course
           // print success
       } else {
           // print failure
       }
   }
   

3. What does it mean to set course<n> to the course input? Wouldn't you need to do something like setFree(false) too to indicate the course is 'no longer free'? This is why I am suggesting that you re-evaluate the logic you require for adding a course to a period and marking that period/course as occupied. You may want to do something like the following:

   if (selectedCourse.isFree()) {
       selectedCourse.copyFrom(course); // maybe some checks on 'free' will be done too
   }
   

Answer 3

Since you are testing equality for integer you use switch instead of multiple if-else if.

switch(p){
    case 1:
       break;
    case 2:
       break;

}

The code above is equivalent to

if(p == 1){

}
else if(p == 2){

}

You can use the shorthand if using the operator ? for simple if-else. Thus, your code will look something like this:

public void add(int p, Course c){
    Course currentCourse;
    switch(p){
     case 1:
           //if course1.isFree, currentCourse is c, else currentCourse is course1
           currentCourse = course1.isFree() ? c : course1;
           break;
     case 2:
           currentCourse = course2.isFree() ? c : course2;
           break;

     <add more cases here>
     }

     //display appropriate message
     if(currentCourse.equals(c)){
         System.out.println("Course " + c + "added to period " + p +".\n");
     }else{
         System.out.println("Unable to add class to a nonfree period.");
     }
}