Testing: reasoning over "testing implementation detail" vs "testing features"

G'day:

I can't see how this article will be a very long one, as it only really dwells on one concept I had to reason through before I decided I wasn't talking horseshit.

I'm helping some community members with improving their undertanding of testing, and I was reviewing some code that was in need of some test coverage. The code below is not the actual code - for obvious "plausible deniability" reasons - but it's pretty much the same situation and logic flow we were looking at, just with different business entities and the basis for the rule has been "uncomplicated" for the sake of this article:

function validateEmail() {
    if ((isInstanceOf(this.orderItems, "ServicesCollection") || isInstanceOf(this.orderItems, "ProductsCollection")) && !isValid("email", this?.email) ) {
        var hasItemWithCost = this.orderItems.some((item) => item.cost > 0)
        if (hasItemWithCost) { // (*)
            this.addError("Valid email is required")
        }
    }
}

// (*) see AdamT's comment below. I was erroneously calling .len() on hasItemWithCost due to me not having an operational brain. Fixed.

This function is called as part of an object's validation before it's processed. It's also slightly contrived, and the only real bit is that it's a validation function. For this example the business rule is that if any items in the order have a cost associated with them; then the order also needs an email address. Say the receipt needs to be emailed or something. The real world code was different, but this is a fairly common-knowledge sort of parallel to the situation.

All the rest of the code contributing to the data being validated already existed, we're just got this new validation rule around the email not always being optional now.

We're not doing TDD here (yet!), so it's a matter of backfilling tests. The exercise was to identify what test need to be written.

We picked two obvious ones:

• make an orderItems collection with no costs in it, and the email shouldn't be checked;
• make an orderItems collection with at least one oneitem having a costs in it, and the email should be present and valid;

But then I figured that we've skipped a big chunk of the conditional logic: we're ignoring the check of whether the collection is one of a ServicesCollection or a ProductsCollection. NB: unfortunately the order can comprise collections of two disparate types, and there's no common interface we can check against instead. There's possibly some tech debt there.

I'm thinking we have three more cases here:

• the collection is a ServicesCollection;
• the collection is a ProductsCollection;
• the collection is neither of those, somehow. This seems unlikely, but it seems it could be true.

Then I stopped and thought about whether by dissecting that if statement so closely, I am just chasing 100% line-of-code coverage. And worse: aren't the specifics of that part of the statement just implementation detail, rather than part of the actual requirement? The feature was "update the order system so that orders requiring a financial transaction require a valid email address". It doesn't say anything about what sort of collections we use to store the order.

Initially I concluded it was just implementation detail and there was no (test) case to answer to here. But it stuck in my craw that we were not testing that part of the conditional logic. I chalked that up to me being pedantic about making sure every line of code has coverage. Remember I generally use TDD when writing my code, so it's just weird to me to have conditional code with no specific tests, because the condition would only ever be written because a feature called for it.

But it kept gnawing at me.

…

Then the penny dropped (this was a few hours later, I have to admit).

It's not just testing implementation detail. The issue / confusion has arisen because we're not doing TDD.

If I rewind to when we were writing the function, and take a TDD approach, I'd be going "right, what's the first test case here?". The first test case - the reason why want to type in isInstanceOf(this.orderItems, "ServicesCollection") - is because part of the stated requirement is implicit. When the client said "if any items in the order have a cost associated with them…" they were not explicit about "this applies to both orders for services as well as orders for products", because in the business's vernacular the distinction between types of orders isn't one that's generally considered. For a lot of requirements there's just the concept of "orders" (this is why there should be an interface for orders!). However the fully-stated requirement could be made more accurately "if any items in an order for services have a cost associated with them…", followed by "likewise, if any items in an order for products have a cost associated with them…". It's this business information that inform our test cases, which would be better enumerated as:

• it requires an email address if any items in a services order have an associated cost;
• it requires an email address if any items in a products order have an associated cost;
• it does not require an email address if the order is empty;

I had to chase up what the story was if the collection was neither of those types, and it turns out there wasn't a third type that was immune to the email address requirement, it's just - as per the nice clear test case! - the order might be empty, but the object as a whole still needs validation.

There is still "implementation detail" here - the code checking the collection type is obviously the implementation of that part of the business requirement: all code is implementation detail after all. The muddiness here arose because we were backfilling tests, rather than using TDD. Had we used TDD I'd not have been questioning myself when I was deciding on my test cases. The test cases are driven by business requirements; they implementation is driven by the test cases. And, indeed, the test implementation has to busy itself with implementation detail too, because to implement those tests we need to pass-in collections of the type the function is checking for.

---

As an aside, I was not happy with how busy that if condition was. It seemed to be doing too much to me, and mixing a coupla different things: type checks and a validity check. I re-reasoned the situation and concluded that the validation function has absolutely nothing to do if the email is there and is valid: this passes all cases right from the outset. So I will be recommending we separate that out into its own initial guard clause:

function validateEmail() {
    if (isValid("email", this.email)) {
        return
    }
    if ((isInstanceOf(this.orderItems, "ServicesCollection") || isInstanceOf(this.orderItems, "ProductsCollection"))) {
        var hasItemsWithCost = this.orderItems.some((item) => item.cost > 0)
        if (hasItemsWithCost.len()) {
            this.addError("Valid email is required")
        }
    }
}

I think this refactor makes things a bit easier to reason through what's going on. And note that this is a refactor, so we'll do it after we've got the tests in place and green.

I now wonder if this is something I should be looking out for more often? Is a compound if condition that is evaluating "asymmetrical" sub-conditions indicative of a code smell? Hrm… will need to keep an eye on this notion.

Now if only I could get that OrderCollection interface created, the code would be tidier-still, and closer to the business requirements.

Righto.

--
Adam

 

