Showing posts with label James Mohler. Show all posts
Showing posts with label James Mohler. Show all posts

Saturday, 5 March 2016

CFML: go have a look at a Code Review

I've already put more effort into this than a Saturday morning warrants, so I'll keep this short and an exercise in copy and past.

Perennial CFMLer James Mohler has posted some code on Code Review:  "Filtering the attributes for a custom tag". I'll just reproduce my bit of the review, with his code by way of framing.

Go have a look, and assess our code as an exercise.

James's functions:


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;

His reworking:

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

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

    return local.result;

Now for my code review (this is a straight copy and paste from my answer):

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);
            it("works with an irrelevant attribute", function(){
                var testStruct = {notRelevant=3};
                var resultFromOriginal = passThrough(testStruct);
                var resultFromRefactored = extractCodeCentricAttributesToMarkupSafeAttributes(testStruct);
            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);
            it("works with a mix of attribute relevance", function(){
                var testStruct = {notRelevant=7, onRelevant=11};
                var resultFromOriginal = passThrough(testStruct);
                var resultFromRefactored = extractCodeCentricAttributesToMarkupSafeAttributes(testStruct);
            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);



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

One thing I didn't put as I don't think it's relevant to this code review is that one should be careful with inline function expressions. They're not themselves testable, so if they get more than half a dozen statements or have any branching or conditional logic: extract them into their own functions, and test them separately.

Oh, and in case yer wondering... yes I did write the tests before I wrote any of my own code. And I found a coupla bugs with my planned solution (and assumptions about the requirement) in doing so. Always TDD. Always.

Anyway, all of this is getting in the way of my Saturday.



Saturday, 16 January 2016

Missing/undecided chapters in this CFML book (and other questions)

So with me "abandoning" that CFML book I was writing (Abandonware: "Learn CFML in 24h" notes), I've now got rekindled interest in it. I'm nothing if not contrary.

I've read over what I've done so far and I'm reasonably happy with it as a first draft / general direction sort of thing, but it's also raised some questions.

If you haven't looked, it's currently got these chapter headings (only a few have content: * indicates a first draft of the whole chapter is there; ** means I've started it, but it's incomplete):

  1. Housekeeping (*)
  2. Commands/statements/expressions, variables, operators (*)
  3. Flow control structures (*)
  4. Built-in types (*)
  5. OO with CFML (*)
  6. OO with CFML cont'ed
  7. Error handling (*)
  8. TDD
  9. Clean code
  10. The CFML application server, the request/response process & scopes (**)
  11. Application framework
  12. Basic database persistence with ORM
  13. MVC basics
  14. View logic with CFML tags
  15. Custom Tags
  16. Advanced OO
  17. Advanced DB
  18. Advanced ORM
  19. File system / HTTP / FTP
  20. REST
  21. Functional programming concepts
  22. Threads and locking
  23. Java interaction
  24. TBC
  25. TBC
Reviewing it today I have a few questions.
  1. Are the chapter breaks for 0-22 in a sensible order? Does anything stand out as being out of place? It's supposed to progress from chapter to chapter, with a subsequent chapter pretty much requiring the framing of one or more of the earlier chapters for it to make sense in context.
  2. What should I do for chapters 23-24? I am actively omitting a lot of CFML (like <cfclient> and anything referenced in ColdFusion UI the Right Way). None of that stuff should be in CFML and I'm damned if I will ever encourage anyone to ever use it. Or even make them aware of it. When thinking about this, bear in mind this thing is about CFML (the language), it's not about ColdFusion (the application server). So I'm not interested in stuff that's mostly related to the ColdFusion server (admin, security etc). There's some stuff CFML facilitates like all the PDF manipulation and spreadsheet stuff which is "significant" to CFML, but don't in themselves represent anything "different" in the way one uses CFML. Once one knows how to use functions and tags, one knows how to use said functionality to do PDF stuff or spreadsheet stuff. There's no need to go over it again.
  3. MVC. Nolan did an excellent presentation at CFSummit about MVC without a framework, which - coincidentally - was where I was gonna start with this, but the more I think about it... the more I think it's just encouraging bad practice to take that approach. If one is doing MVC, one should use a framework. I guess the first bit could be a discussion on the MVC pattern, but I don't want to be writing code that demonstrates a sub-par approach to things. Equally I don't want to regurgitate stuff that I'm sure FW/1 / ColdBox etc already covers much better. What should the narrative approach be here? Sean and Brad, I'd be particularly interested in what you think on this one?
  4. It irks me that in chapter 0 I encourage readers to save CFM files into a web-browseable directory. That is pretty poor practice, although it's kinda necessary for running small scripts like one will end up with in this book. I don't really want to get into "web sites" too much, but I guess it's innate to CFML usage, and this made me think I ought to mention "do not put your CFML files in a web browseable directory" somewhere. But there's not much to say on that (other than explain why, and what one should do instead). Is there scope for a full "other best practices" chapter? I will already cover most of what I want to say on that in the TDD (might undergo a title change, based on some stuff we're doing on our project now), and Clean Code chapters, but there are perhaps some CFML-specific best practices that ought to be written down?
  5. Alex was dismayed I changed the slant from Lucee-centric to ColdFusion-centric. I couldn't write it completely Lucee-centric because, well: almost everyone using CFML doesn't use Lucee. And the more I wrote code that needed to run on both, the more Lucee f***ed me off for not being CFML-compliant (their prerogative, and I used to be all for it, but I've changed my mind there). This was more in the CFScript docs I wrote, but it was also impacting me here. But Alex makes a good point in that there's benefit to include Lucee's CFML in all this too. So I thought an appendix? What do you think?
  6. Anything else I've obviously missed from all this? I've had some pretty CFML-savvy eyes over this thing already, but more eyes would be better.
I have to say this still conceptually interests me quite a lot as an exercise. I'm just struggling with motivation to write it. Plus I also need to hunker down and just work out which next bite of the elephant to chow down on.

And... hang on... am not I a PHP developer these days? :-S

Oh, and a special shout out to James Mohler who's dived in and trying his best to help with this thing. And even more so for enduring my idiosyncractic pull request requirements. Cheers fella.