Visitor vs. InstanceOf
Over the last few days there has been an interesting thread on comp.object regarding whether it is better to use Visitor or if/else chains of instanceof statements. I'd like to explore that argument here, by pursuing a simple example.
Imagine that I have a system that tracks courses being taught at Object Mentor. Imagine the system knows the kinds of courses we teach, the dates that they are taught, and how many computers must be shipped to the customer site.
Here is a simple junit test that describes how I would like the system to behave:
public class CourseResourceTrackerTest extends TestCase {
private CourseResourceTracker crt;
private GregorianCalendar monday;
private GregorianCalendar friday;
private GregorianCalendar nextMonday;
protected void setUp() throws Exception {
crt = new CourseResourceTracker();
monday = new GregorianCalendar(2005, 2, 28);
friday = new GregorianCalendar(2005, 3, 1);
nextMonday = new GregorianCalendar(2005, 3,4);
}
public void testNullCase() throws Exception {
assertEquals(0, crt.getComputersInUse(monday));
}
public void testOneAOODCourse() throws Exception {
crt.addCourse(new AOODCourse(new GregorianCalendar(3, 31, 2005), 10));
assertEquals(0, crt.getComputersInUse(monday));
}
public void testOneJavaCourse() throws Exception {
crt.addCourse(new JavaCourse(monday, 10));
assertEquals(5, crt.getComputersInUse(monday));
assertEquals(5, crt.getComputersInUse(friday));
assertEquals(0, crt.getComputersInUse(nextMonday));
}
}
Here are is the simple program that passes these tests:
public class CourseResourceTracker {
LinkedList courses = new LinkedList();
public int getComputersInUse(GregorianCalendar date) {
int resources = 0;
for (Iterator i = courses.iterator(); i.hasNext();) {
Course course = (Course) i.next();
resources += course.getComputersInUse(date);
}
return resources;
}
public void addCourse(Course course) {
courses.add(course);
}
}
public abstract class Course {
protected GregorianCalendar startDate;
protected int students;
public Course(GregorianCalendar startDate, int students) {
this.startDate = (GregorianCalendar) startDate.clone();
this.students = students;
}
abstract public int getComputersInUse(GregorianCalendar date);
}
public class AOODCourse extends Course {
public AOODCourse(GregorianCalendar startDate, int students) {
super(startDate, students);
}
public int getComputersInUse(GregorianCalendar date) {
return 0;
}
}
public class JavaCourse extends Course {
private GregorianCalendar endDate;
public JavaCourse(GregorianCalendar startDate, int students) {
super(startDate, students);
endDate = (GregorianCalendar) startDate.clone();
endDate.add(Calendar.DAY_OF_WEEK, 4);
}
public int getComputersInUse(GregorianCalendar date) {
int resources = 0;
if (!date.before(startDate) && !date.after(endDate)) {
resources = Math.round(students/2);
}
return resources;
}
}
Adding a report.
Now let's suppose that our customers would like a report of all the courses, and the computers used in those courses. Let's say that the format of this report is:
03/28/2005 Java 5 Computers
04/04/2005 AOOD 0 Computers.
We could easily add this functionality to the system by adding the method generateComputerReport() to the Resource Tracker and Course hierarchy as follows:
public String generateComputerReport() {
StringBuffer report = new StringBuffer();
for (Iterator i = courses.iterator(); i.hasNext();) {
Course course = (Course) i.next();
report.append(course.generateComputerReportLine());
}
return report.toString();
}
public abstract class Course {
...
protected SimpleDateFormat dateFormat;
public Course(GregorianCalendar startDate, int students) {
...
dateFormat = new SimpleDateFormat("MM/dd/yyyy");
}
public abstract int getComputersInUse(GregorianCalendar date);
public String generateComputerReportLine() {
StringBuffer line = new StringBuffer();
line.append(dateFormat.format(startDate.getTime())).append(" ")
.append(courseName()).append(" ")
.append(computersInUse()).append(" Computers\n");
return line.toString();
}
protected abstract String courseName();
protected abstract String computersInUse();
}
protected String courseName() {
return "AOOD";
}
protected String computersInUse() {
return "0";
}
protected String courseName() {
return "Java";
}
protected String computersInUse() {
return String.valueOf(Math.round(students/2));
}
Ugly
But this is ugly. Why should the Course hierarchy have to know anything about date formatting? Why should the course class know about the format of the report? Why should the CourseResourceTracker know anything about reports at all? What if the business wants new kinds of reports? Do we add another method to the CourseResourceTracker and the whole Course hierarchy? That would be bad. Every new report would cause us to add new methods. Every time the format of one of the reports changes, we have to change the Course hierarchy. In the end, this violates the Single Repsponsibility Principle (SRP).We might be able to solve this by creating a new class named CourseComputerReport as follows:
public class CourseComputerReport {
protected SimpleDateFormat dateFormat;
public CourseComputerReport() {
dateFormat = new SimpleDateFormat("MM/dd/yyyy");
}
public String generateComputerReport(List courses) {
StringBuffer report = new StringBuffer();
for (Iterator i = courses.iterator(); i.hasNext();) {
Course course = (Course) i.next();
report.append(dateFormat.format(course.GetStartDate().getTime())).append(" ");
if (course instanceof AOODCourse) {
report.append("AOOD 0 Computers\n");
} else if (course instanceof JavaCourse) {
report.append("Java ");
JavaCourse jc = (JavaCourse)course;
report.append(String.valueOf(jc.getTotalComputers())).append(" Computers\n");
}
}
return report.toString();
}
}
Cleaner...
This is better because it keeps all the report junk out of the Course and CourseResourceTracker hierarchies. Another way to say this is that we are creating a separation between the business rules and the UI. Changes to the report format, or the kinds of report don't affect the business rules at all. We could put the business rules and the reports into completely different jar files and deploy them seperately.However, there are some things I don't like about this. For one thing, the chain of if/else statement will grow without bound as more and more Course derivatives are added to the system. It takes time to execute the instanceof statement, and on average we will have to execute about half of them for each course. This means that the runtime complexity of the CourseComputerReport is O(n), where n is the number of derivatives. Another thing I don't like about this is that each new report will have to copy the loop and the if/else chain of instanceof statements.
I can fix this final complaint by using the Template Method pattern as follows:
public abstract class CourseReport {
protected SimpleDateFormat dateFormat;
public CourseReport() {
dateFormat = new SimpleDateFormat("MM/dd/yyyy");
}
public String generateComputerReport(List courses) {
StringBuffer report = new StringBuffer();
for (Iterator i = courses.iterator(); i.hasNext();) {
Course course = (Course) i.next();
report.append(dateFormat.format(course.GetStartDate().getTime())).append(" ");
if (course instanceof AOODCourse) {
appendAOODLine((AOODCourse)course, report);
} else if (course instanceof JavaCourse) {
appendJavaLine((JavaCourse) course, report);
}
}
return report.toString();
}
protected abstract void appendJavaLine(JavaCourse course, StringBuffer report);
protected abstract void appendAOODLine(AOODCourse course, StringBuffer report);
}
public class CourseComputerReport extends CourseReport {
protected void appendJavaLine(JavaCourse course, StringBuffer report) {
report.append("Java ");
report.append(String.valueOf(course.getTotalComputers())).append(" Computers\n");
}
protected void appendAOODLine(AOODCourse course, StringBuffer report) {
report.append("AOOD 0 Computers\n");
}
}
Even cleaner...
Clearly this is better. We can now create derivatives of CourseReport for any new kind of line by line report we want. Of course if we add a new derivative of Course, we'll have to add the corresponding appendXXXLine method to the CourseReport[?] class, and all it's derivatives; but that doesn't seem inappropriate. However, I still don't like the O(n) behavior. We spend a lot of time executing instanceof statements. We can fix that by using the Visitor pattern.
public abstract class Course {
...
public abstract void accept(CourseVisitor v);
...
}
}
public class AOODCourse extends Course {
...
public void accept(CourseVisitor v) {
v.visit(this);
}
}
public class JavaCourse extends Course {
...
public void accept(CourseVisitor v) {
v.visit(this);
}
}
public interface CourseVisitor {
void visit(AOODCourse course);
void visit(JavaCourse course);
}
public class ComputerReportCourseVisitor implements CourseVisitor {
private StringBuffer report;
public ComputerReportCourseVisitor(StringBuffer report) {
this.report = report;
}
public void visit(AOODCourse course) {
report.append("AOOD 0 Computers\n");
}
public void visit(JavaCourse course) {
report.append("Java ");
report.append(String.valueOf(course.getTotalComputers())).append(" Computers\n");
}
}
public abstract class CourseReport {
...
public String generateComputerReport(List courses) {
StringBuffer report = new StringBuffer();
ComputerReportCourseVisitor v = makeReportVisitor(report);
for (Iterator i = courses.iterator(); i.hasNext();) {
Course course = (Course) i.next();
report.append(dateFormat.format(course.GetStartDate().getTime())).append(" ");
course.accept(v);
}
return report.toString();
}
protected abstract ComputerReportCourseVisitor makeReportVisitor(StringBuffer report);
}
public class CourseComputerReport extends CourseReport {
protected ComputerReportCourseVisitor makeReportVisitor(StringBuffer report) {
return new ComputerReportCourseVisitor(report);
}
}
This is better?
This is starting to look like a lot of code to solve a simple problem. However, let's put it in perspective. I'm only showing one type of report, and two types of courses. Imagine that the circumstances were a bit different. Imagine that there were dozens of different types of courses and reports. In that case the amount of code invested in the visitor would be small in comparison.This solves the O(n) problem; the runtime complexity is now O(1). But there are other problems. The most severe is that Course, CourseVisitor[?], and the derivatives of course are now involved in a cycle of dependencies: Cycle->CycleVisitor[?]->{AOODCourse,JavaCourse[?]}->Course. This knot of dependencies means that every time we want to add a new course, we'll have to rebuild and redeploy all the existing derivatives and visitors. We can resolve this by reinserting instanceof back into the structure in a slightly different way. This is called Acyclic Visitor.
public interface CourseVisitor {
}
public interface JavaCourseVisitor {
void visit(JavaCourse course);
}
public interface AOODCourseVisitor {
void visit(AOODCourse course);
}
public class JavaCourse extends Course {
...
public void accept(CourseVisitor v) {
if (v instanceof JavaCourseVisitor) {
JavaCourseVisitor jv = (JavaCourseVisitor) v;
jv.visit(this);
}
}
}
public class AOODCourse extends Course {
...
public void accept(CourseVisitor v) {
if (v instanceof AOODCourseVisitor) {
AOODCourseVisitor av = (AOODCourseVisitor) v;
av.visit(this);
}
}
}
public class ComputerReportCourseVisitor implements CourseVisitor, AOODCourseVisitor, JavaCourseVisitor {
...
}
The cycle is broken.
This structure may look even more complex than the visitor, but it breaks the dependency cycle and keeps the runtime complexity at O(1). Now if we add a new derivative of Course, we do not have to recompile and redeploy all the other Course derivatives. Moreover, we only need to redeploy those visitors that actually are affected. You might think that all the visitors are affected, but consider that some of them might not implement all the visitor interfaces.Conclusion
I am not trying to make the case that Acyclic Visitor should always be used. Indeed, each of the forms we have discussed could be appropriate in various situations. I use the following decision table:| Context Hierarchy Changes | Visitor Hierarchy Changes | Efficiency is important | |
| if/else of instanceof | no | no | no |
| Visitor | no | yes | yes |
| Acyclic Visitor | yes | yes | yes |
And, of course, none of these solutions are necessary if the original function (e.g. the report) actually belongs in the context (Course) hierarchy.
!commentForm
How about:
public String generateComputerReport(List courses) {
StringBuffer[?] report = new StringBuffer[?]();
for (Iterator i = courses.iterator(); i.hasNext();) {
Course course = (Course) i.next();
report.append(dataFormat.format(course.GetStartData[?]().getTime())).append(" ");
report.append(course.getName()).append(" ");
report.append(String.valueOf(course.getTotalComputers())).append(" Computers\n");
}
return report.toString();
}
Keeps knowledge about the name and how many computers are involved in the particular course (not a seperate class that knows all this like the visitor).
Your thoughts?
public String generateComputerReport(List courses) {
StringBuffer[?] report = new StringBuffer[?]();
for (Iterator i = courses.iterator(); i.hasNext();) {
Course course = (Course) i.next();
report.append(dataFormat.format(course.GetStartData[?]().getTime())).append(" ");
report.append(course.getName()).append(" ");
report.append(String.valueOf(course.getTotalComputers())).append(" Computers\n");
}
return report.toString();
}
Keeps knowledge about the name and how many computers are involved in the particular course (not a seperate class that knows all this like the visitor).
Your thoughts?
- 5/3/2005 Uncle Bob: This works well so long as the format of each line is independent of the course type. As Ravi points out below, if each course type had a different structure for the line on the report, or if some courses had no line on the report, then this approach stops working as well.
Mike's alternative is interesting.
The key here is that the information requested of both sub-classes is very similar.
It would be much more interesting if we needed to print the text books used, but only for the JavaCourse[?].
Then Mike's method can not be used while the Visitor pattern would work correctly.
The Visitor pattern requires the creation of a new Visitor hierachy of classes and changes to the Course class and its sub-classes, making the classes of the Course hierarchy aware of the Visitor hierarchy. Another interesting point is that the Visitor hierarchy is dependent on the Course hierarchy. It is actually a copy of the Course hierarchy.
In good software design, the Course class should never be aware of any other classs that it does not need to know about. Given that the Visitor hierarchy is the same as the Course hierarchy, and that the Course classes must be aware of the Visitor classes, it is no surprise that there is a cyclic dependency. The fact that the Visitor pattern creates the cyclic depedency suggests that it is not a good pattern to use.
In the Acyclic Visitor Pattern, we see the use of "instance of". Instead of having the if .. else (or, what is effectively the same, "instance of") constructs in one place (as in the very first example that was deemed to be "UGLY"), the "instance of" has been moved to the new classes, one per class.
Is that really an improvement over the original "UGLY" design? I am not so sure.
My personal preference is to use reflection to achieve this , not the Visitor pattern. That way, you do not have any "instance of" or "if .. else" code. You also never have to modify the Course hierarchy of classes, nor do you have to create any of the Visitor hierarchy of classes.
If a language does not have reflection, only then should the abomination known as the Visitor pattern be used.
The key here is that the information requested of both sub-classes is very similar.
It would be much more interesting if we needed to print the text books used, but only for the JavaCourse[?].
Then Mike's method can not be used while the Visitor pattern would work correctly.
The Visitor pattern requires the creation of a new Visitor hierachy of classes and changes to the Course class and its sub-classes, making the classes of the Course hierarchy aware of the Visitor hierarchy. Another interesting point is that the Visitor hierarchy is dependent on the Course hierarchy. It is actually a copy of the Course hierarchy.
In good software design, the Course class should never be aware of any other classs that it does not need to know about. Given that the Visitor hierarchy is the same as the Course hierarchy, and that the Course classes must be aware of the Visitor classes, it is no surprise that there is a cyclic dependency. The fact that the Visitor pattern creates the cyclic depedency suggests that it is not a good pattern to use.
In the Acyclic Visitor Pattern, we see the use of "instance of". Instead of having the if .. else (or, what is effectively the same, "instance of") constructs in one place (as in the very first example that was deemed to be "UGLY"), the "instance of" has been moved to the new classes, one per class.
Is that really an improvement over the original "UGLY" design? I am not so sure.
My personal preference is to use reflection to achieve this , not the Visitor pattern. That way, you do not have any "instance of" or "if .. else" code. You also never have to modify the Course hierarchy of classes, nor do you have to create any of the Visitor hierarchy of classes.
If a language does not have reflection, only then should the abomination known as the Visitor pattern be used.
- 5/3/2005, Uncle Bob: I think instanceof is a simple form of reflection. It is type information available at runtime. So Acyclic Visitor does depend on reflection. I'd like to see how you would use reflection in Java to solve this problem though. Can you post some code?
Not exactly the best way to do this, and personally I would use the visitor myself, but here is one way to "solve" it using reflection. I should note that I did not compile this (no JDK here), but it looks to be correct.
You could, of course, extend this for other courses. One of the nice things about this method is that the Course class doesn't even have to know that it's going to be used to generate a report at all. But there are performance considerations to take into account when using reflection like this.
public class ReflectionReporter {
public void generateReport(Cource course) {
Class courseClass = course.getClass();
Method reportMethod = this.getClass().getMethod("generateReport", courseClass);
reportMethod.invoke(this, course);
}
public void generateReport(JavaCourse course) {
...
}
public void generateReport(AOODCourse course) {
...
}
}
You could, of course, extend this for other courses. One of the nice things about this method is that the Course class doesn't even have to know that it's going to be used to generate a report at all. But there are performance considerations to take into account when using reflection like this.
Yes, Sean's implementation was my original idea too.
To speed up the process, I would cache the method names in a HashMap[?] and then the only extra overhead would be in the Method.invoke() method.
I do not think that the performance hit would be prohibitive in real time, not relative time comparisons. The major hit occurs when you determine the method name. If you cache itm then you will do the method lookup only once for each subclass, every subsequent call would use the HashMap[?] lookup to get the Method object.
(I'll be supplying the detailed code using Java 5 in the next comment.)
But the main point of this is that it may be possible to encapsulate the dynamic call in a different class, and use it whenever the Visitor pattern is needed!
That is, if we could generalize this, then we get a huge productivity gain!
Any time we think we need Visitor pattern, we would just use something similar to what Sean suggested, as follows: (using Java 5 syntax)
public class ReflectionReporter[?] {
public void generateReport(List<Course> courses) {
for (Course c : courses)
{
// The following three lines of code would be replaced with something like
// Utils.generateReport(this, "generateReport", course);
Class courseClass = course.getClass();
Method reportMethod = this.getClass().getMethod("generateReport", courseClass);
// The above line would be optimized by caching results. See next comment.
reportMethod.invoke(this, course);
}
}
public void generateReport(JavaCourse[?] course) {
...
}
public void generateReport(AOODCourse course) {
...
}
}
To speed up the process, I would cache the method names in a HashMap[?] and then the only extra overhead would be in the Method.invoke() method.
I do not think that the performance hit would be prohibitive in real time, not relative time comparisons. The major hit occurs when you determine the method name. If you cache itm then you will do the method lookup only once for each subclass, every subsequent call would use the HashMap[?] lookup to get the Method object.
(I'll be supplying the detailed code using Java 5 in the next comment.)
But the main point of this is that it may be possible to encapsulate the dynamic call in a different class, and use it whenever the Visitor pattern is needed!
That is, if we could generalize this, then we get a huge productivity gain!
Any time we think we need Visitor pattern, we would just use something similar to what Sean suggested, as follows: (using Java 5 syntax)
public class ReflectionReporter[?] {
public void generateReport(List<Course> courses) {
for (Course c : courses)
{
// The following three lines of code would be replaced with something like
// Utils.generateReport(this, "generateReport", course);
Class courseClass = course.getClass();
Method reportMethod = this.getClass().getMethod("generateReport", courseClass);
// The above line would be optimized by caching results. See next comment.
reportMethod.invoke(this, course);
}
}
public void generateReport(JavaCourse[?] course) {
...
}
public void generateReport(AOODCourse course) {
...
}
}
Here is the complete code using reflection. (Hope I get the formatting correct.)
This runs under Java 5 only.
!* CourseComputerReport.java
This runs under Java 5 only.
!* CourseComputerReport.java
public class CourseComputerReport
{
public CourseComputerReport(){}
! private void printReportLine(AOODCourse course)
{
System.out.println("\n AOOD has " + course.getStudents() + " Students ");
}
! private void printReportLine(JavaCourse course)
{
System.out.println("\n Java has " + course.getStudents() + " Students \n " +
" TextBook for Java Course is: " + course.getTextBookName());
}
public void generateReport(List<Course> courses)
{
Utils.dynamicDispatch(this, "printReportLine", courses);
}
public static void main(String[] args)
{
List<Course> courses = new ArrayList<Course> ();
Course c1 = new AOODCourse(5);
JavaCourse c2 = new JavaCourse(10);
c2.setTextBookName("Thinking in Java");
courses.add(c1);
courses.add(c2);
CourseComputerReport rep = new CourseComputerReport();
rep.generateReport(courses);
}
}
Here's the Utils package that does the dynamic invocation and the caching of Methods.
public class Utils
{
private static Map<Class, Method> methods = new HashMap<Class, Method> ();
public static void dynamicDispatch(Object obj, String methodName, List<Course> courses)
{
for (Course course : courses)
{
Class klass = course.getClass();
if (! methods.containsKey(klass) )
{
try{
Method m = obj.getClass().getDeclaredMethod(methodName, klass);
m.setAccessible(true);
methods.put(klass, m);
}
catch(NoSuchMethodException nsmEx) {}//ignore
catch(SecurityException secEx) { } //ignore -- for now
}
Method m = methods.get(klass);
try{m.invoke(obj, course);}
catch(IllegalAccessException illAccessEx)
{System.out.println("Illegal Access Exception ");}
catch(IllegalArgumentException illArgEx)
{System.out.println("Illegal Argument Exception");}
catch(InvocationTargetException invTgtEx)
{System.out.println("Invocation Target Exception");}
}
}
}
! Here are the Course, AOOD and JavaCourse[?] classes.
public abstract class Course
{
protected int students;
public Course(int students) {this.students = students;}
abstract public int getStudents();
}
public class AOODCourse extends Course
{
public AOODCourse(int students) {super(students);}
public int getStudents(){return 0;}
}
public class JavaCourse extends Course
{
private String textBookName;
public JavaCourse(int students){super(students);}
public int getStudents(){return Math.round(students/2);}
public void setTextBookName(String name) {this.textBookName = name;}
public String getTextBookName() {return this.textBookName;}
}
Here are the pros and cons of the two methods:
1. Speed: Definitely the Visitor pattern is faster at runtime.
2. Ease of development (Productivity): Once we define the Utils class in a generic way to accept a list of any type [Java 5 syntax is List<? extends Object>] and we allow for having many parameters for the "printReportLine" type methods (requires very minor changes to the Utils class), the reflection method is as simple as one line of code followed by testing. The Visitor pattern requires you to create a few classes and to modify the original Course hierarchy classes. One line of code will always be faster than writing a few classes, I feel.
3. The level of abstraction is increased. That is, we are programming at a level closer to the actual problem rather than getting lost in the details of designing a few new classes.
4. Reusability: The Visitor classes have to be created every time such a pattern occurs. Using reflection, you never have to write these extra classes. You can re-use the Utils method any time the urge to use Visitor pattern arises. No coding needed at all except one line!
Unless it is proven that the reflection pattern will not give acceptable performance, I will not use the Visitor pattern. Even if it is shown that the reflection method is slow, I would rather consider the use of if..else blocks before deciding what to do.
A much better solution would be available to us if Java had methods as first class objects and the ability to invoke a method by giving its name and class. Then, if we had two methods: "printAOODCourseLine" and "printJavaCourseLine", we could do something like:
for (Course c: courses)
{ System.execMethod(this.getClass(), "print" + c.getClass().getSimpleName() + "Line", this, c);}
where the first parameter is the class the method belongs to;
the second is the name of the method built up at run time;
and the third and fourth are obvious.
The Utils.dynamicDispatch method does essentially that but we have to write the underlying code ourselves.
If methods were first class objects, the code would be much simpler. Also, Java programmers would be using somewhat different ways to solve problems.
Most importantly, though, the JVM could be optimized to execute these methods.
1. Speed: Definitely the Visitor pattern is faster at runtime.
2. Ease of development (Productivity): Once we define the Utils class in a generic way to accept a list of any type [Java 5 syntax is List<? extends Object>] and we allow for having many parameters for the "printReportLine" type methods (requires very minor changes to the Utils class), the reflection method is as simple as one line of code followed by testing. The Visitor pattern requires you to create a few classes and to modify the original Course hierarchy classes. One line of code will always be faster than writing a few classes, I feel.
3. The level of abstraction is increased. That is, we are programming at a level closer to the actual problem rather than getting lost in the details of designing a few new classes.
4. Reusability: The Visitor classes have to be created every time such a pattern occurs. Using reflection, you never have to write these extra classes. You can re-use the Utils method any time the urge to use Visitor pattern arises. No coding needed at all except one line!
Unless it is proven that the reflection pattern will not give acceptable performance, I will not use the Visitor pattern. Even if it is shown that the reflection method is slow, I would rather consider the use of if..else blocks before deciding what to do.
A much better solution would be available to us if Java had methods as first class objects and the ability to invoke a method by giving its name and class. Then, if we had two methods: "printAOODCourseLine" and "printJavaCourseLine", we could do something like:
for (Course c: courses)
{ System.execMethod(this.getClass(), "print" + c.getClass().getSimpleName() + "Line", this, c);}
where the first parameter is the class the method belongs to;
the second is the name of the method built up at run time;
and the third and fourth are obvious.
The Utils.dynamicDispatch method does essentially that but we have to write the underlying code ourselves.
If methods were first class objects, the code would be much simpler. Also, Java programmers would be using somewhat different ways to solve problems.
Most importantly, though, the JVM could be optimized to execute these methods.
Why do you need all the overhead of introspection when it isn't needed. Isn't the simplest thing to define an interface between ComputerReportGenerator[?] and Course that idetifies the needs the report generator has. So the set of methods needed are defined at the Course level and implemented as needed for each subclass. So if the report generator needs to know the following:
1) Name of course
2) Is there a book
2a) Name of book is there is one
3) Number of students
Then why not write simple code that asks those questions of each Course and print out the result. What am I missing?
public String generateComputerReport(List courses) {
StringBuffer[?]? report = new StringBuffer[?]?();
for (Iterator i = courses.iterator(); i.hasNext();) {
Course course = (Course) i.next();
! report.append(dataFormat.format(course.GetStartData[?]?().getTime())).append(" ");
report.append(course.getName()+ " has " + course.getStudents() + " Students \n ");
if (course.hasTextBook())
report.append("TextBook[?] for " + course.getName() + " course is: " + course.getTextBookName());
}
return report.toString();
}
YAGNI right?
1) Name of course
2) Is there a book
2a) Name of book is there is one
3) Number of students
Then why not write simple code that asks those questions of each Course and print out the result. What am I missing?
public String generateComputerReport(List courses) {
StringBuffer[?]? report = new StringBuffer[?]?();
for (Iterator i = courses.iterator(); i.hasNext();) {
Course course = (Course) i.next();
! report.append(dataFormat.format(course.GetStartData[?]?().getTime())).append(" ");
report.append(course.getName()+ " has " + course.getStudents() + " Students \n ");
if (course.hasTextBook())
report.append("TextBook[?] for " + course.getName() + " course is: " + course.getTextBookName());
}
return report.toString();
}
YAGNI right?
I just realized that I have been arguing instead of realizing. Isn't the whole point that sometimes you have functionality that differs per sub class, but for some reason doesn't belong in each subclass. The ways to handle this are:
1) If thens on instanceof
2) Visitor pattern
3) Introspection
4) Use an interface to the super-class that allows the logic to be generic (my original comment)
The next question is when is each appropriate to you use (and inapproprate). I believe the example in the original post is best severed by 4 (I sound like my children "I'm right" ;). Of course others might disagree.
1) If thens on instanceof
2) Visitor pattern
3) Introspection
4) Use an interface to the super-class that allows the logic to be generic (my original comment)
The next question is when is each appropriate to you use (and inapproprate). I believe the example in the original post is best severed by 4 (I sound like my children "I'm right" ;). Of course others might disagree.
Mike, the line if course.hasTextBook() is a method of the JavaCourse[?] class, not the Course class. This will result in a compile time failure.
That is a limitation of Java.
If you want to make this work, you'll have to check if the course is an instance of JavaCourse[?], cast it to JavaCourse[?], and then invoke the method.
The YAGNI principle of XP is a difficult concept for most people to understand. If I see a pattern in my work where I see Visitor like classes appearing frequently, then I ask myself the question: To avoid duplication can I either encapsulate this functionality so that it becomes re-usable? Or can I solve the problem in a way that permits reuse? If we can do that, I gain in productivity.
YAGNI must not be applied in isolation to a small piece of code. One must understand the complete problem that we are trying to solve. Since the reflection method make the overall code simpler.
Look at the result. The reflection approach results in much simpler code, that can be easily extended and reused. It avoids unnecessary duplication. So where does YAGNI come into the picture? It does not!
The only meaningful question left is: Is the performance acceptable? If the answer is yes, there is no need to do anything more.
As for reflection being slow, agreed that it is. But as some well known person said (para-phrasing): Premature optimization is the root of all software evil.
The Visitor pattern is an exercise in premature optimization that leads to extra code which needs to be repeated for every single usage of the pattern.
That is a limitation of Java.
If you want to make this work, you'll have to check if the course is an instance of JavaCourse[?], cast it to JavaCourse[?], and then invoke the method.
The YAGNI principle of XP is a difficult concept for most people to understand. If I see a pattern in my work where I see Visitor like classes appearing frequently, then I ask myself the question: To avoid duplication can I either encapsulate this functionality so that it becomes re-usable? Or can I solve the problem in a way that permits reuse? If we can do that, I gain in productivity.
YAGNI must not be applied in isolation to a small piece of code. One must understand the complete problem that we are trying to solve. Since the reflection method make the overall code simpler.
Look at the result. The reflection approach results in much simpler code, that can be easily extended and reused. It avoids unnecessary duplication. So where does YAGNI come into the picture? It does not!
The only meaningful question left is: Is the performance acceptable? If the answer is yes, there is no need to do anything more.
As for reflection being slow, agreed that it is. But as some well known person said (para-phrasing): Premature optimization is the root of all software evil.
The Visitor pattern is an exercise in premature optimization that leads to extra code which needs to be repeated for every single usage of the pattern.
Ravi, you mention that reflection is the simpler solution, but I have an argument against that.
While yes, I do agree that the code is certainly smaller, and for those who understand reflection simpler, unfortunantly a great many people do not understand reflection. I've even met many senior developers who found reflection to be confusing. So I would say, just based on my experiences mind you, that the reflection approach is more complex.
With the visitor based approach, it's fairly simple to follow the logic, even if there typically will be more code than with reflection.
One interesting thing to note is that the reflection approach can be extended through inheritance.
While yes, I do agree that the code is certainly smaller, and for those who understand reflection simpler, unfortunantly a great many people do not understand reflection. I've even met many senior developers who found reflection to be confusing. So I would say, just based on my experiences mind you, that the reflection approach is more complex.
With the visitor based approach, it's fairly simple to follow the logic, even if there typically will be more code than with reflection.
One interesting thing to note is that the reflection approach can be extended through inheritance.
Sean, I believe that software is about managing complexity. The use of reflection is one way of doing it. The complexity is hidden in the reusable Utils class. Whereas when we use the Visitor pattern we see complexity all over the place. This complexity has to be repeated time and again. Not so using reflection. That, I believe, is the main advantage of the reflection approach.
Being a lazy programmer, I try to avoid duplication. Thus, reflection is by far the better option.
Being a good programmer, I hate it when basic software principles are violated. The Visitor pattern violates basic software design principles by forcing the addition of an extra method (unecessarily so, IMO) to the Course classes and tightly couples the Vistor classes and the Course classes.
One example where complexity has not been handled correctly is in Java's JDBC libraries. The methods throw SQLException almost without exception. A well designed library would have given the user the option to handle Exceptions in a more flexible way. As a result of the poorly planned library, client code is littered with try catch blocks. This makes the code totally unreadable. Well, the java.sql package is just one of many examples of poor design in Java, the language and the libraries.
Being a lazy programmer, I try to avoid duplication. Thus, reflection is by far the better option.
Being a good programmer, I hate it when basic software principles are violated. The Visitor pattern violates basic software design principles by forcing the addition of an extra method (unecessarily so, IMO) to the Course classes and tightly couples the Vistor classes and the Course classes.
One example where complexity has not been handled correctly is in Java's JDBC libraries. The methods throw SQLException almost without exception. A well designed library would have given the user the option to handle Exceptions in a more flexible way. As a result of the poorly planned library, client code is littered with try catch blocks. This makes the code totally unreadable. Well, the java.sql package is just one of many examples of poor design in Java, the language and the libraries.
"One example where complexity has not been handled correctly is in Java's JDBC libraries. The methods throw SQLException almost without exception. A well designed library would have given the user the option to handle Exceptions in a more flexible way. As a result of the poorly planned library, client code is littered with try catch blocks. This makes the code totally unreadable. Well, the java.sql package is just one of many examples of poor design in Java, the language and the libraries."
Either that or a lot of throws statements on the methods. I never did find checked exceptions to be a particularily useful feature of the language, more a hinderance. While it can be nice having the compiler tell you when you need to handle possible exceptions, I would rather that it warned instead of required. My reasoning for this is simple: If I have a method today that throws one exception, I might later end up refactoring it, and it might possibly throw a few new exceptions. Now I must either not make those exceptions checked (inconsistent interface then) or I might update the throws to include the new exceptions, but that means I have to alter all of the code that uses checked exceptions. My final option is to use a more general throws statement, but even that's a poor choice IMO.
I never claimed that reflection wasn't a better option. I personally have used reflection to solve other problems like this. However, it generally isn't my first choice. It especially depends on whom I'm working with. If the people aren't familiar with the power that reflection provides (new from a C/C++ background, or were never educated in it) then I'm even more likely to choose the visitor solution. Because *I* may not be the one maintaining the code, so whomever does had better know how it works.
Dependancies are a problem, this is of course where one should bring up AOP, but considering the lack of material and decent language support for it oh well. :D
Either that or a lot of throws statements on the methods. I never did find checked exceptions to be a particularily useful feature of the language, more a hinderance. While it can be nice having the compiler tell you when you need to handle possible exceptions, I would rather that it warned instead of required. My reasoning for this is simple: If I have a method today that throws one exception, I might later end up refactoring it, and it might possibly throw a few new exceptions. Now I must either not make those exceptions checked (inconsistent interface then) or I might update the throws to include the new exceptions, but that means I have to alter all of the code that uses checked exceptions. My final option is to use a more general throws statement, but even that's a poor choice IMO.
I never claimed that reflection wasn't a better option. I personally have used reflection to solve other problems like this. However, it generally isn't my first choice. It especially depends on whom I'm working with. If the people aren't familiar with the power that reflection provides (new from a C/C++ background, or were never educated in it) then I'm even more likely to choose the visitor solution. Because *I* may not be the one maintaining the code, so whomever does had better know how it works.
Dependancies are a problem, this is of course where one should bring up AOP, but considering the lack of material and decent language support for it oh well. :D
I guess I've been lucky to use Visitor on very stable class graphs that also happened to be fairly nasty to navigate. One was the DOM of an HTML parser where the author thoughtfully provided a visitor interface that was a joy to use for many purposes. I wrote the other on a hierarchy of components in a cluster that is built dynamically through a discovery API and visited "on the fly." The original vendor code mixed difficult navigation with the behavior in a completely non-reusable way. So for me, visitor has worked "out of the box" just fine.
I must say I think the reflection approach is neat. It implements visitor-like behavior, without the visitor-like infrastructure. I can see where that would be an advantage in many cases. I also think it could be improved by changing the template method approach in Ravi's code into a Strategy approach. That way the ReportGenerator class could do the reflection and then invoke a method on a passed-in report class. The report class has nothing but the appropriate printReportLine methods. Thus adding new reports becomes even simpler than deriving from Visitor, since they don't have to have a parent class.
I'll have to try this.
I'll have to try this.
Yes, reflection certainly does add a whole new perspective on a lot of good patterns. It would be interesting to conduct an exploration into the types of reflection patterns one might find useful in the field.
Well, "ReportGenerator[?] class could do the reflection". But the utility method that looks up the method name using the class name can be (a) reused; and (b) improved.
If the ReportGenerator[?] class uses this utility class then it can become very simple.
If the lookup occurs using a combination of ClassName[?] and method name and parameter types of the ethod to find the Method object, then the utility class (which I call DynUtils[?] in my work, but should probably rename it to IntrospectUtils[?] or something else) can be used across any application.
I have changed the Utils class presented earlier. Here is a brief summary of the methods:
1. getMethod(Class, methodName, Class[] parameterTypes)
Gets the method name. It has been sanitized so that it does not throw the Exceptions.
(See comments with method.)
2. invokeMethod(Method, Object, Object[] parameterValues)
Identical to Method.invoke(Object, Object[])
but has been sanitized to handle exceptions. (See comments with method.)
3. A small utility method methodKey(Class, methodName, Class[] parameterTypes)
Returns a String representation of its parameters.
(Could alternatively have over-ridden the hashcode to return a Long)
4. dynamicDispatch(Object, method, List<Object>, Object[] parameterValues)
This is the main method, but consists of only a few lines.
The Object is the ReportGenerator[?] object.
The method is the name of the method on which to do dynamic dispatch, printReportLine in this case.
List<Object> is the list of objects, for each of which we have to print a report line.
parameterValues is the array of parameters that may be needed for printing the report.
For example, typical reports may be bound by a date range, and then the parameterValues would be a start date and an end date.
This method first obtains the parameter types from the parameter values.
Then it gets the appropriate Method.
Then it invokes the method dynamically.
Here's the Java code for the Utils class:
!* Utils.java
If the ReportGenerator[?] class uses this utility class then it can become very simple.
If the lookup occurs using a combination of ClassName[?] and method name and parameter types of the ethod to find the Method object, then the utility class (which I call DynUtils[?] in my work, but should probably rename it to IntrospectUtils[?] or something else) can be used across any application.
I have changed the Utils class presented earlier. Here is a brief summary of the methods:
1. getMethod(Class, methodName, Class[] parameterTypes)
Gets the method name. It has been sanitized so that it does not throw the Exceptions.
(See comments with method.)
2. invokeMethod(Method, Object, Object[] parameterValues)
Identical to Method.invoke(Object, Object[])
but has been sanitized to handle exceptions. (See comments with method.)
3. A small utility method methodKey(Class, methodName, Class[] parameterTypes)
Returns a String representation of its parameters.
(Could alternatively have over-ridden the hashcode to return a Long)
4. dynamicDispatch(Object, method, List<Object>, Object[] parameterValues)
This is the main method, but consists of only a few lines.
The Object is the ReportGenerator[?] object.
The method is the name of the method on which to do dynamic dispatch, printReportLine in this case.
List<Object> is the list of objects, for each of which we have to print a report line.
parameterValues is the array of parameters that may be needed for printing the report.
For example, typical reports may be bound by a date range, and then the parameterValues would be a start date and an end date.
This method first obtains the parameter types from the parameter values.
Then it gets the appropriate Method.
Then it invokes the method dynamically.
Here's the Java code for the Utils class:
!* Utils.java
package visitor;
import java.lang.reflect.*;
import java.util.*;
public class Utils
{
private static Map<String, Method> methods = new HashMap<String, Method> ();
/**
* This method performs the dynamic dispatch and can be used to replace
* the Visitor pattern.
**/
public static void dynamicDispatch(Object obj, String methodName,
List<? extends Object> list, Object... paramValues)
{
Class[] parameterTypes = new Class[paramValues.length+1];
for (int i=1; i<=paramValues.length; i++)
parameterTypes[i] = paramValues.getClass();
Object[] pVals = new Object[paramValues.length+1];
for (int i=1; i<=paramValues.length; i++) pVals[i] = paramValues[i];
for (Object element : list)
{
// For dynamic dispatch the sub-class must be the first parameter.
parameterTypes[0] = element.getClass();
Method m = getMethod(obj.getClass(), methodName, parameterTypes);
// The first value must be the sub-class object.
pVals[0] = element;
invokeMethod(m, obj, pVals);
}
}
/**
* This method invokes the method using the object and the provided parameter values.
* The exceptions may be re-thrown as a sub-class of RuntimeException
* to keep the client code clean.
**/
public static Object invokeMethod(Method m, Object obj, Object... parameterValues)
{
try{return m.invoke(obj, parameterValues);}
catch(IllegalAccessException illAccessEx)
{
throw new RuntimeException("Illegal Access Exception ");
}
catch(IllegalArgumentException illArgEx)
{
throw new RuntimeException("Illegal Argument Exception");
}
catch(InvocationTargetException invTgtEx)
{
throw new RuntimeException("Invocation Target Exception");
}
}
/**
* This method gets the Method object for the given class and
* named method with the specified parameter types.
* The exceptions may be cast into a sub-class of RuntimeException
* to keep the client code clean.
**/
! public static Method getMethod(Class klass, String methodName, Class... parameterTypes)
{
String methodKey = methodKey(klass, methodName, parameterTypes);
if (! methods.containsKey(methodKey) )
{
try{
Method m = klass.getDeclaredMethod(methodName, parameterTypes);
m.setAccessible(true);
methods.put(methodKey, m);
}
catch(NoSuchMethodException nsmEx) {}//ignore
catch(SecurityException secEx) { } //ignore -- for now
}
return methods.get(methodKey);
}
/**
* A simple utility method to form a unique string that represents the
* Class, method and its parameters.
**/
private static String methodKey(Class klass, String methodName, Class... parameterTypes)
{
StringBuffer sb = new StringBuffer(klass.getName());
sb.append("^").append(methodName).append(":");
for (Class type : parameterTypes) sb.append(type.getName()).append("#");
return sb.toString();
}
}
There are several benefits of re-structuring the Utils class in the way shown above.
1. getMethod can be used to get the method for any class and method name with any parameters. Features specific to dynamic dispatch have been moved to the dynamicDispatch method. It does not throw any checked exceptions!
2. invokeMethod also can be easily used anywhere. It, too, does not throw any checked exceptions.
3. There is nothing in the dynamicDispatch method that is hard coded. Hence, it can be used with any type of ReportGenerator[?] without any problem.
Every time somebody needs a Visitor pattern, they can choose to use the dynamicDispatch method of this class. All the work reduces to one statement of code. Using Java5, it is truly a direct invocation of the method.
Many patterns in the GOF book become trivial when using reflection. The Factory method is one. The Strategy pattern also should become simple. I feel that the strategy pattern will become obvious and easy to use if Methods were first class objects.
1. getMethod can be used to get the method for any class and method name with any parameters. Features specific to dynamic dispatch have been moved to the dynamicDispatch method. It does not throw any checked exceptions!
2. invokeMethod also can be easily used anywhere. It, too, does not throw any checked exceptions.
3. There is nothing in the dynamicDispatch method that is hard coded. Hence, it can be used with any type of ReportGenerator[?] without any problem.
Every time somebody needs a Visitor pattern, they can choose to use the dynamicDispatch method of this class. All the work reduces to one statement of code. Using Java5, it is truly a direct invocation of the method.
Many patterns in the GOF book become trivial when using reflection. The Factory method is one. The Strategy pattern also should become simple. I feel that the strategy pattern will become obvious and easy to use if Methods were first class objects.
Hi Bob,
A lot of the visitor pattern about trying to implement a generic solution in a strongly typed language.
Closures in Ruby provide visitor behavior for basically free. I love your analysis of this in a Java
context. How simple might your final solution look if you did it in Ruby?
A lot of the visitor pattern about trying to implement a generic solution in a strongly typed language.
Closures in Ruby provide visitor behavior for basically free. I love your analysis of this in a Java
context. How simple might your final solution look if you did it in Ruby?
Thanks Robert.
(I hope you don't mind a bit of commentary here about 'bad practice phobia'...)
I had become tired of hearing the 'instanceof is bad in all conceivable situations' argument for years. It got to the point that if you mentioned you used instanceof in your code, the chorus in the newsgroups was that you had a poorly designed application and they didn't actually need to see your design to know this! :)
Its one of those issues where the fear of being thought of as being some newbie programmer, or worse, a 'hack', who still 'doesn't get' OO makes people create the most elaborately complex designs to avoid using a specific 'bad practice'. However, a bad practice can never be reduced to something as simplistic as 'never ever use keyword X'. All bad practices have there 'scope of ill-use'which may be very bigbut are almost never (if ever) absolute.
Thanks,
Mike N. Christoff
dmx_dawg@hotmail.com
(I hope you don't mind a bit of commentary here about 'bad practice phobia'...)
I had become tired of hearing the 'instanceof is bad in all conceivable situations' argument for years. It got to the point that if you mentioned you used instanceof in your code, the chorus in the newsgroups was that you had a poorly designed application and they didn't actually need to see your design to know this! :)
Its one of those issues where the fear of being thought of as being some newbie programmer, or worse, a 'hack', who still 'doesn't get' OO makes people create the most elaborately complex designs to avoid using a specific 'bad practice'. However, a bad practice can never be reduced to something as simplistic as 'never ever use keyword X'. All bad practices have there 'scope of ill-use'which may be very bigbut are almost never (if ever) absolute.
Thanks,
Mike N. Christoff
dmx_dawg@hotmail.com
Add Child Page to VisitorVersusInstanceOf