5
\$\begingroup\$

I have function that takes all of the attributes passed to a custom tag, and returns a select subset. I am try to convert the current function, which does not use closures

string function passThrough(required struct attr)   output="false"  {


  local.result = "";


  for(local.myKey in arguments.attr)    {
    if (variables.myKey.left(5) == "data-" || variables.myKey.left(2) == "on" || variables.myKey.left(3) == "ng-")  {

        local.result &= ' #local.myKey.lcase()#="#arguments.attr[local.myKey].encodeForHTMLAttribute()#"';
        } // end if 
    }   // end for

return local.result;
}   

Into one that does

string function passThrough(required struct attr)   output="false"  {

  arguments.attr.filter(
    function(key, value) { 
        return (key.left(5) == "data-" || key.left(2) == "on" || key.left(3) == "ng-");
        }
    ).each(
    function(key, value)    {
        local.result &= ' #key.lcase()#="#value.encodeForHTMLAttribute()#"';
        }
    );  

return local.result;
}   

Both of these are invoked with

...  invoke("bootstrap", "passThrough", {attr = attributes});

Is this the right approach? Should arguments passed to the closure functions be scoped?

\$\endgroup\$
  • \$\begingroup\$ This topic is partially covered in: stackoverflow.com/questions/10604166/… \$\endgroup\$ – James A Mohler Mar 5 '16 at 7:34
  • \$\begingroup\$ I'd use reduce() rather than each() here, I think. That way you can just go return attr.filter(etc).reduce(etc). I'll post a more thorough answer if I get myself in front of a machine with CFML running on it later on. \$\endgroup\$ – Adam Cameron Mar 5 '16 at 7:55
4
\$\begingroup\$

OK, now that I've had coffee, here's my refactoring:

function extractCodeCentricAttributesToMarkupSafeAttributes(attributes){
    var relevantAttributePattern = "^(?:data-|ng-|on)(?=\S)";

    return attributes.filter(function(attribute){
        return attribute.reFindNoCase(relevantAttributePattern);
    }).reduce(function(attributeString, attributeName, attributeValue){
        return attributeString& ' #attributeName#="#attributeValue#"';
    }, "");
}

Notes on my implementation:

  • I could not get TestBox working on my ColdFusion 2016 install (since fixed), so I needed to use CF11 for this, hence using the function version of encodeForHTMLAttribute(). The method version is new to 2016.

  • we could probably argue over the best pattern to use for the regex all day. I specifically wanted to take a different approach to Dom's one, for the sake of comparison. I'm not suggesting mine is better. Just different. The key point we both demonstrate is don't use a raw regex pattern, always give it a meaningful name.

  • looking at the "single-expression-solution" I have here, the code is quite dense, and I can't help but think Dom's approach to separating them out has merit.

Code review notes:

  • your original function doesn't work. It specifies variables.myKey when it should be local.myKey. It's clear you're not testing your original code, let alone using TDD during the refactoring process. You must have test coverage before refactoring.

  • the function name is unhelpfully non-descriptive, as demonstrated by Dom not getting what it was doing. I don't think my function name is ideal, but it's an improvement. I guess if we knew why you were doing this, the function name could be improved to reflect that.

  • lose the scoping. It's clutter.
  • lose the comments. They're clutter.
  • don't abbrev. variable names. It makes the code slightly hard to follow.
  • don't have compound if conditions like that. It makes the code hard to read. Even if the condition couldn't be simplified back to one function call and you still needed multiple subconditions: extract them out into meaningful variable names, eg: isDataAttribute || isOnAttribute || isNgAttribute
  • key and value are unhelpful argument names
  • slightly controversial: but unless it's an API intended to be used by third-parties: lose the type checking. It's clutter in one's own code.
  • there's no need for the output modifier for the function in CFScript.
  • don't quote boolean values. It's just false not "false".

Unit tests for this:

