Ayende @ Rahien

It's a girl

Reviewing OSS Project: Whiteboard Chat

I am currently giving a course, and one of the things that we do during the course is put an OSS project on the board and analyze it. The project for this course is Whiteboard Chat Project.

Overall, it seems to be a nice project, there are some problems, but most of them are “rich men’s problems”, the kind of problems that are sort of good to have. That said, I intend to write a series of blog posts on the following method:

[Transaction]
[Authorize]
[HttpPost]
public ActionResult GetLatestPost(int boardId, string lastPost)
{
    DateTime lastPostDateTime = DateTime.Parse(lastPost);
    
    IList<Post> posts = 
        _postRepository
        .FindAll(new GetPostLastestForBoardById(lastPostDateTime, boardId))
        .OrderBy(x=>x.Id).ToList();

    //update the latest known post
    string lastKnownPost = posts.Count > 0 ? 
        posts.Max(x => x.Time).ToString()
        : lastPost; //no updates
    
    Mapper.CreateMap<Post, PostViewModel>()
        .ForMember(dest => dest.Time, opt => opt.MapFrom(src => src.Time.ToString()))
        .ForMember(dest => dest.Owner, opt => opt.MapFrom(src => src.Owner.Name));

    UpdatePostViewModel update = new UpdatePostViewModel();
    update.Time = lastKnownPost; 
    Mapper.Map(posts, update.Posts);

    return Json(update);
}

This method is a pretty good example of a multitude of anti patterns and other issues that annoys me.

That said, I would like to clarify that I am merely using this method as an example of some bad issues, I wouldn’t want to give the impression that this is any sort of attack of the project or its authors. I have literally looked at the project for the first time today, and I haven’t even checked who the authors are.

More on that, in the next posts, and in the meantime, I’ll let you figure out how many issues there are there that I am going to talk about…

Comments

jonnii
03/09/2011 07:18 PM by
jonnii

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...

Dmitry
03/09/2011 07:41 PM by
Dmitry

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.

Jason Meckley
03/09/2011 07:59 PM by
Jason Meckley

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

Enrique
03/09/2011 08:00 PM by
Enrique

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.

Daniel Lang
03/09/2011 08:12 PM by
Daniel Lang

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

Bartek Szafko
03/09/2011 08:15 PM by
Bartek Szafko
  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

Frank Quednau
03/09/2011 08:28 PM by
Frank Quednau

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.

Jimmy Bogard
03/09/2011 09:15 PM by
Jimmy Bogard

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

Frank Quednau
03/09/2011 09:51 PM by
Frank Quednau

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

Chris
03/09/2011 10:09 PM by
Chris

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.

NC
03/09/2011 10:49 PM by
NC

@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.

Kyle Nunery
03/10/2011 02:18 AM by
Kyle Nunery

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.

Rob
03/10/2011 01:26 PM by
Rob

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.

NC
03/10/2011 01:48 PM by
NC

"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.

WilliamH
03/10/2011 02:08 PM by
WilliamH

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.

Phil
03/10/2011 04:01 PM by
Phil

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

Mike Stokes
03/10/2011 07:17 PM by
Mike Stokes

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.

Ayende Rahien
03/10/2011 09:14 PM by
Ayende Rahien

Rob,

Precisely the reason that I was very careful to point out what my intentions were in the post.

Comments have been closed on this topic.