4
\$\begingroup\$

I have created a set of functions, that when given a struct of structs full of names, it will highlight those names that appear in a body of HTML based text. Is this the best method of doing this?

<cfcomponent>
  <cffunction name="init" access="public" output="false" returntype="PartnerHighlighter" hint="Pseudo-constructor">
    <cfargument name="stPartners"   type="struct" required="true">
    <cfargument name="sLink" type="string" required="true" hint="the link to use when replacing.">
    <cfscript>
      variables.stPartners  = arguments.stPartners;
      variables.sStopString = '<span class="nolink">';
      variables.sLink       = arguments.sLink;

      return this;
    </cfscript>
  </cffunction>

    <cffunction name="highlightPartners" returntype="string" access="public" hint="this searches through the partner struct and highlights the partner">
<cfargument name="sSearchText" type="string" required="true" hint="the text to search">
<cfargument name="sHighlightType" type="string" required="false" default="link" hint="can add others if needed later">
<cfscript>
  local.sHighlightedText              = arguments.sSearchText;
  local.lstPunct                      = '';
  local.lstPunct                      = listAppend(local.lstPunct, ",", ',');
  local.lstPunct                      = listAppend(local.lstPunct, ".", ',');
  local.lstPunct                      = listAppend(local.lstPunct, " ", ',');
  local.lstPunct                      = listAppend(local.lstPunct, "'", ',');
  local.lstPunct                      = listAppend(local.lstPunct, '"', ',');
  local.lstPunct                      = listAppend(local.lstPunct, '’', ',');
  local.lstPunct                      = listAppend(local.lstPunct, '<', ',');
  local.lstPunct                      = listAppend(local.lstPunct, '>', ',');
  local.lstPunct                      = listAppend(local.lstPunct, '&', ',');
  local.lstPunct                      = listAppend(local.lstPunct, '?', ',');
  local.lstPunct                      = listAppend(local.lstPunct, '!', ',');
  local.lstReplaced                   = "";

  for(local.stPartner in variables.stPartners){
        local.stArguments.stPartner   = variables.stPartners[local.stPartner];
        local.stArguments.sHighlightType          = arguments.sHighlightType;
        local.nNameLength             = len(local.stArguments.stPartner.sFullName);

        local.nExists                 = findnocase(local.stArguments.stPartner.sFullname, local.sHighlightedText);
        local.aBytes                  = local.sHighlightedText.GetBytes(); 

        if(local.nExists gt 0){
            local.sSupposedName       = mid(local.sHighlightedText, local.nExists, local.nNameLength);

            local.nNextByte           = (local.nExists-1)+(local.nNameLength+1);

            if(listFindNoCase(local.lstPunct, Chr(local.aBytes[local.nNextByte]), ',') gt 0){
                local.nStop           = findNoCase(variables.sStopString,local.sHighlightedText,local.nExists-22);
                if(listFindNoCase(local.lstReplaced, local.stArguments.stPartner.sFullName, "|") eq 0 AND local.nStop eq 0){
                    local.sHighlighted          = getHighlight(argumentCollection=local.stArguments);
                    local.sHighlightedText      = rereplacenocase(local.sHighlightedText, local.stArguments.stPartner.sFullname, local.sHighlighted, "all");
                    local.lstReplaced           = listAppend(local.lstReplaced, local.stArguments.stPartner.sFullName, "|");
                }
            }
         }
     }
     return local.sHighlightedText;
  </cfscript>
</cffunction>

<cffunction name="getHighlight" returntype="string" access="private" hint="do the actual highlight">
  <cfargument name="stPartner" type="struct" required="true" hint="the partner">
  <cfargument name="sHighlightType" type="string" required="true" hint="can add others if needed later">
  <cfscript>
    var stLocalPartner          = arguments.stPartner;
    var sLocalHighlightType     = arguments.sHighlightType;
    var sHighlight              = "";
    var sNewLink                = "";

    if(local.sLocalHighlightType eq "link"){
        local.sNewLink          = rereplacenocase(variables.sLink, "\[id\]", local.stLocalPartner.nID, "all");
        local.sNewLink          = rereplacenocase(local.sNewLink, "\[lastname\]", local.stLocalPartner.sLastName, "all");
        local.sNewLink          = rereplacenocase(local.sNewLink, "\[dupe\]", local.stLocalPartner.sIsDupe, "all");
        local.sNewLink          = rereplacenocase(local.sNewLink, "\[seo\]", local.stLocalPartner.sFullName, "all");
        local.sHighlight        = '<a href="#local.sNewLink#" title="more information on this expert from example.com" target="_blank">#local.stLocalPartner.sFullName#</a>';
        local.sHighlight        = "<span>#local.sHighlight#</span>";
    }
    return local.sHighlight;
    </cfscript>
  </cffunction>
