Sunday 13 June 2021

CFML: messing around with mixins (part 3)

G'day:

In the preceding two articles in this series (1 and 2) I looked at how to implement mixins in CFML, both at compile time and at runtime, with an increasing amount of functionality / complexity. My closing conclusion was that the runtime approach I was taking was a wee bit too magic and opaque to be something one should do; but at least it's interesting to look at the issues & techniques involved. Today I'm going to try to come up with a less opaque solution.

Before I go any further I will repeat what I've said in the previous two articles: I would never actually recommend using mixins at all. They just seem like a hack to me: an anti-pattern and poor design. As a concept they're just either a poor-person's inheritance, or a poor-person's dependency-injection technique. Just use the real things.

What do I mean by my previous work was a bit opaque? I'll repeat the example from the last article.

component {

    function getObjects(orderBy="id") {
        return getObjectsFromStorage(orderBy)
    }
}

Externally, I am mixing in that getObjectsFromStorage method at runtime. But how does the person looking at that code in isolation know this? They don't. They need to just already know that there might (or might not!) be some external shenanigans going on to avail that method at runtime. This is rubbish. The code in any given class should be self-contained, and be capable of being executed provided one fulfils the contract the code specifies. One should not need to know what context it's being used in to understand how it works: that's an anti-pattern. Contrast with this:

component {

    function init(required Repository repository) {
        variables.repository = repository
    }

    function getObjects(orderBy="id") {
        return variables.repository.getObjectsFromStorage(orderBy)
    }
}

Someone looking at that code is not surprised at all: they can tell what's going on. One should never write code that seems like the former example; go for the latter. Don't. Put. Surprises. In. Your. Code.

Today I'm gonna try to effect some hybrid of using clear dependency injection to avail my object of external resources, but then using my earlier mixing-in strategy to expose them to the code within the class as "local" functions. So basically still being able to use getObjectsFromStorage rather than qualify it with variables.repository.getObjectsFromStorage.

But… but … why, Adam?

Good question. Note I would never ever do any of this in my own code. This exercise cropped up from a conversation I was having about some framework code, and how it seems to revel in having an impenetrable (and pretty naive) implementation. It just injects methods all over the place - with no consideration as to whether they belong in the object they are being injected into, I might add - and it's all held together with some level-one-quality magic. I do not mean that in a good way, in case you can't tell. It's currently being overhauled a bit for its next version, and I'm going through this exercise to see if I can offer some suggestions as to how to make it seem less magical, and more coherent. This is part of that.

Did I mention I would never ever ever do any of this sort of thing in my own code? Good. Glad we cleared that one up.

The "interesting" thing about this article is that I have not got a single line of code yet - I mean as I type this sentence - and only a vague idea of how I'm going to solve this (and I'll be copy and pasting code from the previous article). I have some goals in place though, by way of the test cases from the previous articles. I've cannibalised those to a subset of cases I need to address for this exercise.

So that's a place to start. The cases might change as I go, but I'll start with the first one and work from there.

It tests unmodified setup

As some context, for this first case we are going to already assume some existing code. This part is not breaking TDD, but to wire things into objects, I need to have those objects' classes first. So I've got this lot:

//MyModel.cfc
import cfmlInDocker.miscellaneous.mixins.runtime.advanced.MyRepository

component {

    public MyModel function init(required MyRepository repository) {
        variables.repository = repository
        
        return this
    }

    public Colour[] function getObjectsViaInjectedRepository() {
        return variables.repository.getAllObjects()
    }
}
// MyRepository.cfc
component {

    public MyRepository function init(MyDao dao) {
        variables.dao = dao
        
        return this
    }

    public Colour[] function getAllObjects() {
        return dao.getRecords("id", -1).reduce((array=[], record) => array.append(new Colour(record.mi)))
    }
}
//MyDao.cfc
component {

    public query function getRecords(required string orderBy, required numeric maxrows) {
        records = queryNew("id,en,mi", "integer,varchar,varchar", [
            [1,"red","whero"],
            [2,"green","kakariki"],
            [3,"blue","kikorangi"]
        ])
        return queryExecute(
            "SELECT * FROM records ORDER BY #orderBy#",
            {},
            {dbtype="query", maxrows=maxrows}
        )
    }
}

