Re: [REVIEW] Cleanup/Minor refactoring of Generators in jsf-tools

From: Ed Burns <Ed.Burns_at_Sun.COM>
Date: Mon, 13 Dec 2004 07:08:46 -0800

Hello Ryan,

This looks good, but I have some comments. I don't need to see another
change bundle, but I want to see your response before I give the final
review and approval.



>>>>> On Thu, 09 Dec 2004 11:07:48 -0500, Ryan Lubke <Ryan.Lubke_at_Sun.COM> said:

RL> Cleanup up jsf-tools generator code in preparation for JSF 1.2 work.
RL> Generators (HtmlComponent, HtmlTaglib, and RenderKitSpecification) all
RL> configured via a properties file. This should allow enough flexibility
RL> to use
RL> the taglib generator and renderkit doc generators with any faces-config.

RL> SECTION: Modified Files
RL> ----------------------------
RL> M jsf-ri/build.xml
RL> - copy the TAG-DEF files to jsf-ri before generating tags

Question: why are we copying them to ${basedir}? Generally, we don't
like to put ephemeral artifacts into ${basedir}. Can these be made to
reside inside ${basedir}/build during their existence?

RL> M jsf-tools/src/com/sun/faces/generate/
RL> - general cleanup
RL> - moved some methods common to all generator types to new GeneratorUtils
RL> - Added new inner class CodeWriter with general helper methods for
RL> writing read-only, write-only, read/write properties, various comment
RL> types. This writer also simplifies formatting. User needs to call
RL> indent
RL> or unindent instead of adding formatting directly into Strings as
RL> before.

RL> Index: jsf-tools/src/com/sun/faces/generate/
RL> ===================================================================


RL> + protected static final Set JAVA_KEYWORDS = new HashSet(57);

I don't see the real value in hard coding the size. If someone were to
add or remove an entry in the future, this may cause an IOOBE. In the
name of futureproofing, please return to having no size. Since this is
a generator, performance isn't as key.


RL> + protected static Map UNWRAPPERS = new HashMap(8);

Same thing here.

RL> + protected static Map WRAPPERS = new HashMap(8);

and here.

RL> +
RL> + public void unindent() {
RL> +
RL> + depth.pop();
RL> + updateFormatString(depth.size());
RL> +
RL> + } // END unindent

Personally I prefer "outdent" to "unindent" but it's your call. This
inner class is handy. It would seem to have a wider

RL> M jsf-tools/src/com/sun/faces/generate/
RL> - general cleanup
RL> - leverage CodeWriter

RL> Index: jsf-tools/src/com/sun/faces/generate/
RL> ===================================================================

RL> // Generate the copyright information
RL> - writer.write(copyright);
RL> + writer.writeJavadocComment(
RL> + propManager.getProperty(PropertyManager.COPYRIGHT));
RL> // Generate the package declaration
RL> - writer.write("\npackage javax.faces.component.html;\n");
RL> + writer.fwrite("\npackage javax.faces.component.html;\n");
RL> writer.write("\n\n");
RL> // Generate the imports
RL> - writer.write("import;\n");
RL> - writer.write("import javax.faces.context.FacesContext;\n");
RL> - writer.write("import javax.faces.el.MethodBinding;\n");
RL> - writer.write("import javax.faces.el.ValueBinding;\n");
RL> + writer.fwrite("import;\n");
RL> + writer.fwrite("import javax.faces.context.FacesContext;\n");
RL> + writer.fwrite("import javax.faces.el.MethodBinding;\n");
RL> + writer.fwrite("import javax.faces.el.ValueBinding;\n");
RL> writer.write("\n\n");

If we're going to have methods like writeReadWriteProperty(), why not

public void writePackage(String fqpn);

public void writeImport(String fqcn);