PS: I actually doubted myself again whilst writing this, but by the time I had written it all out, I'm happy that the line of thinking and the test cases are legit.

Test coverage: it's not about lines of code

G'day

A coupla days ago someone shared this Twitter status with me:

I'll just repeat the text here for google & copy-n-paste-ability:

My personal opinion: Having 100% test coverage doesn't make your code more effective. Tests should add value but at some point being overly redundant and testing absolutely every line of code is ineffective. Ex) Testing that a component renders doesn't IN MY OPINION add value.

Emma's position here is spot on, IMO. There were also a lot of "interesting" replies to it. Quelle surprise. Go read some of them, but then give up when they get too repetitive / unhelpful.

In a similar - but slightly contrary - context, I was reminded of my erstwhile article on this topic: "Yeah, you do want 100% test coverage".

But hang on: on one hand I say Emma is spot on. On the other hand I cross-post an old article that seems to contradict this. What gives?

The point of differentiation is "testing absolutely every line of code" vs "100% test coverage". Emma is talking about lines of code. I'm talking about test cases.

Don't worry about testing lines of code. That's intrinsically testing "implementation". However do care about your test cases: the variations that define the feature you are working on. you've been asked to develop a feature and its variations, and you don't know you've delivered the feature (or in the case of automated testing: continue to having been delivered in a working state) unless you test it.

Now, yeah, sure: features are made of lines of code, but don't worry about those. A trite example I posited to a mate yesterday is this: consider this to be the implementation of your feature:

doMyFeature() {
    // line 1
    // line 2
    // line 3
    // line 4
    // line 5
    // line 6
    // line 7
    // line 8
    // line 9
    // line 10
}

I challenge you to write a test for "My Feature", and not by implictation happen to test all those lines of code. But it's the feature we care about, not the lines of code.

On the other hand, let's consider a variant:

doMyFeature() {
    // line 1
    // line 2
    // line 3
    // line 4
    // line 5
    if (someVariantOfMyFeature) {
        // line 6
    }
    // line 7
    // line 8
    // line 9
    // line 10
}

If you run your test coverage analysis and see that line 6 ain't tested, this is not a case of wringing one's hands about line 6 of the code not being tested; it's that yer not testing the variation of the feature. It's the feature coverage that's missing. Not the lines-of-code coverage.

Intrinsically your code coverage analysis tooling probably marks individual lines of code as green or red or whatever, but only if you look that closely. If you zoom out a bit, you'll see that the method the lines of code are in is either green or orange or red; and out further the class is likewise green / orange / red, and probably says something like "76% coverage". The tooling necessarily needs to work in lines-of-code because its a dumb machine, and those are the units it can work on. You are the programmer don't need to focus on the lines-of-code. You have a brain, and what the report is saying is "you've not tested part of your feature", and it's saying "the bit of the feature that is represented by these lines of code". That bit. You're not testing it. Go test yer feature properly.

There's parts of the implementation of a feature I won't test maybe. Your DAOs and your other adapters for external services? Maybe not something I'd test during the TDD cycle of things, but it might still be prudent to chuck in an integration test for those. I mean they do need to work and continue to work. But I see this as possibly outside of feature testing. Maybe? Is it?

Also - now I won't repeat too much of that other article - there's a difference between "coverage" and "actually tested". Responsible devs can mark some code as "won't test" (eg in PHPUnit with a @codeCoverageIgnore), and if the reasons are legit then they're "covered" there.

Why I'm writing this fairly short article when it's kinda treading on ground I've already covered is because of some of the comments I saw in reply to Emma. Some devs are a bit shit, and a bit lazy, and they will see what Emma has written, and their take away will be "basically I don't need to test x because x is some lines of code, and we should not be focusing on lines of code for testing". That sounds daft, but I know people that have rationalised their way out of testing perfectly testable (and test-necessary) code due to "oh we should not test implementation details", or "you're just focusing on lines of code, that's wrong", and that sort of shit. Sorry pal, but a feature necessarily has an implementation, and that's made out of lines of code, so you will need to address that implementation in yer testing, which will innately report that those lines of code are "covered".

Also this notion of "100% coverage is too high, so maybe 80% is fine" (possibly using the Pareto Pinciple, poss just pulling a number of out their arses). Serious question: which 80%? If you set that goal, the a lot of devs will go "aah, it's in that 20% we don't test". Or they'll test the easy 80%, and leave the hard 20% - the bit that needs testing - to be the 20% they don't test (as I said in the other article: cos they basically can't be arsed).

Sorry to be a bit of a downer when it comes to devs' level of responsibility / professionalism / what-have-you. There are stack of devs out there who rock and just "get" what Emma (et al) is saying, and crack on with writing excellently-designed and well-tested features. But they probably didn't need Emma's messaging in the first place. I will just always think some more about advice like this, and wonder how it can be interpreted, and then be a contrarian and say "well actually…" (cringe, OK I hope I'm not actually doing that here?), and offer a different informed observation.

So my position remains: set your sights on 100% feature coverage. Use TDD, and your code will be better and leaner and you'll also likely end up with 100% LOC coverage too (but that doesn't matter; it's just a test-implementation detail ;-).

Righto.

--
Adam

CFML: static methods and properties

G'day:

Context

In Lucee 5.0 (so: ages ago) and ColdFusion 2021 (so: you know… just now), support for static properties and methods was added to CFML. This isn't a feature that you'd use very often, but it's essential to at least know about it. And it's bloody handy, and makes your code clearer sometimes. I had reason to be googling for docs / examples today, and what's out there ain't great so I thought I'd write my own "probably still not great" effort.

OK so that's the bar set suitably low. Let's get on with it.

What?