</cfcomponent>

<cfscript>
  variables.stPartners      = {'123'={sFullName='Michael Fish',nID='133',sLastName='Fish',sIsDupe='No'},'333'={sFullName='Michael Fisher',nID='133',sLastName='Fisher',sIsDupe='No'}};

  variables.stArgs          = {};
  variables.stArgs.stPartners = variables.stPartners;
  variables.stArgs.sLink    = '//example.com/profiles/[id]/[dupe]/[lastname]/[seo]/';

  variables.oPartnerHighlight = createObject('component','PartnerHighlight').init(argumentCollection=variables.stArgs);

  variables.stArgs = {};
  variables.stArgs.sSearchText = '<p>Michael Fish is a Weatheman</p>';

  writeOutput(variables.oPartnerHighlight.highlightPartners(argumentCollection=variables.stArgs));

  //expecting output <p><a href="//example.com/profiles/133/0/Fish/Michael Fish/" title="more information on this expert from example.com" target="_blank">Michael Fish</a> is a Weatheman</p>

  variables.stArgs = {};
  variables.stArgs.sSearchText = '<p>Michael Fi is a name</p>';

  writeOutput(variables.oPartnerHighlight.highlightPartners(argumentCollection=variables.stArgs));

  //expecting output <p>Michael Fi is a name</p>

  variables.stArgs = {};
  variables.stArgs.sSearchText = '<p>John Travolta is an Actor</p>';

  writeOutput(variables.oPartnerHighlight.highlightPartners(argumentCollection=variables.stArgs));

  //expecting output <p>John Travolta is an Actor</p>
</cfscript>

I've realised I haven't documented this very well.

So variables.sStopString tells the functions that if you come across a name wrapped in a <span class="nolink"> not to link the name. so <span class="nolink">Michael Fish</span> should stop a link being put around the name, even if it exists in the struct of structs.

The punctuation is a little bit iffy, but i'm trying to say that if the character next to the supposed name is of type punctuation, then it's likely ended, rather than if the next character were an e or alphabetic. This might fall down on international names.

\$\endgroup\$
  • \$\begingroup\$ what version of coldfusion are you using? \$\endgroup\$ – Ryan Guill Oct 31 '14 at 15:26
  • \$\begingroup\$ Coldfusion 9 (pure) \$\endgroup\$ – Jarede Oct 31 '14 at 15:27
2
\$\begingroup\$
    <cffunction name="highlightPartners" returntype="string" access="public" hint="this searches through the partner struct and highlights the partner">
<cfargument name="sSearchText" type="string" required="true" hint="the text to search">
<cfargument name="sHighlightType" type="string" required="false" default="link" hint="can add others if needed later">
<cfscript>
  local.sHighlightedText              = arguments.sSearchText;
  local.lstPunct                      = '';
  local.lstPunct                      = listAppend(local.lstPunct, ",", ',');
  local.lstPunct                      = listAppend(local.lstPunct, ".", ',');
  local.lstPunct                      = listAppend(local.lstPunct, " ", ',');
  local.lstPunct                      = listAppend(local.lstPunct, "'", ',');
  local.lstPunct                      = listAppend(local.lstPunct, '"', ',');
  local.lstPunct                      = listAppend(local.lstPunct, '’', ',');
  local.lstPunct                      = listAppend(local.lstPunct, '<', ',');
  local.lstPunct                      = listAppend(local.lstPunct, '>', ',');
  local.lstPunct                      = listAppend(local.lstPunct, '&', ',');
  local.lstPunct                      = listAppend(local.lstPunct, '?', ',');
  local.lstPunct                      = listAppend(local.lstPunct, '!', ',');
  local.lstReplaced                   = "";

  for(local.stPartner in variables.stPartners){
        local.stArguments.stPartner   = variables.stPartners[local.stPartner];
        local.stArguments.sHighlightType          = arguments.sHighlightType;
        local.nNameLength             = len(local.stArguments.stPartner.sFullName);

        local.nExists                 = findnocase(local.stArguments.stPartner.sFullname, local.sHighlightedText);
        local.aBytes                  = local.sHighlightedText.GetBytes(); 

        if(local.nExists gt 0){
            local.sSupposedName       = mid(local.sHighlightedText, local.nExists, local.nNameLength);

            local.nNextByte           = (local.nExists-1)+(local.nNameLength+1);

            if(listFindNoCase(local.lstPunct, Chr(local.aBytes[local.nNextByte]), ',') gt 0){
                local.nStop           = findNoCase(variables.sStopString,local.sHighlightedText,local.nExists-22);
                if(listFindNoCase(local.lstReplaced, local.stArguments.stPartner.sFullName, "|") eq 0 AND local.nStop eq 0){
                    local.sHighlighted          = getHighlight(argumentCollection=local.stArguments);
                    local.sHighlightedText      = rereplacenocase(local.sHighlightedText, local.stArguments.stPartner.sFullname, local.sHighlighted, "all");
                    local.lstReplaced           = listAppend(local.lstReplaced, local.stArguments.stPartner.sFullName, "|");
                }
            }
         }
     }
     return local.sHighlightedText;
  </cfscript>
