Ayende @ Rahien

My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:


+972 52-548-6969

, @ Q c

Posts: 6,026 | Comments: 44,842

filter by tags archive

Reviewing OSS ProjectWhiteboard Chat

time to read 3 min | 429 words

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:

public ActionResult GetLatestPost(int boardId, string lastPost)
    DateTime lastPostDateTime = DateTime.Parse(lastPost);
    IList<Post> posts = 
        .FindAll(new GetPostLastestForBoardById(lastPostDateTime, boardId))

    //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…

More posts in "Reviewing OSS Project" series:

  1. (15 Mar 2011) Whiteboard Chat–The Select N+1 issue
  2. (14 Mar 2011) Whiteboard Chat–Unbounded Result Sets and Denial of Service Attacks
  3. (11 Mar 2011) Whiteboard Chat–setup belongs in the initializer
  4. (10 Mar 2011) Whitboard Chat–overall design
  5. (09 Mar 2011) Whiteboard Chat



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.

Jason Meckley

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 =


    .FindAll(new GetPostLastestForBoardById(lastPostDateTime, boardId))


inside a Service ?

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

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

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

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

Frank Quednau

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.

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.


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.

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


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

Comment preview

Comments have been closed on this topic.


No future posts left, oh my!


  1. Technical observations from my wife (3):
    13 Nov 2015 - Production issues
  2. Production postmortem (13):
    13 Nov 2015 - The case of the “it is slow on that machine (only)”
  3. Speaking (5):
    09 Nov 2015 - Community talk in Kiev, Ukraine–What does it take to be a good developer
  4. Find the bug (5):
    11 Sep 2015 - The concurrent memory buster
  5. Buffer allocation strategies (3):
    09 Sep 2015 - Bad usage patterns
View all series


Main feed Feed Stats
Comments feed   Comments Feed Stats