And I couldn't help but pop a test in for this as I was tweaking some of the stuff I was porting over from the previous article's examples:

//FacadeTest.cfc

import testbox.system.BaseSpec
import cfmlInDocker.miscellaneous.mixins.runtime.facade.*

component extends=BaseSpec {

    function run() {
        describe("Testing facade proof of concept", () => {
            it("tests unmodified setup", () => {
                model = new MyModel(new MyRepository(new MyDao()))

                results = model.getObjectsViaInjectedRepository()

                expect(results).toBe([
                    new Colour("whero"),
                    new Colour("kakariki"),
                    new Colour("kikorangi")
                ])
            })
        })
    }
}

Basically we have a model representing Colour collections, where the colours are the Maori terms for various colours. All somewhat contrived, I know.

It creates facades for methods

From a TDD perspective, my intended approach here "interests" me. I already know the end result I want, and I pretty much already have the code I need to use. I'll just be using it in a different way from before. My inclination here is to write (read: copy and paste) a lot of code first up to get the mechanics in place to do any of the mixing-in, and at that point each case is just reimplementing each step from the last article as I support more and more features in the mixinMap. Writing a lot of code for a single case is a smell to me. But is this new code? Is it kind of a refactoring instead, so I just need to make sure my existing cases are all still accounted for? I think I'm doing TDD wrong here, but will will just crack on with it, and pay attention to what I'm doing in case I can work out a better strategy as I go. And as always I'm open to advice as to how I should have been doing something.

Cracking on with it, here's my test:

it("creates facades for methods", () => {
    model = new MyModel(new MyRepository(new MyDao()), new FacadeMapper())

    results = model.getObjectsViaFacade()

    expect(results).toBe([
        new Colour("whero"),
        new Colour("kakariki"),
        new Colour("kikorangi")
    ])
})

I'm passing-in an optional dependency here: a FacadeMapper. I'm also calling a new method I've added: MyModel.getObjectsViaFacade: each test is going to need a method to call to test the case in question).

FacadeMapper's functionality is lifted largely from DependencyInjectionImplementor in the previous article:

component {

    public void function setFacadesFromMap(required Component target, required Component source, required struct map) {
        target.__getVariables = getVariables
        scopes = {
            public = target,
            private = target.__getVariables()
        }
        structDelete(target, "__getVariables")

        map.each((sourceMethod, mapping) => {
            targetMethod = mapping.keyExists("target") ? mapping.target : sourceMethod

            requestedAccess = mapping.keyExists("access") ? mapping.access : "private"
            targetAccess = requestedAccess == "public" ? "public" : "private"

            bind = mapping.keyExists("bind") && isBoolean(mapping.bind) ? mapping.bind : true

            proxy = bind
                ? () => source[sourceMethod](argumentCollection=arguments)
                : source[sourceMethod]

            scopes[targetAccess][targetMethod] = proxy
        })
    }

    private struct function getVariables(){
        return variables
    }
}

I explain it all throughly there, so go have a read. But basically it grabs references to a source object's methods, wraps them in a proxy, and injects them into the target.

And the relevant new code in MyModel to call this is here:

component {

    public MyModel function init(required MyRepository repository, Component facadeMapper) {
        variables.repository = repository

        if (arguments.keyExists("facadeMapper")) {
            mapFacades(facadeMapper)
        }

        return this
    }

    private void function mapFacades(facadeMapper) {
        facadeMapper.setFacadesFromMap(
            this,
            variables.repository,
            {
                "getAllObjects" = {}
            }
        )
    }
    
    // ...    
}

Unlike in the previous article's approach, where the wiring of the mixins was done completely outside of the target object, so no-one would ever have any idea what's going on with the code and why it magically works; here it's explicit. We list the methods we are creating facades for (only one just for now). So when we come to use the facaded (?is that a word?) method, we can tell where it came from:

public Colour[] function getObjectsViaFacade() {
    return getAllObjects()
}

It remaps function names if the map specifies it

It handles public/private method access on the mixed-in function

It ignores access modifiers other than public / private (falling back to private)

It doesn't bind facade function to its original context if bind option is false

I'm gonna cover the cases for these in one fell swoop because it's all repetition from the previous article anyhow. This is why I'm kinda considering this more of a refactoring exercise than the red/green part of a TDD exercise. The tests are thus:

