Form Design Code Review

A while ago I was asked to code review a customer form.  I enjoy doing this in order to get an understanding of what customers are trying to do with their forms and where their pain points are.  In this case, I wanted to share some of the code review on my blog so that others can learn from the experience.  This post is a miscellaneous collection of XFA scripting suggestions arising from one customer form.

Repetitive Code

The form had a large block of script that executed before print that looked like:

if (Page1.InvestProdSupp.InvestProdSupp1.MplsInd.rawValue == 0)
   {Page1.InvestProdSupp.presence = "hidden";}
   else
   {Page1.InvestProdSupp.presence = "visible";}

if (Page1.CompMan.CompMan1.MplsInd.rawValue == 0)
   {Page1.CompMan.presence = "hidden";}
   else
   {Page1.CompMan.presence = "visible";}

if (Page1.Oversight.Oversight1.MplsInd.rawValue == 0)
   {Page1.Oversight.presence = "hidden";}
   else
   {Page1.Oversight.presence = "visible";}

This pattern shown 3 times above was repeated 31 times in the method.  A total of around 155 lines of code. Whew.  I enjoy writing code as much as the next person, but this repetitive code is tedious and error prone.  Added or removed a new subform? Don’t forget to update this script. Copy and pasted some script code?  Hope you updated all the relevant pieces correctly.

The general pattern is that there are a bunch of fields called "MpIsInd" under nested subforms.  When the value of this field is zero, hide the parent subform, otherwise make it visible.  Here are two alternative approaches:

Solution 1: Use Recursion

We could define a method to recursively search for fields called "MpIsInd" and apply the logic to each one we find:

function printPrep(vObject)
{
    // Have we found the trigger field?
    if (vObject.name == "MplsInd")
    {
        if (vObject.rawValue == 0)
            vObject.parent.presence = "hidden";
        else
            vObject.parent.presence = "visible";

        // Once we’ve found the field, we can exit early.
        return;
    } else if (vObject.className == "subform")
    {
        // Call this method recursively for each child.
        for (var i=0; i<vObject.nodes.length; i++)
            printPrep(vObject.nodes.item(i));
    }
}
// Set visibility based on all the nested MplsInd descendents
printPrep(Page1);

The end result is 21 lines of script instead of 155 — and a form that is easier to maintain.

Solution2: Use wildcards in SOM

But recursion is not for everyone.  It certainly would not come easily to a novice script writer

The form author could also have used SOM expressions to find all the MplsInd fields.  We are used to using the wildcard character (*) inside braces to indicate "all occurrences". e.g. po.item[*].subtotal.  I suspect it is not widely known that the wildcard character may also appear in place of an element name to indicate "all elements".  Using the wildcard this way, our SOM expression to find all MplsInd grandchildren of Page1 is : Page1.*.MplsInd.

Given that these fields occur as either grandchildren of Page1 or as great-grandchildren, we can use two calls to resolveNodes() to find them:

// Generate lists of the descendant MplsInd fields…
var vList1 = xfa.resolveNodes("Page1.*.MplsInd");
var vList2 = xfa.resolveNodes("Page1.*.*.MplsInd");
for (var i=0; i<vList1.length; i++)
{
    var vChild = vList1.item(i);
    vChild.parent.presence =
                  vChild.rawValue == 0 ? "hidden" : "visible";
}
for (var i=0; i<vList2.length; i++)
{
    var vChild = vList2.item(i);
    vChild.parent.presence = 
                  vChild.rawValue == 0 ? "hidden" : "visible";
}

Relative SOM expressions

There are buttons on the form that insert subforms after the current ancestor subform.  The click event script looks like:

fields.Page1.RevSec.RevSec2.Table1.Row1.Table2.Row1.Cell1::click – (JavaScript, client)fields.Page1.RevSec.RevSec2.Table1.Row1.instanceManager.insertInstance(this.parent.parent.parent.index+1);

