Ayende @ Rahien

Reviewing RavenDB app: ReleaseCandidateTracker

Wed, 16 May 2012 09:00:00 GMT

ReleaseCandidateTracker is a new RavenDB based application by Szymon Pobiega. I reviewed version 5f7e42e0fb1dea70e53bace63f3e18d95d2a62dd. At this point, I don’t know anything about this application, including what exactly it means, Release Tracking.

I downloaded the code and started VS, there is one project in the solution, which is already a Good Thing. I decided to randomize my review approach and go and check the Models directory first.

Here is how it looks:

This is interesting for several reasons. First, it looks like it is meant to keep a record of all deployments to multiple environments, and that you can lookup the history of each deployment both on the environment side and on the release candidate side.

Note that we use rich models, which have collections in them. In fact, take a look at this method:

Which calls to this method:

You know what the really fun part about this?

It ain’t relational model. There is no cost of actually making all of these calls!

Next, we move to the Infrastructure folder, where we have a couple of action results and the RavenDB management stuff. Here it how RCT uses RavenDB:

public static class Database
{
    private static IDocumentStore storeInstance;

    public static IDocumentStore Instance
    {
        get
        {
            if (storeInstance == null)
            {
                throw new InvalidOperationException("Document store has not been initialized.");
            }
            return storeInstance;
        }
    }

    public static void Initialize()
    {
        var embeddableDocumentStore = new EmbeddableDocumentStore {DataDirectory = @"~\App_Data\Database"};
        embeddableDocumentStore.Initialize();
        storeInstance = embeddableDocumentStore;
    }
}

It is using an embedded database to do that, which makes it very easy to use the app. Just hit F5 and go. In fact, if we do, we see the fully functional website, which is quite awesome .

Let us move to seeing how we are managing the sessions:

public class BaseController : Controller
{
    public IDocumentSession DocumentSession { get; private set; }
    public CandidateService CandidateService { get; private set; }
    public ScriptService ScriptService { get; private set; }

    protected override void OnActionExecuting(ActionExecutingContext filterContext)
    {
        if (filterContext.IsChildAction)
        {
            return;
        }
        DocumentSession = Database.Instance.OpenSession();
        CandidateService = new CandidateService(DocumentSession);
        ScriptService = new ScriptService(DocumentSession);
        base.OnActionExecuting(filterContext);
    }
    
    protected override void OnActionExecuted(ActionExecutedContext filterContext)
    {
        if (filterContext.IsChildAction)
        {
            return;
        }
        if(DocumentSession != null)
        {
            if (filterContext.Exception == null)
            {
                DocumentSession.SaveChanges();
            }
            DocumentSession.Dispose();
        }
        base.OnActionExecuted(filterContext);
    }
}

This is all handled inside the base controller, and it is very similar to how I am doing that in my own apps.

However, ScriptService and CandidateService seems strange, let us explore them a bit.

public class ScriptService
{
    private readonly IDocumentSession documentSession;

    public ScriptService(IDocumentSession documentSession)
    {
        this.documentSession = documentSession;
    }

    public void AttachScript(string versionNumber, Stream fileContents)
    {
        var metadata = new RavenJObject();
        documentSession.Advanced.DatabaseCommands.PutAttachment(versionNumber, null, fileContents, metadata);
    }

    public Stream GetScript(string versionNumber)
    {
        var attachment = documentSession.Advanced.DatabaseCommands.GetAttachment(versionNumber);
        return attachment != null 
            ? attachment.Data() 
            : null;
    }
}

So this is using RavenDB attachment to store stuff, I am not quite sure what yet, so let us track it down.

This is being used like this:

[HttpGet]
public ActionResult GetScript(string versionNumber)
{
    var candidate = CandidateService.FindOneByVersionNumber(versionNumber);
    var attachment = ScriptService.GetScript(versionNumber);
    if (attachment != null)
    {
        var result = new FileStreamResult(attachment, "text/plain");
        var version = candidate.VersionNumber;
        var product = candidate.ProductName;
        result.FileDownloadName = string.Format("deploy-{0}-{1}.ps1", product, version);
        return result;
    }
    return new HttpNotFoundResult("Deployment script missing.");
}

So I am assuming that the scripts are deployment scripts for different versions, and that they get uploaded on every new release candidate.

But look at the CandidateService, it looks like a traditional service wrapping RavenDB, and I have spoken against it multiple times.

In particular, I dislike this bit of code:

public ReleaseCandidate FindOneByVersionNumber(string versionNumber)
{
    var result = documentSession.Query<ReleaseCandidate>()
        .Where(x => x.VersionNumber == versionNumber)
        .FirstOrDefault();
    if(result == null)
    {
        throw new ReleaseCandidateNotFoundException(versionNumber);
    }
    return result;
}

public void Store(ReleaseCandidate candidate)
{
    var existing = documentSession.Query<ReleaseCandidate>()
        .Where(x => x.VersionNumber == candidate.VersionNumber)
        .Any();
    if (existing)
    {
        throw new ReleaseCandidateAlreadyExistsException(candidate.VersionNumber);
    }
    documentSession.Store(candidate);
}

From looking at the code, it looks like the version number of the release candidate is the primary way to look it up. More than that, in the entire codebase, there is never a case where we load a document by id.

When I see a VersionNumber, I think about things like “1.0.812.0”, but I think that in this case the version number is likely to include the product name as well, “RavenDB-1.0.812.0”, otherwise you couldn’t have two products with the same version.

That said, the code above it wrong, because it doesn’t take into account RavenDB’s indexes BASE nature. Instead, the version number should actually be the ReleaseCandidate id. This way, because RavenDB’s document store is fully ACID, we don’t have to worry about index update times, and we can load things very efficiently.

Pretty much all of the rest of the code in the CandidateService is only used in a single location, and I don’t really see a value in it being there.