What are static properties and methods. I'm going to start from the other end of things. What are general properties and methods? They're properties and methods of an object; you know, one of those things one gets as the result of calling new MyComponent() (or from a createObject call if yer old skool). An object is a set of properties and methods that maintain a current state: values specific to that object.

son = new Person("Zachary")
dad = new Person("Adam")

We have two instances of the Person CFC: one for Zachary and one for me. The behaviour - ie: methods - of these Person objects is defined in Person.cfc, but when one calls an object method on a given object, it acts on the state of that specific object. So if one was to call son.getName(), one would get "Zachary"; if one was to call dad.getName(), one would get "Adam". The implementation of getName is the same for both son and dad - as defined by Person - but their implementation acts on the values associated with the object ("Zachary" and "Adam" respectively). You know all this stuff already, but it's just to contextualise things.

So what these object-based properties and methods are to an object; static properties and methods are to the class (the CFC itself). To get that, one has to have clear in one's mind that objects - instances of a class - are not the same thing as the class. Also it's important to understand that a CFC is not simply a file of source code: it still gets loaded into memory before any objects are made, and it is still a data structure in its own right, and it too can have its own properties, and methods to act on them.

We saw above examples of calling methods on an object. We can also call methods on the class itself, without needing an object. EG:

dog = Animal::createFromSpecies("canis familiaris")

Here we have an Animal class - and that is a reference to Animal.cfc, not an object created from Animal.cfc - and it has a factory method which we use to return a Dog object. Internally to createFromSpecies there might be some sort of look-up table that saying "if they ask for a 'canis familiaris', return a new Dog object"; the implementation detail doesn't matter (and I'll get to that), the important bit is that we are calling the method directly on the Animal class, not on an object. We an also reference static properties - properties that relate to the class - via the same :: syntax. I'll show a decent example of that shortly.

How?

Here's a completely contrived class that shows static syntax:

// Behaviour.cfc
component {

    static {
        static.defaultMyVarValue = "set in static constructor"
        static.myVar = static.defaultMyVarValue
    }

    static.myVar = "set in pseudo-constructor"

    public static function resetMyVar() {
        static.myVar = static.defaultMyVarValue
    }
}

There's a coupla relevant bits here.

The static constructor. This is to the class what the init method is to an object. When the class is first loaded, that static constructor is executed. It can only reference other static elements of the class. Obviously it can not access object properties or object methods, because when the static constructor is executed, it's executed on the class itself: there are no objects in play. the syntax of this looks a bit weird, and I don't know why this was picked instead of having:

public static function staticInit() {
    static.defaultMyVarValue = "set in static constructor"
    static.myVar = static.defaultMyVarValue
}

That's more clear I think, and doesn't require any new syntax. Oh well.

Note that when the class is first initialised the pseudo-constructor part of the CFC is not executed. That is only executed when a new object is created from the class.

A static method can only act on other static elements of the class, same as the static constructor, and for the same reason.

I demonstrate the behaviour of this code in its tests:

import testbox.system.BaseSpec
import cfmlInDocker.miscellaneous.static.Behaviour

component extends=BaseSpec {

    function run() {

        describe("Tests for Behaviour class", () => {
            beforeEach(() => {
                Behaviour::resetMyVar()
            })

            it("resets the test variable when resetMyVar is called", () => {
                behaviour = new Behaviour()
                expect(behaviour.myVar).toBe("set in pseudo-constructor")
                expect(Behaviour::myVar).toBe("set in pseudo-constructor")
                Behaviour::resetMyVar()
                expect(behaviour.myVar).toBe("set in static constructor")
                expect(Behaviour::myVar).toBe("set in static constructor")
            })

            it("doesn't use the pseudo-constructor for static values using class reference", () => {
                expect(Behaviour::myVar).toBe("set in static constructor")
            })

            it("does use the pseudo-constructor for static values using object reference", () => {
                behaviour = new Behaviour()
                expect(behaviour.myVar).toBe("set in pseudo-constructor")
            })
        })
    }
}

I need that resetMyVar method there just to make the testing easier. One thing to consider with static properties is that they belong to the class, and that class persists for the life of the JVM, so I want to make the initial state of the class for my tests the same each time. It's important to fully understand that when one sets a static property on a class, that property value will be there for all usages of that class property for the life of the JVM. So it persists across requests, sessions and even the lifetime of the application itself.

Why?

Properties

That Behaviour.cfc example was rubbish. It gives one no sense of why on might want to use static properties or methods. Here's a real world usage of static properties. I have a very cut down implementation of a Response class; like one might have in an MVC framework to return the value from a controller that represents what needs to be sent back to the client agent.

// Response.cfc
component {

    static {
        static.HTTP_OK = 200
        static.HTTP_NOT_FOUND = 404
    }

    public function init(required string content, numeric status=Response::HTTP_OK) {
        this.content = arguments.content
        this.status = arguments.status
    }

    public static function createFromStruct(required struct values) {
        return new Response(values.content, values.status)
    }
}

Here we are using static properties to expose some labelled values that calling code can use to create their response objects. One of its tests demonstrates this:

it("creates a 404 response", () => {
    testContent = "/bogus/path was not found"
    response = new Response(testContent, Response::HTTP_NOT_FOUND)

    expect(response.status).toBe(Response::HTTP_NOT_FOUND)
    expect(response.content).toBe(testContent)
})

Why not just use the literal 404 here? Well for common HTTP status codes like 200 and 404, I think they have ubiquity we could probably get away with. But what about a FORBIDDEN response. What status code is that? 403? Or is it 401? I can never remember, can you? So what wouldn't this be more clear, in the code:

return new Response("nuh-uh, sunshine", Response::HTTP_FORBIDDEN)

