The following code has just been written (never run, never tested).

It’s purpose is to serve as a high speed, no locking transport between two threads, once of them producing information, the other consuming it, in a bounded, non blocking manner.

Note that this is done because the default usage of BlockingCollection<T> here generated roughly 80% of the load, which is not ideal

Originally posted at 3/22/2011

This time, taking on Who Can Help Me, I found some really interesting things there.

For one thing, we got this little guy:

I was all ready to discover INewsRepository –<> NewsRepository : Repository<NewsItem>, etc. What I found, instead was a real service:

That is how it is supposed to be, to be frank. There is a need to abstract something, and we write just enough code to make it work. I would argue with the implementation of this, however, because the approach for multi threading is wrong headed. We shouldn’t spin out a new thread pool work item just to execute the request when the FluentTwitter API already contains async version that can do quite well for us, but the overall concept is sound. And I was relieved not to find nested repositories in my first few steps there.

Of course, I then discovered that this nice service has some friends from the left side of the blanket:

I think that I expressed my opinion about code such as this:

A waste of keystrokes, and highly inefficient to boot. You don’t make queries by Id in NHibernate, you use Get or Load instead.

Overloading the infrastructure – After reviewing the code, I was surprised to see so much of it dedicated for caching:

What surprised me more is that in the entire application there were exactly two locations where a cache was used. In both cases, it led to the same service. Implementing a much simpler solution in that service would have chopped quite a bit of code out of this project.

And then there was this:

Luckily, this is dead code, but I was quite amused by this code, in a “shake you head in disbelief” fashion.

First, for a code that is obviously meant to be used in a multi threaded fashion, it is not thread safe. Second, it is actually a memory leak waiting to happen, more than anything else. If you call that method, your items will never freed.

The next is a personal opinion, but I can’t help feeling that this is pretty heavy weight:

Well, that is true whenever you are looking at an XSD, but in this case, we just need to expose two properties, and I think that it would have been perfectly fine to stick that into the <appSettings/> section. There are actually several places where similar approach has been tried, but I don’t see this of any value if you aren’t writing a library that might require special configuration. If you are writing an app, using the default modes is usually more than enough.

Reading the controllers code wasn’t a real surprise. One thing that did bother me is the amount of mapping that is going on there, and how much of that I was simply unable to follow. For example:

Which is then calling;

Which then goes into additional custom implementations and convention based ones.

The reason that this is important is that this is a prime location for Select N+1 issues, and indeed, we have several such occurrences of the problem just in this piece of code.

Originally posted at 3/22/2011

It has been suggested that I’ll look at a more modern implementation of SharpArchitecture, and I was directed toward the MultiTenant project.

The first thing to notice is the number of projects. It is actually hard to keep the number of projects down, as I well know, but this has several strange choices.

I am not really sure what is the point in separating the controllers into a separate assembly, or why we have a separate project for the ApplicationServices.

I am not the only one thinking so, I think:

Then there is the Core project:

Personally, I wouldn’t create a project for just two files, but I can live with that. I don’t like attributes like DomainSignature. It is hard for me to really say what, except that I think that they encourage a way of thinking that puts the model in the Center of All Things. I am usually much more interested in what something is doing than how it is shaped.

The data project is mostly concerned with setting up NHibernate via Fluent NHibernate.

Next up is the Framework project. And there we run into the following marker interfaces. I really don’t like marker interfaces, and having those here doesn’t seem to be adding anything important to the application.

It seems that there is a lot going on simply to try to get a fallback to a non tenant situation, but the problem here is that it is usually much better to be explicit about those sort of things. You have the CentralSession, and you have the TenantSession, and you are working with each in a different manner. It makes the infrastructure easier to manage and usually result in code that is clearer to follow.

So far, it has all been pretty much uninteresting, I would strongly encourage merging the solution into just two projects, the web & the tests projects, but that is about it.

Now we move into the fancy bits, the controllers project. And there we find the following piece of code:

public class TenantListQuery : NHibernateQuery, ITenantListQuery
{
  public IPagination<TenantViewModel> GetPagedList(int pageIndex, int pageSize)
  {
    var query = Session.QueryOver<Tenant>()
      .OrderBy(customer => customer.Name).Asc;

    var countQuery = query.ToRowCountQuery();
    var totalCount = countQuery.FutureValue<int>();

    var firstResult = (pageIndex - 1) * pageSize;
    TenantViewModel viewModel = null;
    var viewModels = query.SelectList(list => list
                            .Select(mission => mission.Id).WithAlias(() => viewModel.Id)
                            .Select(mission => mission.Name).WithAlias(() => viewModel.Name)
                            .Select(mission => mission.Domain).WithAlias(() => viewModel.Domain)
                            .Select(mission => mission.ConnectionString).WithAlias(() => viewModel.ConnectionString))
      .TransformUsing(Transformers.AliasToBean(typeof(TenantViewModel)))
      .Skip(firstResult)
      .Take(pageSize)
      .Future<TenantViewModel>();

    return new CustomPagination<TenantViewModel>(viewModels, pageIndex, pageSize, totalCount.Value);
  }
}

I quite like this code. It is explicit about what it is doing. There is a good reason to hide this sort of thing behind a class, because while it is easy to read, it is also a lot of detailed code that should be abstracted. I like the use of futures to reduce the number of queries, and that we have explicit paging here. I also like the projection directly into the view model.

What I don’t like is that I really don’t understand how the Session instance is being selected. Oh, I understand how MultiTenantSessionFactoryKeyProvider is working, and that we get the central database because we aren’t using a tenanted entity, but it still seems too much magic here I would rather a CentralSession instead.

Another thin that I liked was the code structure:

All of the code is grouped by feature in a very nice fashion.

My main peeve with the application is that this is basically it. We are talking about an application that is basically two CRUD pages, nothing more. Yes, it is a sample app to show something very specific, but I would have liked to see some more meat there to look at.

Multi tenancy is a hard problem, and this application spend quite a bit of time doing what is essentially connection string management.

I got some feedback about my previous review, that the PetShop 2.0 was recognized as architecturely unsound, and that I should look at version 3.0 of the code, which is:

Version 3.x of the .NET Pet Shop addresses the valuable feedback given by reviewers of the .NET Pet Shop 2.0 and was created to ensure that the application is aligned with the architecture guideline documents being produced by the Microsoft.

I have to say, it looks like someone told the developers, we need an architecture, go build one. The result is... strange. It make my spider sense tingle. I can't really say that it is wrong, but it makes me uncomfortable.

Take a look at the account class, and how it is structured:

Well, I don't know about you, but that is poor naming convention to start with. And I am seeing here an architecture by rote, if this makes any sort of sense.

Then there are such things as:

Which leads us to this:

The MSG_FAILURE is:

I am sorry, but while there was some effort made here over the previous version, I am not really impressed with it. As I said, the architecture is now probably sound, if suspicious because of lack of character, but the implementation is still not really one that I would call decent. I have to admit about a strong bias here, though. I don't like te naked CLR, but the code has missed a lot of opportunities to avoid unnecessary duplication and work.

I have been also asked what I would consider a good sample application, and I would like to recommend Cuyahoga, as the application that probably models my thinking the best. SubText is also good, but it is more interesting case, because I don't like its reliance on stored procedures. Nevertheless, it is a valid approach, and it certainly serving this blog very well.

I gave a talk about ReSharper today, and I used the PetShop demo app as the base code. I have purposefully avoided looking at the source code of the sample until today, because I wanted to get a gueniue experience, rather than a rehearsed one. I don't think it went as well as it could have, but that is not the point of this post. The point is to talk about just the code quality of the PetShop application.

First, let us see what the PetShop application is:

The Microsoft .NET Pet Shop 2.0 illustrates basic and advanced coding concepts within an overall architectural framework

The Pet Shop example is supposed to illustrate "coding concepts", but mostly it demonstrate those that you want to avoid. I was shocked by what I was seeing there.

I am going to touch just the account class, because if has enough issues all on its own. The account class is supposed to be a domain entity, but what do you see when you open it?

And I really don't like to see SQL inside my code.

And then there is this:

And suddenly I am confused, because I see a class that is doing the work of three classes, and that is just by casual browsing.

And then we had this:

Somehow, public fields and violating the .NET naming conventions doesn't really strikes me as a good idea.

Code duplication, like between the Account.Insert() and Account.Update(), both have significant duplication in the form of:

I thought that the whole idea of a sample app was to show of best practices, and that is... quite not so, in this case.