HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Filtering the attributes for a custom tag

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thetagcustomattributesforfiltering

Problem

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?

Solution

From a code review perspective, I'd say:

  • 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)



  • The long filter logic could be concisely expressed as a regular expression to avoid the long line of logic ("^((data|ng)-|on)")



  • 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

Code Snippets

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()#"';
    }, "" );
}

Context

StackExchange Code Review Q#121963, answer score: 5

Revisions (0)

No revisions yet.