I think that's fair enough. But OK, why are they static? Why not just use this.HTTP_FORBIDDEN? Simply to show intended usage. Do HTTP status codes vary from object to object? Would one Response object have a HTTP_BAD_GATEWAY of 502, but another one having a HTTP_BAD_GATEWAY value of 702? No. These properties are not part of an object's state, which is what's implied by using the this scope (or variables scope). They are specific to the Response class; to the concept of what it is to be a Response.

Methods

That Response.cfc has a static method createFromStruct which I was going to use as an example here:

it("returns a Response object with expected values", () => {
    testValues = {
        content = "couldn't find that",
        status = Response::HTTP_NOT_FOUND
    }
    response = Response::createFromStruct(testValues)

    expect(response.status).toBe(testValues.status)
    expect(response.content).toBe(testValues.content)
})

But it occurred to me after I wrote it that in CFML one would not use this strategy: one would simply use response = new Response(argumentCollection=testValues). So I have a request class instead:

// Request.cfc
component {

    public function init(url, form) {
        this.url = arguments.url
        this.form = arguments.form
    }

    public static Request function createFromScopes() {
        return new Request(url, form)
    }
}

This is the other end of the analogy I started with an MVC controller. This is a trimmed down implementation of a Request object that one would pass to one's controller method; encapsulating all the elements of the request that was made. Here I'm only using URL and form values, but a real implementation would also include cookies, headers, CGI values etc too. All controller methods deal in the currency of "requests", so makes sense to pass a Request object into them.

Here we have a basic constructor that takes each property individually. As a rule of thumb, my default constructor always just takes individual values for each property an object might have. And the constructor implementation just assigns those values to the properties, and that's it. Any "special" way of constructing an object (like the previous example where's there's not discrete values, but one struct containing everything), I use a separate method. In this case we have a separate "factory" method that instead of taking values, just "knows" that most of the time our request will comprise the actual URL scope, and the actual form scope. So it takes those accordingly.

That's all fairly obvious, but why is it static? Well, if it wasn't static, we'd need to start with already having an object. And to create the object, we need to give the constructor some values, and that… would defeat the purpose of having the factory method, plus would also push implementation detail of the Request back into the calling code, which is not where that code belongs. We could adjust the constructor to have all the parameters optional, and then chain a call to an object method, eg:

request = new Request().populateFromScopes() 

But I don't think that's semantically as good as having the factory method. But it's still fine, that said.

My rationale for using static methods that way is that sometimes creating the object is harder than just calling its constructor, so wrap it all up in a factory method. Now static methods aren't only for factories like that. Sometimes one might need to implement some behaviour on static properties, and to do that, it generally makes sense to use a static method. One could use an object method, but then one needs to ask "is it the job of this object to be acting on properties belonging to its class?" The answer could very well be "no". So implement the code in the most appropriate way: a static method. But, again, there could be legit situations where it is the job of an object method to act upon static properties. Code in object methods can refer to static properties as much as they need to. Just consider who should be doing the work, and design one's implementation accordingly.

A note on syntax

I could not think of a better place to put this, further up.

I've shown how to call static methods and access static properties via the class, it's: TheClassName::theStaticMethodName() (or TheClassName::theStaticPropertyName). However sometimes you have an object though, and legit need to access static elements of it. In this situation use the dot operator like you usually would when accessing an object's behaviour: someObject.theStaticMethodName() (or someObject.theStaticPropertyName). As per above though, always give thought to whether you ought to be calling the method/property via the object or via the class though. It doesn't matter, but it'll make your code and your intent clearer if you use the most appropriate syntax in the given situation.

Outro

Bottom line it's pretty simple. Just as an object can have state and behaviour; so can the class the object came from. Simple as that. That's all static properties and methods are.

Righto.

--
Adam

 

Oh. There are official docs of sorts:

• ColdFusion 2021+
• Lucee 5.0+

Switch refactoring on the road to polymorphism

G'day:

A while back I wrote an article "Code smells: a look at a switch statement". That deals with a specific case which was pretty tidy to start with, and wasn't completely clear as to whether the original or my refactoring was "better" (I think the initial switch was OK in this case).

Exceptions will always prove the rule as "they" say, and I'm gonna chalk that one up to a case of that. Also a good example of why sometimes it's not good to be dogmatic about rules.

However there a coupla general forms of switch statements that one can codify general refactoring advice for. Well: the main general rule is "switches are seldom necessary in your business logic code". If you find yourself writing one: stop to think if yer letting yerself down from an OOP perspective.

The other two situations I'm thinking of boil down to sticking to the Single Responsibility Principle: not munging more than one thing into a function basically. In the case of a switch, the two things are "what to do" and "how to do it". As well as when you fire a switch right into the middle of some other code, it's obvious it's breaking the SRP.

I'll start with that latter one.

Instead of this:

function f() {

    // code statements
    
    switch
    
    // code statements
}

Do this:

function f() {
    // code statements
    
    switchHandler()
    
    // code statements
}

function switchHandler() {
    switch
}

Note that switchHandler quite possibly doesn't even belong in the same class as the code using it.

In this case the first function is already probably doing more than one thing… it's got three sections, so that says "three things" to me. The whole thing should be looked at, but take the switch out completely. It is definitely and clearly doing at least one additional thing just by itself.

The second one is the other way around. Don't do this:

function f() {
    switch (expression) {

        case "A":
            // implementation of A
            break;

        case "B":
            // implementation of B
            break;

        case "C":
            // implementation of C
            break;

        default:
            // implementation of default
    }
}

Do this:

function f() {
    switch (expression) {

        case "A":
            caseAHandler()
            break;

        case "B":
            caseBHandler()
            break;

        case "C":
            caseCHandler()
            break;

        default:
            defaultHandler()
    }
}

function caseAHandler() {
    // implementation of A
}

// etc

