Reorder a list of courses, given their prerequisites

I recently graduated from college and applied for a position with a company. I was sent a coding exercise as part of the process. After completing the exercise and submitting it I was told that the code provided the correct solution but was too hard to follow, the logic should be in the Course class, and "..due to refactoring requirements." (I'm not sure what that means).

I know that I have a lot to learn and that I am still a novice programmer, but I was wondering if I could get some constructive suggestions from more experienced developers as to what needs to be changed in my code?

The basic requirements are: Takes a String[] that contains the courses you must take and returns a String[] of courses in the order the courses should be taken so that all prerequisites are met. Validation is to be done on each course description and course names. Return an empty string[] if an error occurs.

Examples of valid input Strings are: "CSE111: CSE110 MATH101" "CSE110:"

```
public class Course {
private String courseName;
private List prerequisites;

public Course() {
this.prerequisites = new ArrayList();
}

public void setCourseName(String courseName) {
this.courseName = courseName;
}

public String getCourseName() {
return courseName;
}

public List getPrerequisites() {
return prerequisites;
}

public void addPrerequisite(String prerequisite) {
prerequisites.add(prerequisite);
}

public int getCourseNumber() throws InvalidCourseNameException {
int courseNumber = -1;
try {
if (courseName != null && courseName.length() > 3) {
courseNumber = Integer.parseInt(courseName.substring(courseName.length() - 3));
}
} catch (NumberFormatException e) {
throw new InvalidCourseNameException(e.getMessage());
}
return courseNumber;
}
}

public class CourseScheduler {
private Map availableCourses;

A class should know & do everything about itself

• IsValidCourseName and isValidCourseDescription should be in the

Course class

Design should reflect your domain

-
What are we talking about here? A University, yes? Use that to frame
your design. What things are in there and what do we do with them?
What attributes to these things have?

-
I think there should be a Schedule class. This schedule may be
ordered, i.e. "scheduled" or it may be unordered, i.e. "just a list of
courses I've signed up for."

-
Maybe a Schedule has a boolean to indicated that it's been
scheduled, or maybe theres a separate class CourseLoad to
encapsulate the idea that this is a list of courses not yet
scheduled.

-
Maybe a CourseCatalog should encapsulate all the "available
courses" stuff.

-
Then client code is necessarily written & reads in terms of your
business model. e.g. compare: public String[]
scheduleCourses(String[] param0) and public Schedule
scheduleCourses(CourseLoad newCourseLoad). It becomes virtually self
documenting.

-
You should get 10 lashes for every parameter name like param0

-
havePrerequisitesBeenTaken() is totally baffling. Where the hell
did courseDescription come from? It's not in Course. The actual
code suggests that if a course has a prerequisite then, by
definition, it has not been taken. Yet your comments say otherwise.
That makes no sense. And I had to study the code too much to figure
that out.

I like the CourseScheduler as a separate class

-
Separating out complex algorithms is a good way to contain complexity
and keep other classes cleaner and clearer. This separation enhances
maintenance.

-
Single Responsibility Principle (SRP) says a class should do only
one thing. In this case "schedule courses." It should not be
building the available courses prerequisite map.

Design & coding principles are fractal

-
A fractal is a self-similar pattern, and likewise good design
principles should be applied at all levels of your code. Abstraction
and encapsulation apples at module, class, method, code block levels.

-
I.E. make classes, methods, code bits as needed to express things in
business and process terms as much as practical. "Push details down".
Otherwise you tend to obscure what's going on.

-
buildSchedule() is just one such method that is cluttered and it's
function is not readily apparent without some deliberate diving into
the details. Yes, at some point the code must do what it does, but at
the conceptual level of "how to build a schedule" I want to see the
conceptual steps expressed.

-
The CourseScheduler class is cluttered because it's doing more than
just producing a schedule. Specifically it seems to be the course
catalog as well.

Refactoring

Refactoring is a term with a technical meaning. Refactoring is the act of changing code without changing it's behavior (i.e. without breaking it!). There is an excellent book on the subject that should be on every programmer's bookshelf - hear me now and believe me later.

Good OO design significantly enhances your ability to refactor. So what, you say? Invariably code must be changed, either to fix bugs or add functionality. So the act of refactoring really starts with a software design that is flexible and extensible.

Refactoring is not a measure of design quality. It is not what you do only after you've delivered your final product. It is what you do from the very beginning of writing your code, staring with a blank sheet of paper (metaphorically speaking, of course). Continuous Refactoring means write what you need now. As you add stuff, refactor as needed to (a) not break what you have (b) apply and maintain good design and coding principles when adding code and (c) ultimately enhance future changes.

-
buildSchedule() should have the catalog & student's course list
passed into it. Now buildSchedule can deal with any catalog and any
course list. If the catalog mapping algorithm changes,
buildSchedule() does not change.

-
When you have complex or obscure logic consider refactoring. Compare:
if (prerequisites == null || prerequisites.size() == 0) vice
if(course.prerequisitesAreMet()). Note that (a) As changed I can
tell what's going and (b) the original code is not in the Course
class, yet it has to know how to figure out prerequisites for the
stupid course.

Good luck!

StackExchange Code Review Q#10858, answer score: 15