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 of 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 marshal 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