Keep the "what to do" and the "how to do it" separate from each other: they are two different things (and the "how to do it" are each different things, so you have an 1+n degree of code smell going on there).

If you do these two things, a proper polymorphic handling of the situation - wherein you just use a type to deal with the logic - might start becoming more clear. Even if it doesn't straight away, this will make the refactoring easier when it does. You could use a factory to get the correct case handler, or if you know ahead of time what expression is, you can just pass in the correct handler in the first place. Or some other way of doing things. But even without taking it further, your code is less smelly, so is better.

Also note that if you have multiple switch statements with the same expression, then it's a clue that the whole lot should come out into its own class, and that a more OOP handling of the situation would simplify your code.

I only had 15min to write this this morning, so apologies it's a bit clipped-sounding. Must dash.

Righto.

--
Adam

Factories, static methods, Law of Demeter and never letting Sean see my code

G'day:

I'm absolutely kidding about one of those things, btw.

Right so yesterday I wrote a coupla articles about what code should and should not go into a controller:

• What logic should be in a controller? (and a wee bit of testing commentary)
• Another thought on controllers and where the buck should stop

Within that first one, I provided some sample code to demonstrate my point. Part of it was the usage of a factory class to provide some necessary wiring for my model class, which I then had as a dependency of my controller:

// UserContentFactory.cfc
component {

    function init(
        ValidationService validationService,
        UserFactory userFactory,
        UserContentService contentService,
        TwitterService twitterService,
        FacebookService facebookService
    ) {
        variables.validationService = arguments.validationService
        variables.userFactory = arguments.userFactory
        variables.contentService = arguments.contentService
        variables.twitterService = arguments.twitterService
        variables.facebookService = arguments.facebookService
    }

    function getUserContent() {
        userContent = new UserContent()
        userContent.setValidationService(validationService)
        userContent.setUserFactory(userFactory)
        userContent.setContentService(contentService)
        userContent.setTwitterService(twitterService)
        userContent.setFacebookService(facebookService)
        
        return userContent
    }
}

// UserContent.cfc
component accessors=true invokeImplicitAccessor=true {

    property publishedContent;
    property twitterContent;
    property facebookContent;

    function init(publishedContent, twitterContent, facebookContent) {
        variables.publishedContent = arguments.publishedContent
        variables.twitterContent = arguments.twitterContent
        variables.facebookContent = arguments.facebookContent
    }
    
    function setValidationService(ValidationService validationService) {
        variables.validationService = arguments.validationService
    }
    
    function setUserFactory(UserFactory userFactory) {
        variables.userFactory = arguments.userFactory
    }
    
    function setContentService(UserContentService contentService) {
        variables.contentService = arguments.contentService
    }
    
    function setTwitterService(TwitterService twitterService) {
        variables.twitterService = arguments.twitterService
    }
    
    function setFacebookService(FacebookService facebookService) {
        variables.facebookService = arguments.facebookService
    }
    
    function loadContentByFilters(required struct filters) {
        validFilters = validationService.validate(filters, getValidationRules()) // @throws ValidationException
        
        user = userFactory.getById(validFilters.id) // @throws UserNotFoundException
        
        publishedContent = contentService.getUserContent(validFilters)
        twitterContent = twitterService.getUserContent(validFilters)
        facebookContent = facebookService.getUserContent(validFilters)
    }
    
    private function getValidationRules() { 
    	// ... elided to save space
    }
}

// UserContentController.cfc
component {

    function init(ViewService viewService, UserContentFactory userContentFactory) {
        variables.viewService = arguments.viewService
        variables.userContentFactory = arguments.userContentFactory
    }

    function getContent(rawArgs) {
        try {
            userContent = userContentFactory.getUserContent().loadContentByFilters(rawArgs)
            
            renderedResponse = viewService.renderView("userContentView", userContent)
            
            return new HtmlResponse(renderedResponse)
            
        } catch (ValidationException, e) {
            return new ClientErrorResponse(400, e)
        } catch (UserNotFoundException e) {
            return new ClientErrorResponse(404, e)
        }
    }
}

(Note that all of this is untested basically "pseudo"-code. I've never run it, it's just a theoretic approach to the issue. By all means let me know if there's anything syntactically or logically wrong with it, but it's not code intended to be run)

loadContentByFilters relied on five external services to do its stuff, and I didn't want to pass those services to UserContent as constructor arguments because then they'd get mungled up with the data values I wanted the UserContent model's constructor to focus on, so I was using setter injection to configure UserContent with those, and have that factory class and method to deal with all that in the background (via whatever dependency injection / IOC container framework I would be using). By the time the controller received the factory it'd been configured, and it had been able to configure a UserContent object, so all the controller needed to do was to call it.

All simple enough.

Sean popped a comment against the article, I responded, and he came back to me again. At which point I decided I'd write this article instead of replying again:

Sean:

Since you're being so hardcore about drawing lines between the MVC parts of your code, I'm going to call this into question:

userContentFactory.getUserContent().loadContentByFilters(rawArgs)

There's an argument often made that you shouldn't have method chaining like this since it couples the caller to the structure of the chained method calls on another object. The argument goes that you should instead have something like:

userContentFactory.getUserContentByFilters(rawArgs)

Thoughts?

---

Adam:

Yeah. I actually started with that! But then I didn't think it was the business of the factory to go as far as the actual data loading. But… I was not entirely convinced either way, to be completely honest.

I'm also completely down with the Law of Demeter when it comes to chaining methods like that. I did think about that at the time too.

The conclusion I drew was that the factory was just a "convenience extraction", or a utility or something that is completely "in the service" of UserContent. It serves absolutely no purpose other than a mechanism (I misspelled that as "MEHchanism" at first, perhaps fittingly) to wire-together the dependencies that loadContentByFilters needs to do its job. I did not want the controller to have to know how to do that.

