I think the most egregious things are:

Should be using etags so we can send back a 304 if there are no changes, this would make the method cache friendly.

Only supports JSON, would be nicer if we could specify how we wanted the results serialized.

Almost no exception handling, for example passing in a bad date would throw a FormatException, probably not ideal.

Doing a potentially expensive query when a cheaper one would probably suffice if the method used etags. For example, you could use a simple select max query to find the latest post to see its etag matched before doing a query to get all the latest posts.

Repository pattern is kind of lame, would be better to wrap up everything in a query (including the sort order).

Casting dates to strings limits your options for how they are serialized.

I'm sure there's lots more...

There is too much stuff going on in the controller method.

The list of posts is unbounded.

The last post logic should be done in the view model and as a strongly typed method.

Date parsing is done in a bad way. It should have been done with a model binder or at least have error handling.

the issues you will cover are listed under future posts :)

Couple of things

1) Why call the method GetLatestPost and force HttpPost instead of HttpGet

2) Shouldn't be nicer to have this logic IList <post posts =

    _postRepository

    .FindAll(new GetPostLastestForBoardById(lastPostDateTime, boardId))

    .OrderBy(x=>x.Id).ToList();

inside a Service ?

3) Why UpdatePostViewModel when you are just getting the latest post?, the namings are kinda confusing.

Additonal to the already said problems, there will be a lazy-loading of "Owner" while AutoMapping. "Fetch(x => x.Owner" could be used instead.

1. [Transaction] - I think transaction should be controlled from repository, not controller method

2. _postRepository is used as a normal NHibernate ISession, can't say exactly because _postRepository def is missing

3. _postRepository - loading all posts just to count them, unbounded result set

4. lastKnownPost - should be loaded with explicit query

5. Mapper.CreateMap - it's not a place to define mapping, there should be a central place

After writing that 6 points:

replace all of the code with projections and setResultTransformer or QueryOver ( no need to use mapper at all) and just use ISession

Indeed your future posts betray you :)

So, to add to "too much going on" and "open to denial of service by sending in DateTime.Min" I cannot see the point of ordering by id and then looking for a Max value instead of lwtting the DB do the work by ordering the query by the relevant Date field and calling FirstOrDefault and be done with it.

If the solution keeps AutoMapper, the CreateMap call is expensive - it should only be done once per AppDomain.

Jimmy, I think that would be "setup belongs in the initializer" ;)

If you want another OS project to pick on that is NHibernate/MVC powered, I'd be interested in (anyone's) feedback for http://hg.shrinkrays.net/roadkill/ . It's yet-another-wiki-engine but for .NET.

It's missing docs and is about 90% complete right now. I feel aloof in saying it has no business logic in the controllers, aside from a few experimental methods.

@Daniel - You're making the assumption that the fetching strategy GetPostLastestForBoardById doesn't already fetch the Owner. Without looking at it, it's possible that it does fetch the owner.

I would put [Authorize] and [Transaction] on a base controller and avoid having to put it on every method.

The .ToList(); causes a database hit and the count and max to be done in memory.

The string lastPost should be a datetime or ideally pass in the UpdateViewModel to the method.

Add a method to the repository called GetLatestPostForBoardById.

Do I need automapper for a single field update?

Change ActionResult to JsonResult if you really are only going to return Json data.

Is it worth remembering that whist this may have a number of problems, it was a university project. They developer is learning in public, which should be applauded.

"The .ToList(); causes a database hit and the count and max to be done in memory."

Yes, but posts is mapped just prior to the return statement. If the method relied on Deferred Execution to call Count/Max the query to the database would call Count, then throw an exception on Max since the query has been executed.

Most comments seem to be fixated on the database related items; the first thing that caught my eye was the first line of code:

DateTime lastPostDateTime = DateTime.Parse(lastPost);

A nice avoidable exception waiting to happen, plus if this is any indication of the coding style, XSS might be a concern in other places as well.

Gotta agree with Rob, pretty impressive for a university project.

The biggest problem is that Microsoft show so many examples developed in just this way with controllers stuffed full of logic that should be placed elsewhere... it's no surprise people who are learning ASP MVC fall into traps like this.