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