The second draft of the actual code had two separate statements:

userContent = userContentFactory.getUserContent()
userContent.loadContentByFilters(rawArgs)

Which is functionally the same, and has the same Law of Demeter considerations you raised, but maybe the thinking is clearer: useContent = userContentFactory.getUserContent() is just an analogy of useContent = new UserContent(). I just slapped it into one statement because that was more convenient (ahem) for the point I was trying to make.

A simpler scenario would be if I wanted to be able to create a UserContent object via a struct that contained name/value pairs matching the constructor params it needs, I'd simply have a (probably static) method on the UserContent class which was createFromStruct which would internally grab the bits and pieces out of the struct and return a new UserContent object, passing those values to its constructor. No need for any separate factory cos there's no external dependencies to have to deal with somehow.

I knew that bit of the code would cause consternation from some quarters... and was hoping one of the quarters would be your own ;-).

---

Sean:

I find it interesting that you might subsume a factory for createFromStruct() as a static method but not for loadByFilters()…?

Now that is a very good point.

Truth be told, I simply didn't think of it at the time I was writing the blog article, but I did recall having done this in the past by the time I was writing the reply to Sean's question.

But let me back up a bit. Law of Demeter. In summary it means it's OK to call methods on a dependency, but it gets decreasingly OK to call method on objects that the methods of the dependency return, and so on. This is poor form:

function myMethod() {
    return someDependency.someMethod().someMethodOfAnotherObject().wowWeAreNowALongWayAwayFromTheDependency()
}

This is because not only is your implementation tightly coupled to its dependency - but we kinda need to live with that - it's also tightly coupled to the objects that implement someMethodOfAnotherObject and wowWeAreNowALongWayAwayFromTheDependency. You should not need to do all that. SomeDependency should take care of it for you. It should not be exposing its inner workings like that.

BTW, this does not apply to objects that implement a fluent interface. This is fine:

function myMethod() {
    return obj.someMethod().someOtherMethodOfObj().andAnotherMethodOfObj()
}

All those methods are part of the API exposed by obj.

So that's what Sean pulled me up on:

userContent = userContentFactory.getUserContent().loadContentByFilters(rawArgs)

That's a bit of a LoD violation, and worth pointing out. I did reason through this (before Sean raised it, as I said), and I'm fine with it.

But Sean's nudging of me back towards static methods solves this.

We could implement UserContent like this:

// UserContent.cfc
component accessors=true invokeImplicitAccessor=true {

    property publishedContent;
    property twitterContent;
    property facebookContent;

    function init(publishedContent, twitterContent, facebookContent) {
        variables.publishedContent = arguments.publishedContent
        variables.twitterContent = arguments.twitterContent
        variables.facebookContent = arguments.facebookContent
    }
    
    static function setValidationService(ValidationService validationService) {
         static.validationService = arguments.validationService
     }
     // etc, all the methods to set the services are static methods, setting a static property
    
    static function loadContentByFilters(required struct filters) {
        validFilters = validationService.validate(filters, getValidationRules()) // @throws ValidationException
        
        user = userFactory.getById(validFilters.id) // @throws UserNotFoundException
        
        return new UserContent(
            contentService.getUserContent(validFilters)
            twitterService.getUserContent(validFilters)
            facebookService.getUserContent(validFilters)
        )
    }
    
    // irrelevant stuff snipped
}

• One sets the dependency services via static methods;
• loadContentByFilters is also a static method which does its stuff, and returns a new instance of itself with the data values set.
• Not shown here is that when the IoC framework creates the dependency container, it runs the factory method then, to set the services into the UserContent class (NB: "class", not "object") once.

Now in the controller we have a coupla options:

userContent = UserContent::loadContentByFilters(rawArgs)

No need to pass the class in as a constructor argument or anything (just as well: one cannot pass classes around as arguments), on just calls the method on the class.

Inlining static method calls like this is not so good for testing, that said, as it kinda bypasses dependency injection just like having new UserContent() in there, so this is less than ideal.

However there's nothing to stop us passing in an uninintialised instance of UserContent into the controller as a dependency. The IoC handling of this would be along the lines of:

container.set("UserContent", (container) => createObject("UserContent")) //NB: *not* using `new`, cos that calls the constructor
container.set("UserContentController", (container) => new UserContentController(container.get("ViewService"), container.get("UserContent")))

Then we use the dot operator instead of the :: operator to call the static loadContentByFilters via the object reference to the class: userContent

userContent = userContent.loadContentByFilters(rawArgs)

Better yet, perhaps, would be to just make the constructor arguments optional, and just use new UserContent() ;-)

Note: only ColdFusion 2021 has static support, and only Lucee 5.3.8 and above allows using the dot operator to call a static method on an object.

---

No doubt Sean will find other points of order from all this, but that's great cos it gives me (/us) an opportunity to think about stuff some more.

Right now I'm thinking about another beer, so I'm gonna do that.

Tomorrow I need to have a prod of CFML's handling of static stuff. I've already spotted something that looks like a bug in ColdFusion (quelle frickin surprise), but I need to check some stuff first, and I CBA doing it now. Because beer.

Righto.

--
Adam

Another thought on controllers and where the buck should stop

G'day:

I just wrote an article on what a controller ought to limit itself to: "What logic should be in a controller? (and a wee bit of testing commentary)". But then I read an old question from Mingo on the article that inspired that, and I have some thoughts on that too.

In most of my code examples around controller methods, I have this sort of method signature:

function handleGet(rawArgs)

What I mean by "raw args" is "all the stuff from an HTTP request, including: query string parameters and arguments (URL scope if yer a CFMLer), request body keys and values (form scope), general request metadata (CGI scope), cookies, and headers. Kind of like how Symfony would do it in the PHP world.