That is pretty verbose.  A more readable form of this would use relative references to the instance manager and to the ancestor subform:

fields.Page1.RevSec.RevSec2.Table1.Row1.Table2.Row1.Cell1::click – (JavaScript, client)
Table1._Row1.insertInstance(Table1.Row1.index+1);

Resolving SOM expressions involves what we call "scoped matching".  In this case, the starting context is the Cell1 button (the field hosting the script).  When asked to find Table1, we look for a match by looking at the children of Cell1.  If not found, we expand the scope of the search by moving up the hierarchy and checking the parent element: Row1 and the children of Row1.  This continues until we encounter Table1.  Note that because there were two Row1 elements in the hierarchy, we differentiated them by including Table1 in the SOM expression.  Note also that we did not worry about which instance of Row1 was returned.  The scoped match will find the direct ancestor before any of its siblings.

The replacement code found the instance manager using its name: _Row1 whereas the original used Row1.instanceManager. Using _Row1 is a better practise, because when there are zero instances of a
subform, the search for Row1 will fail, whereas _Row1 is always available.  The naming convention for instance managers is the subform name prefixed with an underscore.

Locking a form

The form had a script to lock all the fields in the form:

function lockForm(sState){
    try{   
        // Get the field containers from each page.
        for (var i = 0; i < xfa.layout.absPageCount(); i++) {
            var oFields = xfa.layout.pageContent(i, "field");
            var nodesLength = oFields.length;
            // Set the access type.
            for (var j = 0; j < nodesLength; j++) {
                var oItem = oFields.item(j);
                if (oItem != this) {
                    oItem.access = sState;
                }
            }
        }
    }catch(e){
        app.alert("Error in function lockForm() – " + e);
    }
}

Notice that this script is as efficient as possible when it loops through the children in a list. It declares a variable to hold the size of the list, and uses that variable in the "for loop". The advantage of doing this is that in a "for loop" the condition gets evaluated for each iteration of the loop.  It is more efficient to reference a JavaScript variable than it is to evaluate a list.length property.  If you have a "for loop" in an area of performance critical code, this is good practise.

Notice another clever part of the script — it makes sure not to lock the current field.

Unfortunately, it was hard to tell how the script was actually used, because it was not referenced anywhere on the form.  But a couple of observations:

As of Reader 9.0, the access property is now available on subforms.  If your target version is Reader 9, you can code: subform.access = "readOnly" and the result will lock all the descendant fields of the subform.  Setting this property on the root subform would lock the entire form — in one JavaScript statement.

Removing a Subform

The form has buttons to remove subform instances with code that looks like this:

fields.Page1.RevSec.RevSec2.Table1.Row1.Table2.Row1.Cell2::click – (JavaScript, client)
fields.Page1.RevSec.RevSec2.Table1.Row1.instanceManager.removeInstance(this.parent.parent.parent.index);

For starters, we can simplify the SOM expressions as described above. But the other problem is that this particular Row1 subform is defined to have a minimum of one instance.  This means that if the user clicks on the button to remove the last instance of Row1, Reader will generate an error.  The way Reader handles JavaScript errors is to write them to the JavaScript console.  The end user does not get an indication that there was an error.  But in the console you will see:

GeneralError: Operation failed.
XFAObject.removeInstance:1:XFA:fields[0]:Page1[0]:RevSec[0]:RevSec2[0]:Table1[0]:Row1[0]:Table2[0]:Row1[0]:Cell2[0]:click
The element [min] has violated its allowable number of occurrences.

This version of the script protects against attempting to remove beyond the minimum number of instances:

fields.Page1.RevSec.RevSec2.Table1.Row1.Table2.Row1.Cell2::click – (JavaScript, client)
if (Table1._Row1.count > Table1._Row1.min)
    Table1._Row1.removeInstance(Table1.Row1.index);

If the number of subforms has an upper limit, you should similarly guard code that adds instances.

When testing a form it is good practise to leave the JavaScript console open and make sure that it is clear when testing is complete.