For example, let us look at this one:

[HttpPost]
public ActionResult MarkAsDeployed(string versionNumber, string environment, bool success)
{
    CandidateService.MarkAsDeployed(versionNumber, environment, success);
    return new EmptyResult();
}

As you can see, it is merely loading the appropriate release candidate, and calling the MarkAsDeployed method on it.

Instead of doing this needless, forwarding, and assuming that we have the VersionNumber as the id, I would write:

[HttpPost]
public ActionResult MarkAsDeployed(string versionNumber, string environment, bool success)
{
    var cadnidate = DocumentSession.Load<ReleaseCandidate>(versionNumber);
    if (cadnidate == null)
        throw new ReleaseCandidateNotFoundException(versionNumber);
    var env = DocumentSession.Load<DeploymentEnvironment>(environment);
    if (env == null)
        throw new InvalidOperationException(string.Format("Environment {0} not found", environment));

    cadnidate.MarkAsDeployed(success, env);
    return new EmptyResult();
}

Finally, a word about the error handling, this is handled via:

protected override void OnException(ExceptionContext filterContext)
{
    filterContext.Result = new ErrorResult(filterContext.Exception.Message);
    filterContext.ExceptionHandled = true;
}

public class ErrorResult : ActionResult
{
    private readonly string message;

    public ErrorResult(string message)
    {
        this.message = message;
    }

    public override void ExecuteResult(ControllerContext context)
    {
        context.HttpContext.Response.Write(message);
        context.HttpContext.Response.StatusCode = 500;
    }
}

The crazy part is that OnException is overridden only on some of the controllers, rather than in the base controller, and even worse. This sort of code leads to error details loss.

For example, let us say that I get a NullReferenceException. This code will dutifully tell me all about it, but will not tell me where it happened.

This sort of thing make debugging extremely hard.

Reviewing Xenta and wishing I hadn’t

Thu, 10 May 2012 09:00:00 GMT

Xenta Framework is the extensible enterprise n-tier application framework with multilayered architecture.

I was asked by the coordinator for the project to review it.

This isn’t going to take long. I looked at the code, and I got this:

I am sure you remember the last time when I run into something like this, except that the number of projects at that solution was a quarter of what we had here, and I already had to hold my nose.

Looking a bit deeper:

Here is a hint, you are allowed to have more than one class per project.

Having that many project is a nightmare in trying to manage them. Finding things, actually making use of how things work. Not to mention that the level of abstractness required to support that is giving me a headache.

This is still without looking at the code, mind.

Now, let us look at the actual code. I like to start the controllers. The following code is from ForumController:

[HttpPost, ValidateInput(false)]
public ActionResult Update(int forumID, FormCollection form)
{
    ForumModel m = new ForumModel()
    {
        ForumID = forumID
    };

    if(!m.Load())
    {
        return HttpNotFound();
    }

    if(TryUpdateModel<ForumModel>(m, "Model", form))
    {
        try
        {
            m.Update();
        }
        catch(Exception ex)
        {
            ModelState.AddModelError("API", ex);
        }
    }

    if(!ModelState.IsValid)
    {
        TempData.OperationStatus("Failed");
        TempData.PersistObject("Model", m);
        TempData.PersistModelState(ModelState);
    }
    else
    {
        TempData.OperationStatus("Success");
    }

    return RedirectToAction("Edit", new
    {
        ForumID = m.ForumID
    });
}

Seriously, I haven’t seen this style of architecture in a while. Let us dig deeper:

public bool Update()
{
    using(ForumApiClient api = new ForumApiClient())
    {
        var dto = api.UpdateForum(ForumID, ParentForumID, DisplayOrder, Flags);

        return Load(dto);
    }
}

public bool Load()
{
    using(ForumApiClient api = new ForumApiClient())
    {
        var dto = api.GetForum(ForumID);

        return Load(dto);
    }
}

In case you are wondering, those are on the ForumModel class.

This piece of code is from the ForumPostModel class, it may look familiar:

public bool Load()
{
    using(ForumApiClient api = new ForumApiClient())
    {
        var dto = api.GetPost(PostID);

        return Load(dto);
    }
}

This ForumApiClient is actually a WCF Proxy class, which leads us to this interface:

I won’t comment on this any further, but will go directly to ForumApiService, where we actually update a forum using the following code:

public ForumDto UpdateForum(int forumID,
    int parentForumID,
    int displayOrder,
    ForumFlags flags)
{
    ForumService forumService = Infrastructure.Component<ForumService>();

    if(forumService == null)
    {
        throw new FaultException(new FaultReason("forum service"), new FaultCode(ErrorCode.Infrastructure.ToString()));
    }

    try
    {
        ForumEntity entity = forumService.UpdateForum(forumID, parentForumID, displayOrder, flags, DateTime.UtcNow);

        return Map(entity);
    }
    catch(XentaException ex)
    {
         throw new FaultException(new FaultReason(ex.Message), new FaultCode(ex.Code.ToString()));
    }
}

Infrastructure.Component represent a home grown service locator. Note that we have manual exception handling for absolutely no reason whatsoever (you can do this in a behavior once, and since this is repeated for each and every one of the methods…).

I apologize in advance, but here is the full UpdateForum method:

public ForumEntity UpdateForum(int forumID,
    int parentForumID,
    int displayOrder,
    ForumFlags flags, 
    DateTime updatedOn)
{
    ForumEntity oldForum = GetForum(forumID);
            
    if(oldForum == null)
    {
        throw new XentaException(ErrorCode.NotFound, "forum");
    }

    ForumEntity parentForum = null;

    if(parentForumID != 0)
    {
        parentForum = GetForum(parentForumID);

        if(parentForum == null)
        {
            throw new XentaException(ErrorCode.NotFound, "forum");
        }

        ForumEntity tmp = parentForum;

    HierarchyCheck:

        if(tmp.ForumID == forumID)
        {
            throw new XentaException(ErrorCode.InvalidArgument, "parentForumID");
        }
        if(tmp.ParentForumID != 0)
        {
            tmp = tmp.Parent;

            goto HierarchyCheck;
        }
    }

    ForumEntity newForum = null;
    bool res = Provider.UpdateForum(forumID,
        parentForumID,
        oldForum.LastTopicID,
        oldForum.TopicCount,
        oldForum.PostCount,
        displayOrder,
        flags,
        oldForum.CreatedOn,
        updatedOn);

    if(res)
    {
        if(Cache != null)
        {
            Cache.Remove("ForumService_GetForum_{0}", oldForum.ForumID);
        }

        newForum = GetForum(forumID);

        if(EventBroker != null)
        {
            EventBroker.Publish<PostEntityUpdate<ForumEntity>>(this, new PostEntityUpdate<ForumEntity>(newForum, oldForum));
        }
    }
    return newForum;
}

Yes, it is a goto there, for the life of me I can’t figure out why.

Note that there is also this Provider in here, which is an IForumProvider, which is implemented by… ForumProvider, which looks like this:

public bool UpdateForum(int forumID,
    int parentForumID,
    int lastTopicID,
    int topicCount,
    int postCount,
    int displayOrder,
    ForumFlags flags,
    DateTime createdOn,
    DateTime updatedOn)
{
    bool res = false;

    using(DbCommand cmd = DataSource.GetStoredProcCommand("fwk_Forums_Update"))
    {
        SqlServerHelper.SetInt32(DataSource, cmd, "ForumID", forumID);
        SqlServerHelper.SetInt32(DataSource, cmd, "ParentForumID", parentForumID);
        SqlServerHelper.SetInt32(DataSource, cmd, "LastTopicID", lastTopicID);
        SqlServerHelper.SetInt32(DataSource, cmd, "TopicCount", topicCount);
        SqlServerHelper.SetInt32(DataSource, cmd, "PostCount", postCount);
        SqlServerHelper.SetInt32(DataSource, cmd, "DisplayOrder", displayOrder);
        SqlServerHelper.SetInt32(DataSource, cmd, "Flags", (int)flags);
        SqlServerHelper.SetDateTime(DataSource, cmd, "CreatedOn", createdOn);
        SqlServerHelper.SetDateTime(DataSource, cmd, "UpdatedOn", updatedOn);

        res = DataSource.ExecuteNonQuery(cmd) > 0;
    }
    return res;
}

And… this is about it guys.

I checked the source control, and the source control history for this goes back only to the beginning of February 2012. I assume that this is older than this, because the codebase is pretty large.

But to summarize, what we actually have is a highly abstracted project, a lot of abstraction. A lot of really bad code even if you ignore the abstractions, seemingly modern codebase that still uses direct ADO.Net for pretty much everything, putting a WCF service in the middle of the application just for the fun of it.

Tons of code dedicated to error handling, caching, etc. All of which can be handled as cross cutting concerns.

This is the kind of application that I would expect to see circa 2002, not a decade later.

And please note, I actually got the review request exactly 34 minutes ago. I didn’t review the entire application (nor do I intend to). I merely took a reprehensive* vertical slide of the app and followed up on that.

*Yes, this is intentional.

Thoughts after using ASP.Net Web API (beta) in anger for a week

Mon, 12 Mar 2012 08:00:00 GMT

Nitpicker corner: Yes, I know about Open Rasta, NancyFX, FubuMVC and all of the other cool frameworks out there. I am not here today to talk about them. I am not interested in talking about them and why they are so much better in this post.

You might be aware that I am doing a lot of stuff at the HTTP level, owing to the fact that both RavenDB and RavenFS are REST based servers.

As such, I had to become intimately familiar with the HTTP spec, how things work at the low level ASP.Net level, etc. I even had to write my own abstractions to be able to run both inside IIS and as a service. Suffice to say, I feel that I have a lot of experience in building HTTP based system. That said, I am also approaching things from a relatively different angle than most people. I am not aiming to build a business application, I am actually building infrastructure servers.

There was a lot of buzz about the ASP.Net Web API, and I took a brief look at the demo, marked it as “nice, need to check it out some day” in my head and moved on. Then I run into a strange problem in RavenFS. RavenFS is a sibling to RavenDB. Whereas RavenDB is a document database, RavenFS is a distributed & replicated file server for (potentially) very large files. (It is currently in beta testing ,and when we are doing giving it all the bells and whistle of a real product, we will show it to the world , it isn’t really important to this post).

What is important is that I run into a problem with RavenFS, and I felt that there was a strong likelihood that I was doing something wrong in the HTTP layer that was causing this. Despite its outward simplicity, HTTP is pretty complex when you get down to business. So I decided to see what would happen if I would replace the HTTP layer for RavenFS with ASP.Net Web API.

That means that I have been using it in anger for the last week, and here is what I think about it so far.

First, it is a beta. That is something that is important to remember, because it means that it isn’t done yet.

Second, I am talking strictly about the server API. I haven’t even touched the client API as of now.

Third, and most important. I am impressed. It is a really clean API, nice interface, well thought out and quite nice to work with.

More than that, I had to do a bunch of stuff that really isn’t trivial. And there are very little docs for it as of now. I was able to do pretty much everything I wanted by just walking the API and figuring things out on my own.

Things that I particularly liked:

The API guides you to do the right thing. For example, different headers have different meaning, and you can see that when you look at the different headers collections. You have headers that goes in the response, headers that go in the request, headers that goes for the content, and so on. It really guides you properly to using this as you should.
A lot of the stuff that is usually hard is now pretty easy to do. Multi part responses, for example. Ranged requests, or proper routing.
I was able to plug in DI for what I was doing in a couple of minutes without really knowing anything about how things work. And I could do that by providing a single delegate, rather than implement a complex interface.
I provides support for self hosting, which is crucial for doing things like unit testing the server.
It is Async to the core.
I really like the ability to return a value, or a task, or a task of a value, or return an HttpResponseMessage which I can customize to my heart content.

Overall, it just makes sense. I get how it works, and it doesn’t seems like I have to fight anything to get things done.

Please note that this is porting a major project to a completely new platform, and doing some really non trivial things in there while doing this.

Things that I didn’t like with it:

Put simply, errors. To be fair, this isn’t a complaint about the standard error handling, this works just fine. The issue is with infrastructure errors.

For example, if you try to push a 5MB request to the server, by default the request will just die. No error message, and the status code is 503 (message unavailable). This can be pretty frustrating to try to figure out, because there is nothing to tell you what the problem is. And I didn’t look at the request size at first. It just seemed that some request worked, and some didn’t. Even after that I found that it is the size that mattered, it was hard to figure out where we need to fix that (and the answer to that is different depending on where you are running!).

Another example is using PUT or DELETE in your requests. As long as you are running in SelfHost, everything will work just fine. If you switch to IIS, you will get an error (405, Method Not Allowed), again, with no idea how to fix this or why this is happening. This is something that you can fix in the config (sometimes), but it is another error that has horrible usability.

Those are going to be pretty common errors, I am guessing, and any error like that is actually a road block for the users. Having an error code and nothing else thrown at you is really frustrating, and this is something that can really use a good error report. Including details about how to fix the problem.

There are a bunch of other issues that I run into (an NRE when misconfiguring the routing that was really confusing and other stuff like that), but this is beta software, and those are things that will be fixed.

The one thing that I miss as a feature is good support for nested resources (/accounts/1/people/2/notes), which can be a great way to provide additional context for the application. What I actually want to use this for is to be able to do things like: /folders <—FoldersController.Get, Post, Put, Delete and then have: /folders/search <- FolderController.GetSearch., PostSearch, etc.

So I can get the routing by http method even when I am doing controller and action calls.

Final thoughts, RavenFS is now completely working using this model, and I like it. It is really nice API, it works, but most importantly, it makes sense.

Reviewing Postman

Fri, 09 Mar 2012 10:00:00 GMT

I enjoy reading code, and I decided that of a change, I want to read code that isn’t in .NET. The following is a review of Postman:

Postman is a little JavaScript library (well it's actually Coffeescript but the Cakefile handles a build for me) which is similar to a traditional pub/ sub library, just a whole lot smarter.

This is actually the first time that I looked at Coffescript code (beyond cursory glance at the tutorial a time or two). I got to say, it looks pretty neat. Take a look at the definition of a linked list:

Pretty, readable and to the point. I like that.

Then I got to a head scratching piece:

There are various things that refer to postie, but it wasn’t until I got to the bottom of the code that I saw:

So I guess that postie line is actually defining a null argument, so it can be captured by the class Postman class methods.

I’ll be the first to admit that I am not a JS / CoffeeScript guy, so sometimes I am a little slow to figure things out, this method gave me a pause:

It took a while to figure out what is going on there.

The first few lines basically say, skip the first argument and capture the rest, then call all the subscriptions with the new msg.

Note that this is preserving history. So we can do something with this.

There is also an async version of this, confusing called deliverSync.

Getting the notification is done via:

This is quite elegant, because it means that you don’t lose out on messages that have already been published.

I guess that you might need to worry about memory usage, but there seems to be some mechanism to sort that out too. So you can explicitly clean things out. Which works well enough, I guess, but I would probably do some sort of builtin limits for how many msgs it can hold at any one time, just to be on the safe side. I don’t actually know how you would debug a memory leak in such a system, but I am guessing it can’t be fun.

This code makes my head hurt a big, because of the ability to pass a date or a function. I would rather have an options argument here, rather than overloading the parameter. It might be that I am a bad JS / CoffeScript coder and try to impose standards of behavior from C#, though.

All in all, this seems to be a fairly nice system, there is a test suite that is quite readable, and it is a fun codebase to read.

Taking a look at S#arp Lite–final thoughts

Thu, 08 Mar 2012 10:00:00 GMT

This is a review of the S#arp Lite project, the version from Nov 4, 2011.

This project is significantly better than the S#arp Arch project that I reviewed a while ago, but that doesn’t mean that it is good. There is a lot to like, but frankly, the insistence to again abstract the data access behind complex base classes and repositories makes things much harder in the longer run.

If you are writing an application and you find yourself writing abstractions on top of CUD operations, stop, you are doing it wrong.

I quite like S#arp approach for querying, though. You expose things directly, and if it is ugly, you just wrap it in a dedicated query object. That is how you should be handling things.

Finally, whenever possible, push things to the infrastructure, it is usually pretty good and that is the right level of handling things like persistence, validation, etc. And no, you don’t have to write that, it is already there.

A lot of the code in the sample project was simply to manage persistence and validation (in fact, there was an entire project for that) that could be safely deleted in favor of:

public class ValidationListener : NHibernate.Event.IPreUpdateEventListener, NHibernate.Event.IPreInsertEventListener
{
    public bool OnPreUpdate(PreUpdateEvent @event)
    {
        if (!DataAnnotationsValidator.TryValidate(@event.Entity)) 
            throw new InvalidOperationException("Updated entity is in an invalid state");

        return false;
    }

    public bool OnPreInsert(PreInsertEvent @event)
    {
        if (!DataAnnotationsValidator.TryValidate(@event.Entity))
            throw new InvalidOperationException("Updated entity is in an invalid state");

        return false;
    }
}

Register that with NHibernate, and it will do that validation work for you, for example. Don’t try too hard, it should be simple, if it ain’t, you are either doing something very strange or you are doing it wrong, and I am willing to bet on the later.

To be clear, the problems that I had with the codebase were mostly with regards to the data access portions. I didn’t have any issues with the rest of the architecture.

Taking a look at S#arp Lite– The MVC parts

Wed, 07 Mar 2012 10:00:00 GMT

This is a review of the S#arp Lite project, the version from Nov 4, 2011.

Okay, after going over all of the rest of the application, let us take a look a the parts that actually do something.

the following are from the CustomerController:

It is fairly straight forward, all in all. Of course, the problem is that is isn’t doing much. The moment that it does, we are going to run into problems. Let us move a different controller, ProductController and the Index action:

Seems fine, right? Except that in the view…

As you can see, we got a Select N+1 here. I’ll admit, I actually had to spend a moment or two to look for it (hint, look for @foreach in the view, that is usually an indication of a place that requires attention).

The problem is that we really don’t have anything to do about it. If we want to resolve this, we would have to create our own query object to completely encapsulate the query. But all we need is to just add a FetchMany and we are done, except that there is that nasty OR/M abstraction that doesn’t do much except make our life harder.

Taking a look at S#arp Lite, Part III–tasks

Tue, 06 Mar 2012 10:00:00 GMT

This is a review of the S#arp Lite project, the version from Nov 4, 2011.

So far, we have gone over the general structure and the domain. Next on the list is going over the tasks project. No idea what this is. I would expect that to be some sort of long running, background tasks, but haven’t checked it yet.

Unfortunately, this isn’t the case. Tasks seems to be about another name for DAL. And to make matter worse, this is a DAL on top or a Repository on top of an OR/M.

And as if that wasn’t enough to put my teeth on edge, we got some really strange things going on there. Let us see how it goes.

Basically, a CudTask seems to be all about translating from the data model (I intentionally don’t call it domain model) to the view model. I spoke about the issues with repositories many times before, so I’ll suffice with saying that this is still wasteful and serve no real purpose and be done with it.

This TransferFormValuesTo() is a strange beast, and it took me a while to figure out what is going on here. let us look in the parent class to figure out what is going on there.

Let me count the things that are wrong here.

First, we have this IsTransient() method, why do we have that? All we need to do is just to call SaveOrUpdate and it will do it for us. Then the rest of the method sunk in.

The way this system works, you are going to have two instances of every entity that you load (not really true, by the way, because you have leakage for references, which must cause some really interesting bugs). One instance is the one that is managed by NHibernate, dealing with lazy loading, change management, etc. The second is the value that isn’t managed by NHibernate. I assume that this is an instance that you get when you bind the entity view the action parameters.

NHibernate contains explicit support for handling that (session.Merge), and that support is there for bad applications. You shouldn’t be doing things this way. Extend the model binder so it would load the entity from NHibernate and bind to that instance directly. You wouldn’t have to worry about all of this code, it would just work.

For that matter, the same goes for validation as well, you can push that into NHibernate as a listener. So all of this code just goes away, poof!

And then there is the Delete method:

I mean, is there a rule that says “developers should discard error information as soon as possible, because it is useless.” I mean, the next step is to see C# code littered with things like:

catch(Exception e)
{
   delete e; // early release for the memory held by the exception
}

The one good thing that I can say about the CudTasks is that at least they are explicit about not handling reads, and that reads seems to be handled properly so far (but I haven’t looked at the actual code yet).

Taking a look at S#arp Lite, Part II–the domain

Fri, 02 Mar 2012 10:00:00 GMT

This is a review of the S#arp Lite project, the version from Nov 4, 2011.

In my previous post, I looked at the general structure, but not much more. In this one, we are going to focus on the Domain project.

We start with the actual domain:

I have only few comments about this sort of model:

This is a pure CRUD model, which is good, since it is simple and easy to understand, but one does wonder where the actual business logic of the system is. It might be that there isn’t any (we are talking about a sample app, after all).
The few methods that are there are also about data (in this case, aggregation, and Order.GetTotal() will trigger a lazy loaded query when called, which might be a surprise to the caller.
Probably the worst point of this object model is that it is highly connected, which encourages people to try to walk the object graphs where they should issue a separate query instead.

Next, let us look at the queries. We have seen one example where NHibernate low level API was hidden behind an interface, but that was explicitly called out as rare. So how does this get handled on a regular basis?

Hm… I have some issues here with regards to the naming. I don’t like the “Find” vs. “Query” naming. I would use WhereXyz to add a filter and SelectXyz to add a transformation. It would read better when writing Linq queries, but that is about it for the domain.

One thing that I haven’t touched so far is the entities base class:

And its parent:

I strongly support the notion of ComparableObject, this is recommended when you use NHibernate. But what is it about GetTypeSpecificSignatureProperties? What it actually does is select all the properties that has the [DomainSignature] attribute. But what would you want something like that?

Looking at the code, the Customer.FirstName and Customer.LastName have this attribute, looking at the code, I really can’t understand what went on here. This seems to be selected specifically to create hard to understand and debug bugs.

Why do I say that? The ComparableObject uses properties marked with [DomainSignature] for the GetHashCode() calculation. What this means is that if you change the customer name you change its hash code value. This hash code value is used for, among other things, finding the entity in the unit of work, so changing the customer name can cause NHibernate to loose track of it and behave in some really strange ways.

This is also violating one of the core principals of entities:

A thing with distinct and independent existence.

In other words, an entity doesn’t exists because of the particular values that are there for the first and last names. If those change, the customer doesn’t change. It is the same as saying that by changing the shirt I wear, I becomes a completely different person.

Domain Signature is something that I am completely opposed, not only for the implementation problems, but because it has no meaning when you start to consider what an entity is.

Next, we are going to explore tasks…

Taking a look at S#arp Lite, Part I

Thu, 01 Mar 2012 10:00:00 GMT

This is a review of the S#arp Lite project, the version from Nov 4, 2011.

I was asked to review this project a long time ago, but I never got around to it. I had some time and I decided that I might take a look and see how it goes. I don’t like the S#arp Arch project, because it seems too complex and heavy weight for the purpose.

The project comes with a sample application, which is good, because it is easy to see how the framework is intended to be used. Unfortunately, it is yet another online store example, I am getting heartily sick of that. On the other hand, it is a fairly simple model and easy to understand, so I grok why this keeps getting chosen.

Review Rule, I look at the code. If I wanted to deal with documentation, I would write some for our products. I am doing this because I find it fun to look at other people’s code. So skip any comments about “if you read the docs…”.

We start from the project structure:

I am not sure if I like it, I don’t know if I agree that all of those splits are needed, but this is well within reasonable limits, so I am willing to let it slide on the grounds that this is personal taste more than anything else. Looking at the dependencies, we see:

The Init project contains two files, which are responsible for… well, starting up, it seems. Again, I don’t see any reason why this would be a separate project, but that is about it so far.

Next in line is the NHibernateProvider project, in this case, we have the following:

So far, I am cautiously optimistic. All of the files / folders marked with red are actually all about setting NHibernate up, not about hiding it. But then we get to the read me file, which reads in part:

This folder contains any concrete, NHibernate-specific query classes.
There should only be classes in here for any respective query *interfaces* found in
/MyStore.Domain/Queries/

This folder will usually be empty except for very exceptive cases.

This is… interesting. Can’t say whatever I agree or not yet. Looking at the QueryForProductOrderSummaries, we see:

Note the comment, there are better ways to do it, but we demonstrate an ugly way, and how to nicely encapsulate it.

That is enough for now, I think, next post, I’ll touch on the actual model…

Northwind Starter Kit Review: Conclusion

Thu, 26 Jan 2012 10:00:00 GMT

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

A while ago I said:

Seriously?! 22(!) projects to do a sample application using Northwind?

And people took me up to task about it. The criticism was mostly focused on two parts:

I didn’t get that the project wasn’t about Northwind, but about being a sample app for architectural design patterns.
I couldn’t actually decide that a project was bad simply by looking at the project structure and some minor code browsing.

I am sad to say that after taking a detailed look at the code, I am even more firmly back at my original conclusion. I started to do a review of the UI code, but there really is no real need to do so.

The entire project, as I said in the beginning, is supposed to be a sample application for Northwind. Northwind is a CRUD application. Well, not exactly, it is supposed to be an example of an Online Store, which is something much bigger than just Northwind. But it isn’t.

Say what you will, the Northwind Starter Kit is a CRUD application. It does exactly that, and nothing else. It does so in an incredibly complicated fashion, mind, but that is what it does.

Well, it doesn’t do updates, or deletes, or creates. So it is just an R application (I certainly consider the codebase to be R rated, not for impressionable developers).

If you want to have a sample application to show off architectural ideas, make sure that the application can actually, you know, show them. The only thing that NSK does is loading stuff from the database, try as I might, I found no real piece of business logic, no any reason why it is so complicated.

So, to the guys who commented on that, it isn’t a good project. If you like it, I am happy for you, there are also people who loves this guy:

Personally, I would call pest control.

Northwind Starter Kit Review: That CQRS thing

Tue, 24 Jan 2012 10:00:00 GMT

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

It is obvious from reading the code that there was some attention given to CQRS. Unfortunately, I can’t really figure out what for.

To start with, both the Read Model and the Domain Model are actually sitting on the same physical location. If you are doing that, there is a 95% chance that you don’t need CQRS. If you have that, you are going to waste a lot of time and effort and are very unlikely to get anything from it.

In the case of NSK, here is the domain model vs. the read model for the customer.

I marked the difference.

I am sorry, there is nothing that justify a different model here. Just needless complexity.

Remember, our job is to make things simpler, not make it hard to work with the application.

Northwind Starter Kit Review: It is all about the services

Mon, 23 Jan 2012 10:00:00 GMT

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

Okay, enough about the data access parts. Let us see take a look at a few of the other things that are going on in the application. In particular, this is supposed to be an application with…

Domain logic is implemented by means of a Domain Model, onto a layer of services adds application logic. The model is persisted by a DAL designed around the principles of the "Repository" patterns, which has been implemented in a LINQ-friendly way.

Let us try to figure this out one at a time, okay?

The only method in the domain model that have even a hint of domain logic is the CalculateTotalIncome method. Yes, you got it right, that is a method, as in singular. And that method should be replaced with a query, it has no business being on the domain model.

So let us move to the services, okay? Here are the service definitions in the entire project:

Look at the methods carefully. Do you see any action at all? You don’t, the entire thing is just about queries.

And queries should be simple, not abstracted away and made very hard to figure out.

The rule of the thumb is that you try hard to not abstract queries, it is operations that you try to abstract. Operations is where you usually actually find the business logic.

Northwind Starter Kit Review: From start to finishing–tracing a request

Fri, 20 Jan 2012 10:00:00 GMT

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

One of the things that I repeatedly call out is the forwarding type of architecture, a simple operation that is hidden away by a large number of abstractions that serves no real purpose.

Instead of a controller, let us look at a web service, just to make things slightly different. We have the following:

Okay, let us dig deeper:

I really like the fact that this repository actually have have FindById method, which this service promptly ignores in favor of using the IQueryable<Customer> implementation. If you want to know how that is implemented, just look (using the EF Code First repository implementations, the others are fairly similar):

All in all, the entire thing only serves to make things harder to understand and maintain.

Does anyone really think that this abstraction adds anything? What is the point?!

Northwind Starter Kit Review: If you won’t respect the database, there will be pain

Wed, 18 Jan 2012 10:00:00 GMT

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

The database is usually a pretty important piece in your application, and it likes to remind you that it should be respected. If you don’t take care of that, it will make sure that there will be a lot of pain in your future. Case in point, let us look at this method:

It looks nice, it is certainly something that looks like a business service. So let us dig down and see how it works.

It seems like a nice thing, the code is clear, and beside the bug where you get 100% discount if you buy enough and the dissonance between the comment and the code, fairly clear. And it seems that we have service logic and entity logic, which is always nice.

Except that this piece of code issues the following queries (let us assume a customer with 50 orders).

1 Query to load the customer, line 34 in this code. And now let us look at line 35… what is actually going on here:

Okay, so we have an additional query for loading the customer’s orders. Let us dig deeper.

And for each order, we have another query for loading all of that order’s items. Does it gets worse?

Phew! I was worried here for a second.

But it turns out that we only have a Select N+2 here, where N is the number of orders that a customer has.

What do you want, calculating the discount for the order is complicated, it is supposed to take a lot of time. Of course, the entire thing can be expressed in SQL as:

SELECT 
  SUM((UnitPrice * Quantity) * (1 - Discount) Income
FROM OrderItems o
WHERE o.OrderID in (
  SELECT Id FROM Orders
  WHERE CustomerId = @CustomerId
)

But go ahead and try putting that optimization in. The architecture for the application will actively fight you on that.

Northwind Starter Kit Review: Refactoring to an actual read model

Mon, 16 Jan 2012 10:00:00 GMT

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

In my previous post, I talked about how the CatalogController.Product(id) action was completely ridiculous in the level of abstraction that it used to do its work and promised to show how to do the same work on an actual read model in a much simpler fashion. Here is the code.

There are several things that you might note about this code:

It is all inline, so it is possible to analyze, optimize and figure out what the hell is going on easily.
It doesn’t got to the database to load data that it already has.
The code actually does something meaningful.
It only do one thing, and it does this elegantly.

This is actually using a read model. By, you know, reading from it, instead of abstracting it.

But there is a problem here, I hear you shout (already reaching for the pitchfork, at least let me finish this post).

Previously, we have hidden the logic of discontinued products and available products behind the following abstraction:

Now we are embedding this inside the query itself, what happens if this logic changes? We would now need to go everywhere we used this logic and update it.

Well, yes. Except that there are two mitigating factors.

This nice abstraction is never used elsewhere.
It is easy to create our own abstraction.

Here is an example on how to do this without adding additional layers of abstractions.

Which means that our action method now looks like this:

Simple, easy, performant, maintainable.

And it doesn’t make my head hurt or cause me to stay up until 4 AM feeling like an XKCD item.

A meta post about negative code reviews

Sun, 15 Jan 2012 10:00:00 GMT

A lot of people seems to have problems whenever I post a code review. The general theme of the comments is mostly along the lines of:

You are an evil person and a cyber bully to actually do those sort of things and humiliate people.
You have something against the author of the personally, and you set out to avenge them.
There is no value in doing a negative code review.
You never do any good reviews, only bad ones.
You only tells us what is wrong, but not what is right.

There are a bunch of other stuff, but those are the main points.

For point 1 & 2, I have the same answer:

I talk about the code, not the person. I am actually very careful with my phrasing whenever I do this sort of a review. The code or the architecture is wrong, not the person. There is a difference, and a big one.

For point 3:

Most of those code reviews are generated because someone asks me to do them. And given some of the responses to the posts, I feel that they serve a very good purpose. Here is one such comment. I redacted the project name, because it doesn’t really matter, but the point stands:

I grew up on *** as it was my first layered application, almost 5 years ago and I personally believe that the effort ****** has put into this project during the years is simply amazing. The architecture reflect the most common architectural design patterns and represents almost the concepts expressed by Fowler and Evans. *** is not a project to teach you how to work with Northwind and it is not a project designed exclusively for Nhb. It is designed to show how a layered application should be architected in .NET and there is also a book wrote around this project.

My professional opinioned, backed by over a decade of practical experience and work in the field tells me that the project in question is actually not a good template for an application. And I feel that by pointing out exactly why, I am doing a service to the community.

Look, it is actually quite simple. The major reason that I do those negative code reviews is because I keep seeing the same types of mistakes repeated over and over again at customer sites. And the major reason is that those projects follow best practices as they see it. The problem is that they usually ignore the context of those best practices, so it becomes a horrible mess.

What is worse, there is the issue of the non coding architect, when you have someone that doesn’t actually have responsibility for the output making decisions about how it is going to be built. And those things are actually hard to fight against, precisely because they are considered to be best practices. One of the reasons that I am pointing out the problems in those projects is to serve as a reference point for other people when they need a way to escape the over architecture.

For point 4:

It takes something out of the ordinary to get me to actually post something to the blog. The barrier for a negative code review is how much is annoys me. The barrier for a positive code review is how much it impresses me. It is easier to annoy me to impress me, admittedly, but I quite frequently do good code reviews.

The problem is that most good code bases are actually fairly boring. That is pretty much the definition of a good codebase, of course , so there isn’t really that much to talk about.

For point 5:

That usually come up when I do negative code reviews, “you only show the bad stuff, it doesn’t teach us how to do it right”. Well, that is pretty much the point of a negative code review. To show the bad stuff so you would know how to avoid it. There are literally thousands of posts in this blog, and quite a few of them are actually devoted to discussions on how to do it right. I have very little inclination to repeat that advice again in every post.

Even a blog post must obey the single responsibility principle.

Northwind Starter Kit Review: Data Access review thoughts

Fri, 13 Jan 2012 10:00:00 GMT

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

In my last few posts, I have gone over the data access strategy used in NSK. I haven’t been impressed. In fact, I am somewhere between horrified, shocked and amused in the same way you feel when you see a clown slipping on a banana peel. Why do I say that? Let us trace a single call from the front end all the way to the back.

The case in point, CatalogController.Product(id) action. This is something that should just display the product on the screen, so it should be fairly simple, right? Here is how it works when drawn as UML:

To simplify things, I decided to skip any method calls on the same objects (there are more than a few).

Let me show you how this looks like in actual code:

Digging deeper, we get:

We will deal with the first method call to CatalogServices now, which looks like:

I’ll skip going deeper, because this is just a layer of needless abstraction on top of Entity Framework and that is quite enough already.

Now let us deal with the second call to CatalogServices, which is actually more interesting:

Note the marked line? This is generating a query. This is interesting, because we have already loaded the product. There is no way of optimizing that, of course, because the architecture doesn’t let you.

Now, you need all of this just to show a single product on the screen. I mean, seriously.

You might have noticed some references to things like Read Model in the code. Which I find highly ironic. Read Models are about making the read side of things simple, not drowning the code in abstraction on top of abstraction on top of abstraction.

In my next post, I’ll show a better way to handle this scenario. A way that is actually simpler and make use an of actual read model and not infinite levels of indirection.

Northwind Starter Kit Review: The parents have eaten sour grapes, and the children’s teeth are set on edge

Thu, 12 Jan 2012 10:00:00 GMT

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

In my previous posts, I focused mostly on the needlessness of the repositories implementation and why you want to avoid that (especially implementing it multiple times). In this post, I want to talk about other problems regarding the data access. In this case, the sudden urge to wash my hands that occurred when I saw this:

I mean, you are already using an OR/M. I don’t like the Repository implementation, but dropping down to SQL (and unparapetrized one at that) seems uncalled for.

By the way, the most logical reason for this to be done is to avoid mapping the Picture column to the category, since the OR/M in use here (EF) doesn’t support the notion of lazy properties.

Again, this is a problem when you are trying to use multiple OR/Ms, and that is neither required nor really useful.

Okay, enough about the low level data access details. On my next post I’ll deal with how those repositories are actually being used.

Northwind Starter Kit Review: Data Access and the essence of needless work, Part II

Wed, 11 Jan 2012 10:00:00 GMT

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

Update: Andrea, the project’s leader has posted a reply to this series of posts.

Yes, this is another repositories are evil if you are using an OR/M post.

That is probably going to cause some reaction, so I am going to back this up with code from this NSK project. Let us talk about repositories, in particular. Let us see what we have here:

Okaay…

Now here are a few problems that I have with this:

There is no value gained by introducing this abstraction. You aren’t adding any capability what so ever.
In fact, since all OR/Ms provide an abstraction that isn’t dependent on type, creating IRepository<T> and things like ICustomerRepository is just making things more complicated.
There are going to be changes in behavior between different repositories implementations that will break your code.

Let us see what we actually have as a result. This is the Entity Framework POCO implementation:

You can probably guess how the rest of it is actually implemented. Yes, we have a LOT of code that is dedicated solely for this sort of forwarding operations.

And then we have the actual implementation of the delete:

Just to remind you, here is the NHibernate implementation of the same function:

Leaving aside the atrocious error handling code, the EF POCO version will do an immediate delete. The NHibernate version will wait for the transaction to be committed.

And don’t worry, I do remember the error handling. This is simply wrong.

And then we have implementations such as this:

This is for the Entity Framework Code First implementation. There is a message here that is coming to me loud and clear. This code wants to be deleted. It is neglected and abused and doesn’t serve any purpose in life except gobble up pieces of valuable disk space that could be filled with the much more valuable result of reading from/dev/random.

Northwind Starter Kit Review: Data Access and the essence of needless work, Part I

Tue, 10 Jan 2012 10:00:00 GMT

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

Update: Andrea, the project’s leader has posted a reply to this series of posts.

I like to start reviewing applications from their database interactions. That it usually low level enough to tell me what is actually going on, and it is critical to the app, so a lot of thought usually goes there.

In good applications, I have hard time finding the data access code, because it isn’t there. It is in the OR/M or the server client API (in the case of RavenDB). In some applications, if they work against legacy databases or without the benefit of OR/M or against a strange data source (such as a remote web service target) may need an explicit data layer, but most don’t.

NSK actually have 5 projects dedicated solely to data access. I find this.. scary.

Okay, let me start outlying things in simple terms. You don’t want to do things with regards to data access the way NSK does them.

Let us explore all the ways it is broken. First, in terms of actual ROI. There is absolutely no reason to have multiple implementations with different OR/Ms. There is really not a shred of reason to do so. The OR/M is already doing the work of handling the abstraction from the database layer, and the only thing that you get is an inconsistent API, inability to actually important features and a whole lot more work that doesn’t' get you anything.

Second, there are the abstractions used:

I don’t like repositories, because they abstract important details about the way you work with the database. But let us give this the benefit of the doubt and look at the implementation. There is only one implementation of IRepository, which uses NHibernate.

As you can see, this is pretty basic stuff. You can also see that there are several methods that aren’t implemented. That is because they make no sense to a data. The reason they are there is because IRepository<T> inherits from ICollection<T>. And the reason for that is likely because of this:

Mediates between the domain and data mapping layers using a collection-like interface for accessing domain objects.

The fact that this is totally the wrong abstraction to use doesn’t enter to the design, it seems.

Note that we also violate the contract of ICollection<T>.Remove:

true if item was successfully removed from the ICollection<T>; otherwise, false. This method also returns false if item is not found in the original ICollection<T>.

There are other reasons to dislike this sort of thing, but I’ll touch on that on my next post.