In CFML land things are seldom (never?) this organised. One seems to get a hotchpotch of things possibly put into a request context or something, or possibly - as in CFWheels - actually nothing(!!!) gets passed into the controller method; you just need to know the magical place to go look for them. And by "them" I mean a struct that has the form and URL scope munged into it. Ugh. Anyhow, there's all these elements of an HTTP request and the application lifecycle (application, session, request scopes) available to yer CFML code somehow.

And the controller is the only place one should ever access those. Your business logic should never be tightly-coupled the notion of "this stuff came from a specific sort of HTTP request", or from a specific stage in the application's lifecycle. Or even be aware of the ideas of HTTP requests and the like.

Your model tier should just get "values". Ideally by the time you are applying any actual application logic to them, the values will have been modelled into their own objects, not simply a bunch of primitive values.

I guess one could consider a controller to be similar in role to a repository class, just the other way around. A repository encapsulates the mapping between [for example] storage records - which it fetches - and collections of objects - which it returns - to the business-logic tier. A controller is slightly skinnier than that, it takes the values needed from that selection of HTTP request components (URL scope, form scope, what-have-you), and just passes them as independent values - distinct from how they arrived - to the model. I guess it's not a direct parallel because the controller doesn't do the "mapping" part that is intrinsic to a repository; it just removes the context from the values it receives, and leaves it up to the model tier to know what to do with them. But anyhow, there's a clear separation of concerns here, and the separation is that only the controller should ever deal with these things: CGI scope, the value returned from GetHttpRequestData(), form scope, URL scope, cookie scope, application scope, session scope, request scope.

Righto.

--
Adam

What logic should be in a controller? (and a wee bit of testing commentary)

G'day:

This topic has come up for me twice from different directtions in the last week or so, so I'm gonna dump some thoughts. I've actually discussed this before in "I actively consider what I think ought to go into a controller", and the conclusion I came to doesn't quite fit with how I'm writing code now so I'm gonna revise it a bit.

To start with though, I'll reiterate this question: "should this go in the controller?", and I'll repeat Mingo's answer that is still pithy but spot on:

Isn't the answer always "No"?

~ mjhagen 2017

This is a good starting point. 95% of the time if yer asking yerself that question, Mingo has answered it for you there.

The example of what I'd put in a controller from that article is along these lines:

class ContentController {
    function handleGet(rawArgs){
        args = validationService.validate(rawArgs)
        
        content = contentService.getById(args.id)
        
        response = {
            articles = content.published,
            socialContent = {
                twitter = content.twitter,
                facebook = content.facebook
            }
        }
        return new Response(response)
    }
}

It's not the worst code I've written, but I now think this is wrong. The problem lies with how I had a habit of abusing the Anaemic Domain Model pattern in the past, where I had a bunch fo really skinny service classes, and used them to apply behaviour to behaviourless model objects that were just bags of data. Not great.

Looking at that code now, I see these lines:

args = validationService.validate(rawArgs)
content = contentService.getById(args.id)

And I think "nah, those don't belong there. They belong in the model too". I'm doing too much "asking" when I should be "telling" (see "TellDontAsk" by Martin Fowler).

Basically the model here should know what it is to be "valid", so just give it the raw data and let it crack on with it.

My generic controller method these days would be formed along these lines:

function handleRequest(rawRequestValues) {
    try {
        dataFromModel = someModel.getSomeDataFromThisLot(rawRequestValues)
        
        renderedResponse = viewService.renderView("someView", dataFromModel)
        
        return new HtmlResponse(renderedResponse)
        
    } catch (ClientException e) {
        return new ClientErrorResponse(e)
    }
}

Here we clearly have a separation of controller, model and view. It's the controller's job to marshall getting values to a model, and getting the values from that to a view, deal with any error responses that might arise due to those two, or return what came back from the view tier as the response. That's it.

There's an assumption that the framework will deal with any unhandled exceptions there as a controlled 5xx type response. Also there could well be more catch statements, if different types of exception could bubble out of the model, for instance a ValidationException which returns details of validation failures in its response; or a 404 response being returned if a UserNotFoundException came back from some business-logic validation or whatever. But that's the pattern.

The key here is that the only time I'm using a value created by the model is to pass it to the view. I do not pass it to anything else in the interim, like some other model call. That action is not controller logic. It's business logic that we get an x and then pass it to a y. It should be encapsulated in the model.

On the other hand if there was more than one piece of view data to be derived directly from the incoming request values, then that would to me still possibly be legit to be in the controller, eg this is OK:

dataFromModel = someModel.getSomeDataFromThisLot(rawRequestValues)
moreDataFromDifferentModel = someModelOther.getSomeDifferentDataFromThisLot(rawRequestValues)

This would not be OK:

dataFromModel = someModel.getSomeDataFromThisLot(rawRequestValues)
moreDataFromDifferentModel = someModelOther.getSomeDifferentDataFromThisLot(dataFromModel.someValue)

It's a small distinction. But the thing to focus on more than that small example is just to be thinking "no" when you ask yerself "does this belong in the controller?". You're more likely to be right than wrong.

---

How do we apply that pattern to the example in the old article? Like this I think:

// UserContentController.cfc
component {

    function init(ViewService viewService, UserContentFactory userContentFactory) {
        variables.viewService = arguments.viewService
        variables.userContentFactory = arguments.userContentFactory
    }

    function getContent(rawArgs) {
        try {
            userContent = userContentFactory.getUserContent().loadContentByFilters(rawArgs)
            
            renderedResponse = viewService.renderView("userContentView", userContent)
            
            return new HtmlResponse(renderedResponse)
            
        } catch (ValidationException, e) {
            return new ClientErrorResponse(400, e)
        } catch (UserNotFoundException e) {
            return new ClientErrorResponse(404, e)
        }
    }
}

