25
\$\begingroup\$

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();
}
\$\endgroup\$
  • \$\begingroup\$ I would start by fixing the indentation, to be sure what you are dealing with, \$\endgroup\$ – njzk2 Oct 1 '14 at 20:52
34
\$\begingroup\$
    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()) {
\$\endgroup\$
12
\$\begingroup\$

@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
    }
    
\$\endgroup\$
  • \$\begingroup\$ To be honest, most of the things done in this are simply to meet the requirements of my AP-comp sci class... So as long as it works its okay with me. He wanted us to do it this way so I did, I would have wanted an array. Course 1-8 are actually objects and I used a constructor to build them, its a rather rudimentary system, and they're are far more effective ways to do so. \$\endgroup\$ – RexPRGMER Oct 1 '14 at 2:36
  • 3
    \$\begingroup\$ Hmms, if you still prefer course1...course8, maybe you can still consider my idea of a separate getCourse() method to hide the logic of picking the course for the specified period from your actual add method, in case you can and want to change that (to an array/Collection) in the future. This will drastically simplify your code inside the add method. \$\endgroup\$ – h.j.k. Oct 1 '14 at 2:41
  • 4
    \$\begingroup\$ @RexPRGMER If someone in charge wants something that strange, you should follow. But don't interpret it too literally, they want to be able to access course1, etc., so provide such an interface, but ever think about coding everything 8 times. Pack all courses into an array/list ad work with it. Everywhere. \$\endgroup\$ – maaartinus Oct 1 '14 at 3:29
11
\$\begingroup\$

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.");
     }
}
\$\endgroup\$

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.