HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMajor

Adding a course to one of 8 periods

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
onecourseperiodsadding

Problem

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

Solution

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()) {

Code Snippets

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.");
}
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.");
|---}
|
}
if (course.isFree() == true) {
if (course.isFree()) {

Context

StackExchange Code Review Q#64327, answer score: 34

Revisions (0)

No revisions yet.