it("remaps function names if the map specifies it", () => {
    model = new MyModel(new MyRepository(new MyDao()), new FacadeMapper())

    results = model.getSomeObjectsViaFacade(rows=2)

    expect(results).toBe([
        new Colour("whero"),
        new Colour("kakariki")
    ])
})

it("handles public/private method access on the mixed-in function", () => {
    model = new MyModel(new MyRepository(new MyDao()), new FacadeMapper())

    results = model.getOrderedObjects(orderBy="English")

    expect(results).toBe([
        new Colour("kikorangi"),
        new Colour("kakariki"),
        new Colour("whero")
    ])
})

it("ignores access modifiers other than public / private (falling back to private)" , () => {
    model = new MyModel(new MyRepository(new MyDao()), new FacadeMapper())

    results = model.getEnglishObjectsViaFacade()

    expect(results).toBe([
        new Colour("red"),
        new Colour("green"),
        new Colour("blue")
    ])

    expect(() => model.getEnglishObjects()).toThrow(type="Expression", regex="^.*MyModel.*has no\s+function with name.*getEnglishObjects.*$")
})

it("doesn't bind facade function to its original context if bind option is false", () => {
    model = new MyModel(new MyRepository(new MyDao()), new FacadeMapper())
    modelVariables = model.getVariables()

    expect(getMetadata(modelVariables.this)).toBe(getMetadata(model))
})

The mapping made to facilitate these tests has grown:

private void function mapFacades(facadeMapper) {
    facadeMapper.setFacadesFromMap(
        this,
        variables.repository,
        {
            "getAllObjects" = {},
            "getSomeObjects" = {"target" = "getSubsetOfObjects"},
            "getOrderedObjects" = {"access" = "public"},
            "getEnglishObjects" = {"access" = "package"}
        }
    )
    facadeMapper.setFacadesFromMap(
        this,
        new VariablesAccessor(),
        {
            "getVariables" = {"access" = "public", "bind" = false}
        }
    )
}

But all this was covered in depth in the previous article as well. VariablesAccessor is just this:

component {

    public struct function getVariables(){
        return variables
    }
}

This is used in the test case that doesn't bind to the original source's context, instead letting it bind to the target's, so it acts on the target's context (we can use it to access the target's variables scope). Just… look at the relevant test case (such is the beautify of BDD-oriented test cases, they kinda document what's going on in and of themselves.

And in MyModel I've need to add some more methods for each test to call to access the relevant facade-mapping variation. There's no need to pore over this code, it's boilterplate for the tests.

public Colour[] function getSomeObjectsViaFacade(string rows="all") {
    return getSubsetOfObjects(rows)
}

public Colour[] function getEnglishObjectsViaFacade() {
    return getEnglishObjects()
}

And in the repository:

public Colour[] function getSomeObjects(string rows="all") {
    rows = (isValid("integer", rows) && rows > -1) ? rows : -1

    return dao.getRecords("id", rows).reduce((array=[], record) => array.append(new Colour(record.mi)))
}

public Colour[] function getOrderedObjects(string orderby="id") {
    orderBy = mapOrderBy(orderBy)

    return dao.getRecords(orderBy, -1).reduce((array=[], record) => array.append(new Colour(record.mi)))
}

public Colour[] function getEnglishObjects() {
    return dao.getRecords("id", -1).reduce((array=[], record) => array.append(new Colour(record.en)))
}

private string function mapOrderBy(required string property) {
    propertyColumnMap = {
        "id" = "id",
        "english" = "en",
        "maori" = "mi"
    }
    return propertyColumnMap.keyExists(property) ? propertyColumnMap[property] : "id"
}

The DAO already had all the necessary functionality baked in: there were no changes to that.


That might seem like a lot of code changes that I glossed-over there, but the objective of this article is all around the explicit use of that mapping in the class receiving the facades, and how that's clearer than magic mixins.

I'll also re-re-iterate that I think doing this sort of thing is rubbish. It's a lot of effort to provide non-name-spaced functions, and all yer gaining is a lack of clarity, and a few keystrokes. It's just a daft thing to do. But it was an interesting progression of exercises for me, anyhow. And keeps me off the streets on a Sunday (it' too hot to go outdoors, that said).

Righto.

--
Adam