Are we doing MVC wrong?

Are we doing MVC wrong?(withouttheloop.com)

35 points by xivSolutions 12 years ago | 21 comments

My issue here is that BorrowABook is a verb. This isn't a model class, and in my understanding of OOP, doesn't really deserve a class by itself at all. You could instead put the borrow method in a class called Book that represented the model of your application. Then you call borrow on the book, the book does some logic, and returns a status code to the controller. Or since it's a bit silly to ask a book to borrow itself, have a library model object that handles a bunch of book objects, and put the borrow method in the library class. That's how I do MVC anyways.

nbevans 12 years ago | |

BorrowABook is a Command. What the article is, albeit unknowingly, discussing is roughly the CQRS pattern. CQRS can rarely ever be considered to be a bad practice. As good developers mature, they come to realise that generally all the good patterns in the world worth pursuing have traits of immutability, messaging and functional programming (FP), even in OOP-land. Most good OOP designs are merely trying to emulate what FP does.

danso 12 years ago | |

I agree with this...perhaps it is more akin to a factory method, where parameters are passed in and a book, with its borrow status affected, is returned?

mattmanser 12 years ago |

Unfortunately you've misunderstood MVC.

None of that code belongs in the controller. Your 'typical' controller is anything but. All that code belongs in the model.

Your controllers should have zero LINQ statement. You should do all of the validation in the model and the validation failures should returned in a List.

This stuff's been in MVC for years:

http://weblogs.asp.net/scottgu/archive/2010/01/15/asp-net-mv...

When people talk about 'light' or 'skinny' controllers, this is what they're talking about.

So we're not doing MVC wrong, you've just developed bad habits from bad microsoft examples, work with some experienced Devs so they can teach you how to do it correctly.

xivSolutions 12 years ago | |

So, kindly watch the way you throw the word "you" around. I did not write the article, I just wanted to see what the community might have to say about it. I am coming to understand MVC decently well.

That said, your points are good, and I agree, based on what I have learned myself to this point.

:-)

r0s 12 years ago | |

What's the advantage?

Is the model constantly pulling session state to validate against?

Does this prevent caching of data on the models? Or is there 'supposed' to be some even higher level API manager that deals with the responses before they get to the model?

pionar 12 years ago | | |

Your controller should be passing the session state to the model (passing the items needed from session state for the model to manipulate data). Models should have no concept of a session. They should be almost entirely self-contained.

Controllers should do only basic validation (length, format, etc.)

mattmanser 12 years ago | | |

It sounds like you're stuck using `Session` object which you should almost never touch in ASP.Net MVC.

HTTP is stateless. You should program accordingly and not rely on the old ASP.Net webforms ways, they were extremely broken and taught some developers some very bad habits (view state was a terrible, terrible idea. Session state should be used in extremely rare circumstances and it's much better to never use it).

That's one of the reasons MVC caught on so fast. It actually reflects what in reality is happening and is not the terribly leaky abstraction that Webforms was.

In the controller you get an updated object from your repository on each request. You update it with the data that you received in the request (and MVC can do this for you itself). You ask it to validate itself. You save if valid. You return your success view. You don't save if not valid, you return the validation errors.

Hence the controller is only a few lines.

That's how you're supposed to program in MVC because HTTP is stateless. And because SQL is much faster than you probably think it is. And that makes the code vastly simpler. And you can cache persistent things like the data for common dropdowns in memory using the actual `System.Runtime.Caching` functionality.

juskrey 12 years ago |

MVC is not a dogma, it is a no-brainer instruction to make 90% of programming fruits less miserable. I usually end up with something like Views-Composers-Handlers-Routes-Libs etc. And this can vary from project to project.

bbwharris 12 years ago | |

I agree. There's a lot of sentiment around, "the right way to do something". I somewhat blame rails for the explosion of opinions, but maybe it's just developer nature.

The right way is the way that works for you and your business requirements.

mhteas 12 years ago |

It's not that the MVC is wrong, it is that the model is not being built right. Moving to the BorrowABook feature is making a model class.

All too often in my experience I see code that has the model rolled into the controller. Controllers and UI are structurally supported better in frameworks. We get lazy and put model code into controllers. It is the way we make spaghetti code today.

camus2 12 years ago | |

We get lazy because most of the time ,without a proper Ioc container , writing code the right way is too much verbose.

Ioc containers changed the way I code since I did not have to care anymore about how complicated it was to instanciate objects. It helps write very clean code and makes OOP easier.

Anchor 12 years ago |

This is similar (or more or less the same) idea that has been advocated by Uncle Bob and others. Here's my comment on the experiences we have had with this approach: https://news.ycombinator.com/item?id=6470693 (the topic was about running tests quickly, and the approach, as mentioned also in the article, decouples the business logic from frameworks - allowing testing without firing up web server, database, etc.).

I agree that the interactors make rather odd-looking OO classes, but I suppose the imperative nature of most requests to perform some logic make them less objects in that sense. This has the added benefit of seeing what the system does, just by looking the class names in the interactors folder/namespace.

joebo 12 years ago |

> Features return a glorified tuple (Feature<TOutcome,TResult>) that combines an enum value that represents what the outcome was, with a value that represents the result of the operation.

Sounds similar to my understanding of what a monad is. If you have many cases of this, it might simplify the switch statement and improve maintainability.

http://devtalk.net/csharp/chained-null-checks-and-the-maybe-...

http://stackoverflow.com/questions/674855/help-a-c-sharp-dev...

lmm 12 years ago |

The controller is always the ugly part, because it's the piece that's responsible for interfacing between transaction-specific logic and your general backend interface. The article is right that it's easy to put too much business logic in the controller, but fails to recognize that "MVC" isn't supposed to be your whole application - just the UI side. The functionality of this BorrowABook class doesn't belong in the UI layer at all.

bshacklett 12 years ago | |

MVC is something I've never really been able to wrap my head around. I wonder if this is why... All of the guides/articles that I've read on the subject have acted as if MVC was how the entire application was handled.

Could you recommend any references for back-end patterns?

icebraining 12 years ago | | |

I like Patterns of Enterprise Application Architecture, which includes some domain logic patterns. A common one is SOA, by dividing the backend into services that your UI consumes.

nbevans 12 years ago | |

Controller is an adapter or anti-corruption layer designed to couple together two separate bounded contexts. It's feasible and likely there would be two variants of BorrowABook, one in the UI model and one in the service/domain model. They may be subtly different in what fields they hold.

camus2 12 years ago |

> Controllers are the UI

I agree.

> What is the responsibility of the controller?

Controllers are the UI ? but the UI of what ? HTTP. controllers should only deal with HTTP (request,respone,cookies,session) stuff , nothing else. The rest should be encapsulated in services, which communicates through models.

The truth is , it is not MVC , MVC is a poor label for that kind of architecture since MVC means something very specific in software engineering.

liammclennan 12 years ago |

Several peopls have commented that this is really just CQRS.

The approach I described above has nothing to do with CQRS, in fact it directly contradicts CQRS. The design of features is intended to combine commands and queries. If the controller is restricted to commands and queries then the logic of combining those operations ends up in the controller which is exactly what we need to avoid.

dangoor 12 years ago |

This reminds me of Data-Context-Interaction (DCI):

http://en.wikipedia.org/wiki/Data,_Context,_and_Interaction