</cffunction>

Don't use a List (which, by the way, is just a string, so I wonder whether having , in , delimited string will go well) for anything but printing output. There are just so many pitfalls that you can fall in that it's not funny anymore. In this case, use a String instead:

  local.punctation = ",. '\"’<>&?!";

Then, when you need to check if the string contains any of these, instead of

  if(listFindNoCase(local.lstPunct, Chr(local.aBytes[local.nNextByte]), ',') gt 0)

use java.lang.String.contains instead.

  if(local.punctation.contains(Chr(local.aBytes[local.nNextByte])){

ColdFusion Strings are Java Strings. ColdFusion Lists are Java Strings. Get to know the underlying classes, because Coldfusion's functions come with many gotchas, and I personally do not view the language as safe anymore.

See this StackOverflow answer why you can't put comma's in comma delimited lists. Basically, your comma is interpreted as a delimiter.

\$\endgroup\$
  • \$\begingroup\$ Yep, the comma is very much a d'oh moment. Rookie mistake on my part. \$\endgroup\$ – Jarede Nov 2 '14 at 9:54
  • \$\begingroup\$ Even although it is a "rookie mistake", when you're used to abstracting the delimiter away (It's a list, who cares about the delimiter?), this becomes a dangerous gotcha. Getting to know the Java classes will help you enormously - doing certain things becomes amazingly easy. If you ever find yourself wishing for a specific function, see if Java has it already. \$\endgroup\$ – Pimgd Nov 2 '14 at 13:31
2
\$\begingroup\$

Additional observations:

  • there's no need for this CFC to use tags. Get rid.
  • consider losing the hungarian notation. It's just noise, and it seldom actually helps achieve its intended purpose (reading: http://blog.adamcameron.me/2012/08/hungarian-notation.html). If you need H/N to understand what your variables are, then you variables need better names. And, really, almost always a variable's general type can be inferred from its usage. You don't need to carry its type around with it all over the place: that just reduces your code's overall readability, slightly.
  • someone pointed out that you don't need to use listFindNoCase() in the context you do, and suggests using Java.lang.String's .contains() method instead. This will work in your current context, but does a different thing to listFind(), so it's not very solid advice. However there is a contains operator in CFML which should do the trick for your purposes here. Even better (keep the Java usage to a minimum: when you have to, not "when you can").
  • don't use a great sea of listAppend() calls. Just use a literal. But use a literal array instead. "Avoiding lists" was good advice. In your case you must avoid using a comma-delimited list as your data contains a comma.
  • you hint your init() method as the pseudo-constructor. Firstly: a hint like this is unhelpful as it's clear what the init() method is without it. Secondly its inaccurate because in CFML parlance, the pseudo constructor is the code outside the other methods. It's also inaccurate because it's the actual constructor. There's nothing "pseudo" about it. Avoid using hints and comments that simply state the obvious (or are - as in this case - wrong). Reading: http://blog.adamcameron.me/2014/10/comments-in-code-avoid-if-possible.html
  • you can flatten your code if you do this: if (!local.nExists) continue;. Then all the code after if does not need to be in a conditional block. This is cleaner code.
  • same with the next if()
  • and the one in getHighlight()
  • you're kind of misusing struct literals in your calling code. Don't use it as a proxy for a structNew() call and then follow it with separate assignments: use the literal to create the whole struct.
  • consider using new rather than createObject(), perhaps? It just looks nicer (I know, this is subjective).
  • I'm not crazy about hard-coding the mark-up handling in the CFC itself: it seems a bit usage-limiting to me. I know one cannot use inline function expressions in CF9, but one can use callbacks provided the functions are already defined. I might consider how one can pass the mark-up decoration in, rather than hard-code it.

I've not checked the logic of what you're doing, figuring other ppl will do that.

\$\endgroup\$
  • \$\begingroup\$ What's the difference between listFindNoCase and .contains when you're using punctuation? Aside from the integer/boolean difference. \$\endgroup\$ – Pimgd Nov 2 '14 at 13:35
  • \$\begingroup\$ And, I'm just curious here, why prefer ColdFusion over Java? Is there some danger to using Java methods? \$\endgroup\$ – Pimgd Nov 2 '14 at 13:42
  • \$\begingroup\$ I said "This will work in your current context, but does a different thing to listFind(), so it's not very solid advice". Here it's fine. But in general they are disparate functions. This gist demonstrates: gist.github.com/daccfml/392a372cdb9c5066d9ac. listFind() only matches the entire list element; .contains() matches anywhere in the string. \$\endgroup\$ – Adam Cameron Nov 2 '14 at 18:00
  • \$\begingroup\$ If the functionality is available in CFML, there's no need to potentially muddy the water by dropping down to another language to effect the same thing. Nothing is gained by doing it, and it - slightly - obfuscates the code. The CF server also has to mess around a bit more to execute Java methods, over its own functions (needing to horse around with reflection to cast arguments, etc). Stick with CFML when you can; drop to Java methods if needs must. \$\endgroup\$ – Adam Cameron Nov 2 '14 at 18:02
  • \$\begingroup\$ Personally I find that you need to test which methods work well and which ones don't. Array.addAll (CF Array = Java Vector = Java List which has addAll(Collection), and List = Collection) is one of those things. You can iterate over an array and copy the elements to the other array, but simply calling addAll is so much faster. I don't work with ColdFusion any more so I can't give you the numbers, but there are cases where dropping down to Java makes things easier AND faster. \$\endgroup\$ – Pimgd Nov 2 '14 at 22:59
1
\$\begingroup\$

I couldn't get your code to work as is, but I believe I understand the inputs and outputs and this will be an improvement. Its about 60 fewer lines and should be more maintainable. The biggest improvement is using a regular expression instead of the complicated punctuation and boundary testing.

This does not handle your stopString, although that could be easily added I believe (im not 100% sure what you're doing with it). See how well this works for you though:

function linkifyNames(required struct names, required string template, required string text) {

    for (var key in names){
        var name = names[key];

        //this could be cleaned up more if you used the actual variable names from the struct with the hungarian notation
        //or if you dropped the hungarian notation
        name.expandedLink = '<a href="' & template & '" title="more information on this expert from example.com" target="_blank">' & name.sFullname & '</a>';
        name.expandedLink = replaceNoCase(name.expandedLink, "[id]", name.nID);
        name.expandedLink = replaceNoCase(name.expandedLink, "[dupe]", (name.sIsDupe ? 1 : 0));
        name.expandedLink = replaceNoCase(name.expandedLink, "[lastname]", name.sLastName);
        name.expandedLink = replaceNoCase(name.expandedLink, "[seo]", name.sFullName);

        text = reReplaceNoCase(text, "\b" & name.sFullname & "\b", name.expandedLink, "all");//needs to be a regex
    }

    return text;
}


variables.stPartners      = {'123'={sFullName='Michael Fish',nID='133',sLastName='Fish',sIsDupe='No'},'333'={sFullName='Michael Fisher',nID='133',sLastName='Fisher',sIsDupe='No'}};

writedump(linkifyNames(variables.stPartners, '//example.com/profiles/[id]/[dupe]/[lastname]/[seo]/', '<p>Michael Fish is a name</p>'));
writedump(linkifyNames(variables.stPartners, '//example.com/profiles/[id]/[dupe]/[lastname]/[seo]/', '<p>Michael Fi is a name</p>'));
\$\endgroup\$
  • \$\begingroup\$ This isn't so much a review, could you explain what is being done here and why you think it might be better than the OP's post? \$\endgroup\$ – Malachi Oct 31 '14 at 16:01
  • \$\begingroup\$ I'll admit it isn't as much of a review as an alternative way to achieve the same thing which is smaller and cleaner. The biggest improvement was to use a regular expression instead of the punctuation checks. \$\endgroup\$ – Ryan Guill Oct 31 '14 at 16:17
  • \$\begingroup\$ I believe I did. Is there something specific more that you would like me to say? \$\endgroup\$ – Ryan Guill Oct 31 '14 at 16:23

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.