patternjavaMajor
Adding a course to one of 8 periods
Viewed 0 times
onecourseperiodsadding
Problem
I have this rather ghastly
```
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.");
}
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.