Yesterday I had the chance to write some code to determine if I needed to schedule an action for later processing, or do the action immediately. I am working on a system that has a fairly well laid out component architecture, with a fairly good Business model. However, it is almost all “Controllers and Data Structures” at this point. Given my previous experiences with how people would write this type of code, it would invariably end up like this:
-
RootModelObject root = DAOFactory.getRootModelDAO().getRootModel(rootModelID);
-
-
// many lines of code later …
-
-
if ( root.getExtraData() != null &&
-
root.getExtraData().getSchedule() != null ) {
-
Schedule s = root.getExtraData().getSchedule();
-
s.getCronExpression() != null ) {
-
root.setStatus(SCHEDULED);
-
DAOFactory.getRootModelDAO().save(root);
-
// schedule for later code
-
}
-
} else {
-
// deliver immediately code
-
}
-
// many lines of code later …
-
}
There are a number of issues in the above code:
- The interface to this method is abhorrent. I will discuss this in another post.
- The component knows WAY too much about the specifics for processing.
- Hand in hand with the above, it knows way too much about the Business Model.
This is a classic case of “Asking, not Telling”, the antithesis to the awesome article Tell, Don’t Ask, by Dave Thomas and Andy Hunt. GO NOW and read it if you haven’t.
Here is the code I ‘ve written so far:
-
public void someComponentMethod(RootModelObject root) {
-
// small amount of other code
-
-
if ( root.shouldBeScheduled() ) {
-
root.setStatus(SCHEDULED);
-
DAOFactory.getRootModelDAO().save(root);
-
scheduleComponent.schedule(root);
-
}
-
else {
-
root.setStatus(PROCESSING);
-
DAOFactory.getRootModelDAO().save(root);
-
nextComponent.process(root);
-
}
-
}
There are few things here that are better: First, I pass in the object I want to work on… this is SO much better than having to go get the Object from some back-end store by ID. I will definitely be talking about this more in the near future. Second, I broke out the beautiful comments from the code above into separate methods, on the components that they represent. This can only make things easier in that code, which most assuredly would have otherwise been in the if block as the comments suggest. And finally the piece that got me started on this post, I moved all of the logic revolving around the Schedule behind the walls of the RootModelObject. But, I did not stop there. Since from the perspective of the RootModelObject, it is not it’s responsibility to determine if it should be scheduled, I created a shouldBeScheduled method on the ExtraData object, which then called a shouldBeScheduled() method on the Schedule object:
-
class RootModelObject {
-
…
-
public boolean shouldBeScheduled() {
-
return getExtraData().shouldBeScheduled();
-
}
-
}
-
-
class ExtraData {
-
…
-
public boolean shouldBeScheduled() {
-
return getSchedule().shouldBeScheduled();
-
}
-
}
-
-
class Schedule {
-
…
-
public boolean shouldBeScheduled() {
-
getCronExpression() != null );
-
}
-
}
This “string of pearls” makes it much easier to keep the code where it needs to be, while providing convenience to the component developer.
So, the code is better, but part of me says that the code really should look like this:
-
public void someHigherLevelComponentMethod(RootModelObject root) {
-
…
-
root.schedule();
-
}
Since technically, when you say “process immediately” you are effectively saying “schedule for processing now”. What do you think?
Jeff | 02-Dec-06 at 3:49 am | Permalink
The last snippet negates the need for the someHigherLevelCompoinentMethod as root.schedule() could be executed directly from the calling method. How about:
if (root.isDue()) {
root.process(NOW);
} else {
root.process(LATER);
}
Jeff | 02-Dec-06 at 3:53 am | Permalink
or better yet:
if (root.isDue()) {
root.processNow();
} else {
root.processLater();
}