Of course, if you want to be fancy, you could have addImport(String) and
then writeImports() and this would have the effect of sorting the
imports before writing them. Now, this would be a departure from your
existing model, though. It would be the first time anything gets
buffered before being output. When you start to go that far, you have
to wonder if there is some other code out there, perhaps in netbeans,
that facilitates the code generation process by enabling you to
configure a java object instance with a bunch of code properties, and
then kick it to write itself to a Writer or something. As it is now,
you have a nice compromise, so I'd settle for adding
RL> // Generate the restoreState() method
RL> - writer.write(" public void restoreState(FacesContext _context,
RL> Object _state) {\n");
RL> - writer.write(" Object _values[] = (Object[]) _state;\n");
RL> - writer.write(" super.restoreState(_context, _values[0]);\n");
RL> + writer.fwrite(
RL> + "public void restoreState(FacesContext _context, Object
RL> _state) {\n");
RL> + writer.indent();

Yes, this illustrates my point that we have a compromise between a true
OO solution, and a PrintWriter with a few extra methods. A true OO
solution would have a method that writes a method, taking the signature
in some high level way. But let's not go there.

RL> M jsf-tools/src/com/sun/faces/generate/
RL> - general cleanup
RL> - Use the data contained in the properties file to initialize the
RL> previously
RL> static structures

What you lose when you do this is the ability to look in the file and see exactly what the complete set of
configuration info the system needs. A lot of work went into making
those static structures have the right set of values. Now, I'm aware
you've moved the responsibility for containing that set of values into
the PropertyManager, and that certainly is more flexible, but who checks
that the set of values is correct? In other words, previously,
HtmlTaglibGenerator was responsible for "knowing" the correct set of
config values. Now has this
responsibility. Please make it clear that the system utterly depends on
HtmlBasicTaglib12 being correct to function properly.

Also, there are a number of constants for which I see no modern analog:

RL> - private static final String TAGCLASSPATH =
RL> "com.sun.faces.taglib.html_basic";
RL> - private static final String TEICLASS =
RL> "com.sun.faces.taglib.FacesTagExtraInfo";
RL> - private static final String BODYCONTENT = "JSP";
RL> - private static final String REQUIRED = "false";
RL> - private static final String RTEXPRVALUE = "false";

What happened to these?

There was a lot of special case code in this class. Generally, it's
hard to refactor that sort of thing and make sure all the cases are
still covered. I bet that's why we avoid special case code at all
costs. Anyway, are you certain you've migrated all the special cases to
the new design correctly?

RL> Index: jsf-tools/src/com/sun/faces/generate/
RL> ===================================================================

RL> + // ValueHolder and ConvertibleValueHolder components
RL> + st = new StringTokenizer(propManager.getProperty(
RL> + PropertyManager.VALUE_HOLDER_COMPONENTS), ",");
RL> + convertibleValueHolderComponents = new ArrayList(st.countTokens());
RL> + while (st.hasMoreTokens()) {
RL> + convertibleValueHolderComponents.add(st.nextToken());
RL> }
RL> - reader.close();
RL> - if (log.isDebugEnabled()) {
RL> - log.debug(" Copyright text contains " + sb.length() +
RL> - " characters");
RL> +
RL> + // ValueBinding enabled properties
RL> + st = new StringTokenizer(propManager.getProperty(
RL> + PropertyManager.VALUE_BINDING_PROPERTIES), ",");
RL> + valueBindingEnabledProperties = new ArrayList(st.countTokens());
RL> + while (st.hasMoreTokens()) {
RL> + valueBindingEnabledProperties.add(st.nextToken());

If you're going to have a PropertyManager, why make the caller do the
StringTokenizing? Why not add a method that does

StringTokenizer toker =

Or even have it give you an Iterator.

RL> A jsf-tools/conf/
RL> - generator properties file to generate taglibs, components, and
RL> renderkit docs
RL> for JSP 1.2

As I mention above. This file must be correct. Please make sure it's
clear to future developers how important the contents of this file are.

RL> A jsf-tools/src/com/sun/faces/generate/
RL> - Base class for concrete JspTDLGenerator implmentations

The XMLWriter class seems to have a lot of overlap with the
HtmlResponseWriter in the JSF RI. If this is true, and there is
significant cut and paste code reuse, I'd like to know why you had to
resort to cut and paste re-use.


|  | {home: 407 294 2468, office: 408 884 9519 OR x31640}
| homepage:         |
| aim: edburns0sunw | iim:
To unsubscribe, e-mail:
For additional commands, e-mail: