MVC Refactor, Take 2

[TomB]

@Jeff_Mott

It doesn’t seem that way at all to me. In the Web MVC examples, the repository and template rendering objects are still being injected. But in your examples, you’re not just abstracting away the objects themselves, you’re trying to abstract away the invocations we make on those objects, and trying to abstract away the arguments we pass to the abstracted invocations. Just because Web MVC doesn’t go to that extreme doesn’t mean its dependency objects aren’t still injected.

You’re constructing a template with a hardcoded filename. Just because it’s not a class doesn’t mean it’s not the same as new Template(). The same is true of a model, in most web mvc implementations the model is chosen using a service locator, again, going against IoC because the decision of what to include is being made in the controller, which means that the controller logic cannot easily be used with another template.

@Jeff_Mott

Chances are, sure, but not guaranteed. The two might actually be very different that we prefer to keep them as separate templates. But your approach that you advertise as more flexible doesn’t give us this choice.

That’s just not true though is it? there’s no reason the template can’t do this:

if ($album instanceof ClassicAlbum) include 'classicalbum.html.php'
else include 'album.html.php';

This is a lot better than having the logic in the controller because the same controller can be used with this template or another template that does something entirely different.

@Jeff_Mott

Compared to Fowler’s original album = Album.find(id), your alternative strikes me as significantly more complex. I hope we get a lot of bang for this buck.

That’s rather disingenuous. If we’re doing an apples to apples comparison it’s the model I provided vs fowler’s controller. In fowler’s example, the developer would need to write the entire controller, in my first example the developer needs to write a slightly shorter model. In my second, the developer has to write only the router configuration.

@Jeff_Mott

This relies on the very big assumption that albums, blogs and products all use the exact same logic. But one might load by ID while another loads by slug, or another might want to load using a pre-loading query, any of which would foil your reusability and force you to make separate model classes. Or one might be loading a single entity but still have other input to consider such as session data, which would force you to make separate controller classes.

Which is true. Let’s say one of the templates needs some session data. If that’s the case you need to write a whole new controller action that loads the template and passes the session. Let’s start with fowler’s example (ported to php)

class AlbumController {
	
	
	public function doGet() {
	    $album = Album::find($request->getParameter('id'));
	    if ($album === null) $this->template->render('missingalbumerror.tpl.php');
	    else {
	    	if ($album instanceof ClassicAlbum) $this->template->render('classicalbum.tpl.php', ['album' => $album]);
	    	else $this->template->render('classicalbum.tpl.php', ['album' => $album]);
	    }

	}

}

Now from this base point, suppose I want a different template which displays something from the session and the album. I now need to add another function in the controller:

class AlbumController {
	
	
	public function doGet() {
	    $album = Album::find($request->getParameter('id'));
	    if ($album === null) $this->template->render('missingalbumerror.tpl.php');
	    else {
	    	if ($album instanceof ClassicAlbum) $this->template->render('classicalbum.tpl.php', ['album' => $album]);
	    	else $this->template->render('classicalbum.tpl.php', ['album' => $album]);
	    }

	}


	public function doGetWithSession() {
	    $album = Album::find($request->getParameter('id'));
	    if ($album === null) $this->template->render('missingalbumerror.tpl.php');
	    else {
	    	if ($album instanceof ClassicAlbum) $this->template->render('classicalbum.tpl.php', ['album' => $album, 'session' => $_SESSION['whatever']]);
	    	else $this->template->render('classicalbum.tpl.php', ['album' => $album, 'session' => $_SESSION['whatever']]);
	    }

	}

}

Admittedly, it would be possible to refactor the class and move the repeated logic into its own method… but this takes time!

On the other hand, with my approach, there is no need for this refactoring, you’d add the model:

class AlbumRepositoryModel extends RepositoryModel {
	public function getSession() {
		return $_SESSION['whatever'];
	}
}

and add the route accordingly to pull in the new template and the new model.

Alternatively if you need to load via a URL slug or another field in the database then it’s simple enough to make a generic model and controller to handle it (If it’s a common function like that).

@Jeff_Mott

I’m tempted to call your approach “premature refactorization”. You start with the assumption that certain chunks of code will be identical, then refactor as appropriate. But they often won’t be identical, and your prematurely refactored code ends up making things more rigid rather than more flexible.

I’m not sure I agree with this. Having used this approach for several years now (After migrating away from Web MVC as I found myself repeating code far too frequently and having to write far too much code to do simple tasks). With controllers for: Forms, Result sets (With extensions for paginate, search) and single entities, this handles 90%+ of my use-cases. On the odd occasion I do have to write a controller but in web mvc I’d have had to have written that controller anyway, and I’d also have had to have written it the other 90% of the times where I didn’t need to.

@Jeff_Mott

I feel like this topic is almost talked out. I think the code comparisons were a lot better than our usual hand wavy claims; hopefully others thought so too. But obviously I’m not sold.

One last question if you don’t mind: I feel that this topic has been me explaining the advantages of a more mvc-like architecture and you mostly saying “You can do that in WebMVC too if you make these changes”, what I would like is a brief list of what you think the advantages are of web mvc over my suggested approach.

I’ll do the same for my approach:

• Have to write significantly less code when just viewing a static template
• Have to write significantly less code when loading data from a model without any user input
• Have to write less code for common use cases (Forms, pagination, loading a single entity by ID)
• Have to write the same amount of code otherwise
• The model, view or controller can easily be substituted without rewriting the controller action
• Which overall significantly lowers development time
• Programming around interfaces gives a much more standardised approach so it’s easier for new developers to understand
• Controllers don’t break SRP
• Controllers don’t break encapsulation (Knowing what response type the view is)
• Controllers don’t break LoD/Tell, Don’t ask
• Looser coupling overall (Controllers are tightly coupled to models and templates in web mvc most of the time)