>Inlining Code leads to Untestable Code

>I’ve notice that particularly in the UI work, there seems to be a pattern of inlining declarations and overrides for classes. This is a short cut coding method that may be done for various reasons.

1. Performance.
2. Convenience.

The problem here is that modern compilers will take care of the inlining in most cases for optimization, and the convenience of this can lead to duplicate code, and untestable code in a system. A case in point comes from a recent bug report for XSL Tools, where the Result View would return a NullPointerException if a doctype or Processing Instruction was not found in the incoming text. Since I like to have unit tests to go along with the fixes that are provided, I want to be able to test this particular code. However, when I started investigating it I found the following code:


private void handleDebugTarget(JAXPDebugTarget xdt)
{
// first, clear the viewer
sv.setDocument(null);

final Reader reader = xdt.getGenerateReader();
IWorkbenchSiteProgressService service = (IWorkbenchSiteProgressService)getSite().getService(IWorkbenchSiteProgressService.class);
service.schedule(new Job("Result view job"){
@Override
protected IStatus run(IProgressMonitor monitor) {
IStatus status = Status.OK_STATUS;
try {
char[] c = new char[8192]; // this is the default BufferedWriter size, so we will usually get chunks of this size
int size;
while((size = reader.read(c)) != -1)
{
writeString(new String(c,0,size));
}
}
catch (IOException e) {
// ignore
}
finally {
monitor.done();
}
return status;
}

private void writeString(final String s) {
getSite().getShell().getDisplay().syncExec(new Runnable(){

public void run() {
// if this is the first lot of data, determine the correct content type and set the appropriate document
if (sv.getDocument() == null) {
IDocument document;
if (s.startsWith("<!DOCTYPE html")) {
String contentType = "org.eclipse.wst.html.core.htmlsource";
IStructuredModel scratchModel = StructuredModelManager.getModelManager().createUnManagedStructuredModelFor(contentType);
document = scratchModel.getStructuredDocument();
} else if (s.startsWith("<?xml")) {
String contentType = "org.eclipse.core.runtime.xml";
IStructuredModel scratchModel = StructuredModelManager.getModelManager().createUnManagedStructuredModelFor(contentType);
document = scratchModel.getStructuredDocument();
} else {
// TODO how to create a plain text Document??
// Here is the bug that needs to be tested.
document = null;
}
sv.setDocument(document);
}
try {
sv.getDocument().replace(sv.getDocument().getLength(), 0, s);
}
catch (BadLocationException e) {
XSLDebugUIPlugin.log(e);
}
sv.revealRange(sv.getDocument().getLength(),0);
PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage().activate(ResultView.this);
}
});
}
});
}

The problem with the above is that the issue that needed testing is nested in the override of the second inline declaration of Runnable in the run method. This type of code can not be unit tested. It also embeds business logic into the UI as well. Plus, it is difficult to follow with so many overrides and nesting levels. So, what to do? The solution was to refactor the code and get rid of the inline declarations. The refactoring did the following:

1. Created a ResultViewJob class which extends the Job class, and overrides the run method.
2. Create a ResultRunnable class which implements Runnable, and the necessary run method.
3. Refactored run method in ResultRunnable so that bug could be tested.

This simple refactoring now allows the bug to be tested, and also potentially makes this business logic available to other classes that may need it as well. The full refactored code can be seen here.

Sometimes taking the most convenient way is not necessarily the cleanest or easiest to maintain way. It leads to code that is more difficult to test and to reuse. One thing I have found with testable code, is that there are very few private classes, inlining of declarations, and other short cut or convenience methods taken when writing the code. All of these have their place, but in many cases like XML have become over used just for convenience sake.

Advertisements
This entry was posted in agile, eclipse, testing, xml. Bookmark the permalink.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s