(I've changed what the controller is returning so as to still integrate the view tier into the example).

I've done away with the controller handling the validation itself, and left that to the model. If things don't pan out: the model will let the controller know. That's it's job. And it's just the controller's job to do something about it. Note that in this case I don't really need both catches. I could just group the exceptions into one ClientException, probably. But I wanted to demonstrate two potential failures from the logic in loadContentByFilters.

---

What's with this factory I'm using? It's just one of my idiosyncrasies. I like my models' constructors to take actual valid property values, like this:

// UserContent.cfc
component accessors=true invokeImplicitAccessor=true {

    property publishedContent;
    property twitterContent;
    property facebookContent;

    function init(publishedContent, twitterContent, facebookContent) {
        variables.publishedContent = arguments.publishedContent
        variables.twitterContent = arguments.twitterContent
        variables.facebookContent = arguments.facebookContent
    }

Our UserContent represents the data that are those content items. However we've not been given the content items, we've just been given a means to get them. So we can't just create a new object in our controller and slap the incoming values into them. We need to have another method on the UserContent model that works with what the controller can pass it:

function loadContentByFilters(required struct filters) {
    validFilters = validationService.validate(filters, getValidationRules()) // @throws ValidationException
    
    user = userFactory.getById(validFilters.id) // @throws UserNotFoundException
    
    variables.publishedContent = contentService.getUserContent(validFilters)
    variables.twitterContent = twitterService.getUserContent(validFilters)
    variables.facebookContent = facebookService.getUserContent(validFilters)
}

And this demonstrates that to do that work, UserContent needs a bunch of dependencies.

I'm not going to pass these in the constructor because they aren't 100% needed for the operation of a UserContent object, and I want the constructor focusing on its data. So instead these need to be injected as properties:

// UserContent.cfc
component accessors=true invokeImplicitAccessor=true {

    property publishedContent;
    property twitterContent;
    property facebookContent;

    function init(publishedContent, twitterContent, facebookContent) {
        variables.publishedContent = arguments.publishedContent
        variables.twitterContent = arguments.twitterContent
        variables.facebookContent = arguments.facebookContent
    }
    
    function setValidationService(ValidationService validationService) {
        variables.validationService = arguments.validationService
    }
    
    function setUserFactory(UserFactory userFactory) {
        variables.userFactory = arguments.userFactory
    }
    
    function setContentService(UserContentService contentService) {
        variables.contentService = arguments.contentService
    }
    
    function setTwitterService(TwitterService twitterService) {
        variables.twitterService = arguments.twitterService
    }
    
    function setFacebookService(FacebookService facebookService) {
        variables.facebookService = arguments.facebookService
    }

That's all a bit of a mouthful every time we want a UserContent object that needs to use alternative loading methods to get its data, so we hide all that away in our dependency injection set-up, and use a factory to create the object, set its properties, and then return the object:

// UserContentFactory.cfc
component {

    function init(
        ValidationService validationService,
        UserFactory userFactory,
        UserContentService contentService,
        TwitterService twitterService,
        FacebookService facebookService
    ) {
        variables.validationService = arguments.validationService
        variables.userFactory = arguments.userFactory
        variables.contentService = arguments.contentService
        variables.twitterService = arguments.twitterService
        variables.facebookService = arguments.facebookService
    }

    function getUserContent() {
        userContent = new UserContent()
        userContent.setValidationService(validationService)
        userContent.setUserFactory(userFactory)
        userContent.setContentService(contentService)
        userContent.setTwitterService(twitterService)
        userContent.setFacebookService(facebookService)
        
        return userContent
    }
}

The controller just needs to be able to ask the factory for a UserContent object, and then call the method it needs, passing its raw values:

userContent = userContentFactory.getUserContent().loadContentByFilters(rawArgs)

---

You'll noticed I kept the validation separate from the UserContent model:

function loadContentByFilters(required struct filters) {
    validFilters = validationService.validate(filters, getValidationRules()) // @throws ValidationException

(And then there's also this private method with the rules):

private function getValidationRules() {
    return {
        id = [
            {required = true},
            {type = "integer"}
        ],
        startDate = [
            {required = true},
            {type = "date"},
            {
                range = {
                    max = now()
                }
            }
        ],
        endDate = [
            {required = true},
            {type = "date"},
            {
                range = {
                    max = now()
                }
            }
        ],
        collection = [
            {callback = (collection) => collection.startDate.compare(collection.endDate) < 0}
        ]
    }
}

Validation is fiddly and needs to be accurate, so I don't believe how to validate some values is the job of the UserContent class. I believe it's just perhaps its job to know "what it is to be valid". Hence that separation of concerns. I could see a case for that private method to be its own class, eg UserContentValidationRules or something. But for here, just a private method is OK. Wherever those rules are homed, and whatever the syntax of defining them is, we then pass those and the data to be validated to a specialist validation service that does the business. In this example the validation service itself throws an exception if the validation fails. In reality it'd more likely return a collection of rules violations, and it'd be up to the model making the call to throw the exception. That's implementation detail not so relevant to the code here.

---

There's probably more off-piste code in this an on-~, but I think it shows how to keep yer domain / business logic out of your controllers, which should be very very light, and simply marshall the incoming request values to the places that need them to be able to come up with a response. That's all a controller ought to do.

---

Oh before I go. There's an attitude from some testing quarters that one doesn't test one's controllers. I don't actually agree with that, but even if I did: that whole notion is predicated on controllers being very very simple, like I show above. If you pile all (or any of ~) yer logic into yer controller methods: you do actually need to test them! Even in this case I'd still be testing the flow control around the try/catch stuff. If I didn't have that, I'd probably almost be OK if someone didn't test it. Almost.

Righto.

--
Adam