This is part 4 of a series on turning functional code into great code. If you haven't read
part 1,
part 2 or
part 3, go ahead and do so and then come back! (Warning: they're pretty long and rambling; I'm
trying to improve on that front.)
Now the fun really begins: we're going to start looking at design! If you've taken any courses on object oriented design, I'm sure you've heard of several important principles such as
encapsulation and
polymorphism. It's easy to rattle these off on an exam, but it's not always clear how they apply to code in real life.
In this part of the series, we'll focus on encapsulation.
Encapsulation is roughly defined as keeping all the data and behavior related to an object enclosed inside that object. For an example of code that could be better encapsulated, let's have a look at the section of
StylesheetCreator below.
public void createStylesheet( String parentNodeName,
List<String> childNodeNames, List<List<String>> ruleList,
String outputFilename ) throws Exception
{
StylesheetHelper stylesheetHelper = new StylesheetHelper();
StringBuilder ruleContents = new StringBuilder();
int ruleNum = 1;
for ( List<String> rule : ruleList )
{
// ...
switch ( ruleType )
{
case CONCAT:
{
ruleContents.append( stylesheetHelper.concatRule( ruleNum,
columnName, param1 ) );
break;
}
// other cases here...
}
ruleNum++;
}
String stylesheetHeader = stylesheetHelper.getStylesheetHeader(
parentNodeName, childNodeNames );
// Get lots of other stylesheet pieces here...
// Construct the entire stylesheet.
String xsltText = stylesheetHeader + ruleContents + xsltChooseStart
+ parentNode + xsltWhenEnd + xsltOtherwiseStart
+ rejectedParentNode + xsltChooseEnd + stylesheetFooter;
// Create the output file.
FileWriter fw = new FileWriter(
new File( stylesheetDirectory, outputFilename ) );
try
{
fw.write( xsltText );
fw.write( "\n" );
}
finally
{
fw.close();
}
}
This code performs the following steps:
- Create a new
StylesheetHelper (line 5).
- Loop through a list of rules passed into it (line 9), retrieving XSLT fragments representing those rules from
StylesheetHelper (line 16 and others omitted here).
- Retrieve a bunch of other fragments of XSLT from the same class (line 26) and concatenate them together (line 31).
- Write the stylesheet to a file.
What's wrong with this? The problem is that the logic to build the stylesheet is not encapsulated in a single place. Instead, it is split between
StylesheetCreator, which keeps track of the pieces of the stylesheet and the current rule number, and
StylesheetHelper, which generates the pieces of the stylesheet based on a bunch of information passed into it. What would happen if
StylesheetCreator were to keep passing in the same rule number over and over? The resulting stylesheet would be invalid. Wouldn't it be better to design this in such a way that that could never happen?
In order to encapsulate our code better, we need to move all the stylesheet-related code into
StylesheetHelper. Then
StylesheetCreator just does this:
- Create a new
StylesheetHelper.
- Loop through a list of rules, asking the
StylesheetHelper to add those rules to the stylesheet.
- Retrieve the complete stylesheet from
StylesheetHelper.
- Write the stylesheet to a file.
This will simplify
StylesheetCreator and make
StylesheetHelper much more robust, because it will be impossible to call it incorrectly. In fact,
StylesheetHelper is really not a helper anymore, it truly represents an XSLT stylesheet.
Here's a tip: If you have a class that ends in
Helper,
Utils or
Manager, that is often a sign that you have a design problem. If a class's job is simply to help or manage some other class, perhaps that logic belongs on the class itself, or on some other class that has yet to be created. If you have utilities used by some other class, just put the methods on that class. And if you have a class that ends in, say,
DelegateManagerProviderFactoryUtils, then your code is probably beyond hope! ;)
So we can rewrite the above list of actions for
StylesheetCreator as follows:
- Create a new
Stylesheet.
- Loop through a list of rules, adding each to the
Stylesheet.
- Retrieve the full text of the
Stylesheet.
- Write the full text to a file.
We could even combine steps 3 and 4, so that we ask the stylesheet to write itself out. If we do all this, we end up simplifying and shrinking the amount of code in
StylesheetCreator. It now looks like this:
public void createStylesheet( String parentNodeName,
List<String> childNodeNames, List<List<String>> ruleList,
String outputFilename ) throws Exception
{
Stylesheet stylesheet =
new Stylesheet( parentNodeName, childNodeNames );
for ( List<String> rule : ruleList )
{
// ...
switch ( ruleType )
{
case CONCAT:
{
stylesheet.addConcatRule( columnName, param1 );
break;
}
// other cases here...
}
}
// Create the output file.
FileWriter fw = new FileWriter(
new File( stylesheetDirectory, outputFilename ) );
try
{
stylesheet.write( fw );
}
finally
{
fw.close();
}
}
Wouldn't you agree that that is much easier to read? The code is more concise, and we've been able to hide most of the internal parts of
StylesheetHelper (now
Stylesheet), so that the only public methods are all the
addXxxRule methods and the
write method. The fewer public methods on a class, the easier it is to use! Can you think of how we could reduce the number of methods even further? If not, don't worry; we'll cover that in part 5.
The only parts of this code that are still hard to read are the big switch statement on line 11 above (most of which I omitted), and the code on line 10, just inside the
for loop, which I also omitted. We'll tackle the big switch statement next time, but for now, let's take a look at the code inside the
for loop:
public void createStylesheet( String parentNodeName,
List<String> childNodeNames, List<List<String>> ruleList,
String outputFilename ) throws Exception
{
// ...
for ( List<String> rule : ruleList )
{
String columnName = rule.get( 1 );
int ruleId = Integer.parseInt( rule.get( 2 ) );
RuleType ruleType = RuleType.valueFromId( ruleId );
String param1 = rule.get( 3 );
String param2 = rule.get( 4 );
// ...
}
// ...
}
Each of the rules inside of
ruleList is currently represented as a
List<String>. But it's not really a list of strings: it's a column name, a rule type, and up to two rule parameters -- in other words, a structured type. In Java, we'd represent this as a class. I'm going to call this class
RuleInvocation. You might have expected it to be called a
Rule, but I'm going to use that class name for something else. See if you can figure out what that might be! (If you can't figure it out, just wait until part 5 and you'll find out.)
Here's what the first pass at
RuleInvocation looks like:
package net.galluzzo.example.stylesheetbuilder.refactored.part4;
public class RuleInvocation
{
private String columnName;
private int ruleId;
private String param1;
private String param2;
public RuleInvocation( String columnName, int ruleId, String param1, String param2 )
{
this.columnName = columnName;
this.ruleId = ruleId;
this.param1 = param1;
this.param2 = param2;
}
public String getColumnName()
{
return columnName;
}
public RuleType getRuleType()
{
return RuleType.valueFromId( ruleId );
}
public String getParam1()
{
return param1;
}
public String getParam2()
{
return param2;
}
}
Yeah, it's pretty long and has almost no behavior (yet). Unfortunately Java is pretty verbose here; in Scala or Ruby, for example, this would be only a few lines long. The calling code in
StylesheetCreator now looks like this:
public void createStylesheet( String parentNodeName,
List<String> childNodeNames, List<RuleInvocation> ruleList,
String outputFilename ) throws Exception
{
// ...
for ( RuleInvocation rule : ruleList )
{
String columnName = rule.getColumnName();
RuleType ruleType = rule.getRuleType();
String param1 = rule.getParam1();
String param2 = rule.getParam2();
// ...
}
// ...
}
We also need to change our test case, since we just altered the public interface of
StylesheetCreator. After we do this and re-run it, we'll find that it still passes. So our code still works!
RuleInvocation is really another example of code that is not perfectly encapsulated. In the original code, the information about what each of the
Strings in the rule list represents had to be understood by both the
StylesheetCreator and the code that calls it. What if the calling code had passed in the string
"George" for the rule ID? What if the caller forgot to add the weird extra unused string at the beginning of each list?
StylesheetCreator would have thrown an exception in either case. But by putting rules in objects where each member is named and typed, these sorts of errors will not happen.
Actually, even the new code is not fully encapsulated, because
StylesheetCreator is still digging around to get at the members of the
RuleInvocation (for example, by calling
rule.getColumnName()). Ideally, the
RuleInvocation should have all the necessary behavior on it so that the calling code does not need to look at its members, even via JavaBean accessors. This is called
data hiding, and is another part of encapsulation. But we're not quite at the point where we can do that in this particular case.
In fact, that's where we'll leave it for today. We've discussed encapsulation -- both the "single point of responsibility" and "data hiding" senses -- and how it applies to this code. Next time, we'll take a look at polymorphism and how we can use that to improve this code even further. Stay tuned!
Posts
