Reviewing OSS ProjectWhitboard Chat–overall design

time to read 2 min | 374 words

As a reminder, I am reviewing the problems that I found while reviewing the Whiteboard Chat project during one of my NHibernate’s Courses. Here is the 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);

In this post, I’ll just cover some of the easy to notice issues:

The method name is GetLatestPost, but it is actually getting the latest posts (plural). Yes, it is minor, but it is annoying.

Action filter attributes for authorization or transactions shouldn’t be there. To be rather more exact, the [Authorize] filter attribute shows up on all the methods, and transactions with NHibernate should always be used.

Using them as attributes means that you can select to use them or not (and indeed, there are some actions where the [Transaction] attribute is not found).

Stuff that is everywhere should be defined once, and you don’t have to re-define it all over the place.

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