component extends="testbox.system.BaseSpec" {

    function beforeAll(){
        include "original.cfm";
        include "refactored.cfm";
        return this;
    }

    function run(testResults, testBox){
        describe("Testing for regressions", function(){
            it("works with an empty struct", function(){
                var testStruct = {};
                var resultFromOriginal = passThrough(testStruct);
                var resultFromRefactored = extractCodeCentricAttributesToMarkupSafeAttributes(testStruct);
                expect(resultFromRefactored).toBe(resultFromOriginal);
            });
            it("works with an irrelevant attribute", function(){
                var testStruct = {notRelevant=3};
                var resultFromOriginal = passThrough(testStruct);
                var resultFromRefactored = extractCodeCentricAttributesToMarkupSafeAttributes(testStruct);
                expect(resultFromRefactored).toBe(resultFromOriginal);
            });
            it("works with each of the relevant attributes", function(){
                var relevantAttributes = ["data-relevant", "onRelevant", "ng-relevant"];
                for (relevantAttribute in relevantAttributes) {
                    var testStruct = {"#relevantAttribute#"=5};
                    var resultFromOriginal = passThrough(testStruct);
                    var resultFromRefactored = extractCodeCentricAttributesToMarkupSafeAttributes(testStruct);
                    expect(resultFromRefactored).toBe(resultFromOriginal);
                }
            });
            it("works with a mix of attribute relevance", function(){
                var testStruct = {notRelevant=7, onRelevant=11};
                var resultFromOriginal = passThrough(testStruct);
                var resultFromRefactored = extractCodeCentricAttributesToMarkupSafeAttributes(testStruct);
                expect(resultFromRefactored).toBe(resultFromOriginal);
            });
            it("works with multiple relevant attributes", function(){
                var testStruct = {"data-relevant"=13, onRelevant=17, "ng-relevant"=19};
                var resultFromOriginal = passThrough(testStruct);
                var resultFromRefactored = extractCodeCentricAttributesToMarkupSafeAttributes(testStruct);
                expect(resultFromRefactored).toBe(resultFromOriginal);
            });
        });

    }

}

Use them. NB: the includes in beforeAll() simply contain each version of the function.

\$\endgroup\$
  • \$\begingroup\$ These are all spot on. The resulting code will be very close to this \$\endgroup\$ – James A Mohler Mar 5 '16 at 17:28
5
\$\begingroup\$

From a code review perspective, I'd say:

  1. The function, "passthrough", is doing two things and doesn't describe what it does very clearly. I'd suggest two functions here, "filterAttributes" and "structToQueryString" (or whatever their exact purpose is)
  2. The long filter logic could be concisely expressed as a regular expression to avoid the long line of logic ("^((data|ng)-|on)")
  3. As for scoping closure arguments, I'd suggest this was a matter of preference. If the coding guidelines you work with ask for all variables to be scoped, then yes, they should be scoped. I personally find it more readable to not have them. Having small and concise functions should help prevent scoping issues.

Overall, I find the example without closures more readable, but the feedback above would make it more so IMO. Here's how it could look with closures:

struct function stripNonDataNgAndJsAttributes( required struct attributes ) {
    var patternToKeep = "^((data|ng)\-|on)\S";

    return attributes.filter( function( key, value ){
        return key.findNoCase( patternToKeep );
    } );
}

string function structToQueryString( required struct input ) {
    return input.reduce( function( result, key, value ){
        return result & ' #key.lCase()#="#value.encodeForHTMLAttribute()#"';
    }, "" );
}

http://trycf.com/scratch-pad/gist/d6ce9527fa3294b286cc

\$\endgroup\$
  • 1
    \$\begingroup\$ Good advice here. But since where were attributes starting with data- or on "non standard"? Isn't that like referring to ? as a "special character" in a password setting rule? ;-). Also the function name makes it sound like it's filtering them out, not actually only keeping those ones. I agree with the technique, but I don't think the name of that function is an improvement on the original. extractRelevantAttributes ? \$\endgroup\$ – Adam Cameron Mar 5 '16 at 9:04
  • \$\begingroup\$ Agreed, I was guessing at the intention. The name should reflect whatever the actual intention is and should be more specific. Given the focus of the logic, I think it's correct to focus on what it is taking out as it doesn't give any clues as to what it wants to keep in (other than everything else). \$\endgroup\$ – dom_watson Mar 5 '16 at 9:16
  • 1
    \$\begingroup\$ You've got the logic reversed though, Dom. The "special" attributes are the ones he wants to keep. Goes to show the function name is unsuitable though. \$\endgroup\$ – Adam Cameron Mar 5 '16 at 10:49
  • \$\begingroup\$ Oh yes, doh. Fixed. \$\endgroup\$ – dom_watson Mar 5 '16 at 13:44
  • \$\begingroup\$ FYI: I changed the function name to filterAttributes(). I like the RE approach rather than a bunch of || \$\endgroup\$ – James A Mohler Mar 5 '16 at 17:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.