One of the things that we really pride ourselves with Hibernating Rhinos is the level of support. Just to give you some idea, today & yesterday we had core team members on the phone with people (not customers, yes) who are having problems with RavenDB for quite some time.

Now, I understand that you may not always have the information to give, but what you have, give me! So I can help you.

From a recent exchange in the mailing list:

var clickCount = session.Query<TrackerRequest>().Where(t => t.TrackerCreated == doc.Created).Where(t => t.Type == Type.Click).Count();

This gives:

"Non-static method requires a target"

To which I replied:

What is the full error that you get? Compile time? Runtime?

The answer I got:

This is a document I'm trying to 'Count':

{

  "Created": "2012-11-15T16:12:42.1775747",

  "IP": "127.0.0.1",

  "TrackerCreated": "2012-11-15T14:12:16.3951000Z",

  "Referrer": "http://example.com",

  "Type": "Click"

}

Raven terminal gives:

Request # 172: GET     -     3 ms - <system>   - 200 - /indexes/Raven/DocumentsByEntityName?query=Tag%253ATrackerRequests&start=0&pageSize=30&aggregation=None&noCache=-1129797484

        Query: Tag:TrackerRequests

        Time: 2 ms

        Index: Raven/DocumentsByEntityName

        Results: 3 returned out of 3 total.

By the way, you might note that this ISN’T related in any way to his issue. This query (and document) were gotten from the Studio. I can tell by the URL.

Then there was this:

I mean, seriously, I am happy to provide support, even if you aren’t a customer yet, but don’t give me some random bit of information that has absolutely nothing to do to the problem at hand and expect me to guess what the issue is.

Relevant information like the stack trace, what build you are on, what classes are involved, etc are expected.

One of the things that I really hate is to be reminded anew how stupid some people are. Or maybe it is how stupid they think I am.  One of the things that we are doing during interviews is to ask candidates to do some fairly simple code tasks. Usually, I give them an hour or two to complete that (using VS and a laptop), and if they don’t complete everything, they can do that at home and send me the results.

This is a piece of code that one such candidate has sent. To be clear, this is something that the candidate has worked on at home and had as much time for as she wanted:

public int GetTaxs(int salary)
{
    double  net, tax;

    switch (salary)
    {
        case < 5070:
            tax = salary  * 0.1;
            net=  salary  - tax ;
            break;

        case < 8660:
        case > 5071:
            tax = (salary - 5071)*0.14;
            tax+= 5070 * 0.1;
            net = salary-tax;   
            break;
        case < 14070:
        case > 8661:
            tax=  (salary - 8661)*0.23;
            tax+= (8661 - 5071 )*0.14;
            tax+= 5070 *0.1;
            net=  salary - tax;
            break;
        case <21240:
        case >14071:
            tax=  (salary- 14071)*0.3;
            tax+= (14070 - 8661)*0.23;
            tax+= (8661 - 5071 )*0.14;
            tax+= 5070 *0.1;
            net= salary - tax;
            break;
        case <40230:
        case >21241:
            tax=  (salary- 21241)*0.33;
            tax+= (21240 - 14071)*0.3;
            tax+= (14070 - 8661)*0.23;
            tax+= (8661 - 5071 )*0.14;
            tax+= 5070 *0.1;
            net= salary - tax;
            break;
        case > 40230:
            tax= (salary - 40230)*0.45;
            tax+=  (40230- 21241)*0.33;
            tax+= (21240 - 14071)*0.3;
            tax+= (14070 - 8661)*0.23;
            tax+= (8661 - 5071 )*0.14;
            tax+= 5070 *0.1;
            net= salary - tax;
            break;
        default:
            break;
    }
}

Submitting code that doesn’t actually compiles is a great way to pretty much ensures that I won’t hire you.

Originally posted at 2/23/2011

Yes, I know that the NHibernate documentation says that you should override Equals and GetHashCode, but I get the feeling that this wasn’t what the documentation authors intended:

I found myself reading this post, and at some point, I really wanted to cry:

We had relatively long, descriptive names in MySQL such as timeAdded or valueCached. For a small number of rows, this extra storage only amounts to a few bytes per row, but when you have 10 million rows, each with maybe 100 bytes of field names, then you quickly eat up disk space unnecessarily. 100 * 10,000,000 = ~900MB just for field names!

We cut down the names to 2-3 characters. This is a little more confusing in the code but the disk storage savings are worth it. And if you use sensible names then it isn’t that bad e.g. timeAdded -> tA. A reduction to about 15 bytes per row at 10,000,000 rows means ~140MB for field names – a massive saving.

Let me do the math for a second, okay?

A two terabyte hard drive now costs 120 USD. By my math, that makes:

• 1 TB = 60 USD
• 1 GB = 0.058 USD

In other words, that massive saving that they are talking about? 5 cents!

Let me do another math problem, oaky?

Developer costs about 75,000 USD per year.

• (52 weeks – 2 vacation weeks) x 40 work hours = 2,000 work hours per year.
• 75,000 / 2,000 = 37.5 $ / hr
• 37.5 / 60 minutes = 62 cents per minutes.

In other words, assuming that this change cost a single minute of developer time, the entire saving is worse than moot.

And it is going to take a lot more than one minute.

Update: Fixed decimal placement error in the cost per minute. Fixed mute/moot issue.

To those of you pointing out that real server storage space is much higher. You are correct, of course. I am trying to make a point. Even assuming that it costs two orders of magnitudes higher than what I said, that is still only 5$. Are you going to tell me that saving the price of a single cup of coffee is actually meaningful?

To those of you pointing out that MongoDB effectively stores the entire DB in memory. The post talked about disk size, not about memory, but even so, that is still not relevant. Mostly because MongoDB only requires indexes to fit in memory, and (presumably) indexes don't really need to store the field name per each indexed entry. If they do, then there is something very wrong with the impl.

My current mood is somewhere between mad, pissed off, frustrated and exhausted.

All I want to do it arrive home, but dark forces has conspired against me. It works like this:

• I got a ticket both ways to London, and I left to London quite happily, taking the train to the airport. At that point things started going horribly wrong. An idiot has suicided on the train trucks, leading to about 2 hours of delays, which, naturally, caused me to miss my flight. We will leave that aside and just point out that I somehow managed to get to London without being late to my course the next morning.
• That is when I heard that since I was a no show for the outgoing flight, the airline cancelled my flight back.  I had to order a new one, but because I was busy actually doing the course, I asked the company to order that for me.
• They did, and I went to the airport quite satisfied, except that on arrival, the stupid machine told me that my booking is invalid, which is the point that I discovered that the ticket was ordered on Ayende Rahien. Unfortunately, legally speaking, that guy doesn’t exists. The airline was quite insistent that they can’t put me on board with the different name. And they made me buy a new ticket.

That was the point (after I paid for the same bloody seat for the third time) that they told me that they oversold the flight, and that they put me on waiting list.

I tried to explain that they could use the second ticket that I bought, and just give me the same seat, but they told me that they already sold that to someone else.

I am currently waiting to hear whatever I’ll get to go home tonight or not.

Your truly,

Pissed off,

Oren Eini (because being Ayende is currently dangerous, given my mood)

There are no other owners for the group, I have no idea what is going on…

Update: Looks like Google disabled my account, for reasons known only to it. I had to go through a “send me an SMS to activate it” loop, but now it is working.

And this one hits a lot of the high notes in that regard:

Three properties to say the same thing?!

This showed up on my mail box, with the following text:

When i try to insert an item to db, VS gives a stackoverflow exception. is it because of that i am using generic classes??

I am not going to comment further, I am stepping away from the keyboard and going to have a lie down.

There are evil people in this world, and some of them want access to my database. Unfortunately, they are often part of that nasty integration team and they try to integrate directly into my database. I tried beating them with clubs and lobbing arguments about letting other people mess with my implementation details, but they have been persistent. That is when I reached out to a technological solution for the problem.

I want to emphasize that this is the nuclear option and  you want to really consider it before going that route.

We are going to use NHibernate to do that, naturally. Here is how:

Which results in the following schema:

create table [tbl_-1434067361] (
   [col_287061521] INT IDENTITY NOT NULL,
   [col_4699698] INT not null,
   [col_-1966747349] NVARCHAR(255) null,
   [col_-649855325] NVARCHAR(255) null,
   [col_-649855326] NVARCHAR(255) null,
   [col_775690683] NVARCHAR(255) null,
   [col_-2127361396] NVARCHAR(255) null,
   [col_-1334581412] NVARCHAR(255) null,
   primary key ([col_287061521])
)

To make sure that we follow procedure, we are even using the naming convention of the organization! Lovely, isn’t it? It is obvious that this is my people table, right?

All arguments against this schema can be answered using: “It is more secured”.

It might be the nuclear option, but it tend to work :-)

Nitpicker corner: No, I don’t suggest you would do this, that is why the code is an image. This is just an example of how to programmatically modify the configuration and mapping. I only used it once, during a demo to the integration people, to get them to leave my DB alone. If you push something like that for production you should probably be shot.

In general, I have been very happy with PayPal over the years. Recently, however, I have become quite frustrated with it. I am using it quite often to send and accept money to people I am working with, and for a while now, I have been unable to do so.

Apparently, my actions has been caught by some automated security systems and they decided to ban any transactions on my account. That is actually acceptable from my point of view. I feel better knowing that there are active attempts to protect me from fraud or theft.

What I don’t find acceptable is that when I called PayPal to try to resolve that, the response that I got was: It is an act of God, nothing we mere mortals can do about it.

I understand all about false positives, and aside from the annoyance factor, they are acceptable if they aren’t happening very often. What I am not able to comprehend is responses such as: No one in PayPal can help you. And that is after I already escalated to a supervisor!

I recorded my conversation with PayPal, you can download it here or just listen:

I find the whole thing utterly infuriating, and totally unacceptable.

Sometimes, you have to make a distinction between the system architecture and the system implementation. It is possible to have a great architecture and have the implementers butcher it by not paying attention at some points.

Sometimes, the problem in the code implementation is so bad that it becomes an architectural problem. Kobe is one such example.

We can start with the pervasive Primitive obsession that is all over the map. The common use of “string thisIsReallyGuid”, Copy & paste programming, horrible Exception Handling and total lack of respect for the fundamentals of OO.

All of those tie back together to the lack of architectural oversight over the project. I have the feeling that at no point someone stopped and said: “there has got to be a better way of doing this.”

DRY is a true principle in development, it transcend programming languages and paradigms, about the only one that I can think of that does so. If you violate DRY, you are in for a world of trouble, sooner than later.

SOLID is a set of principles used to build good OO applications. If you are using OO language and paradigms and fail to pay attention to SOLID, it will hurt you at some point. Not as soon as if you have violated DRY, but soon enough.

8.5% of Kobe is copy & pasted code. And that is with the sensitivity dialed high, if we set the threshold to 3, which is what I commonly do, is goes up to 12.5%. A lot of the duplicated code relates to exception handling and caching. Those are classic cases for using an aspect to handle cross cutting concerns. That would reduce a lot of the duplicated code.

The other problems that I have with Kobe is its total lack of respect for SOLID principles. You know what, let us throw solid out the window, I am going to focus on a single principle, Single Responsibility Principle. Just about any method that you care to name does more than one thing, in many cases, they do a lot more than one thing.

This is absolutely wrong.

The “repositories” in Kobe has so many responsibilities that they are just shy of having a singularity around them.

Overall, Kobe feels very amaturish, as if it was written without true understanding of what is expected of quality code, and without really understanding the underlying platform, frameworks or the infrastructure that they are using.

I don’t like it at all, and it is a horrible sample application. In my considered opinioned, it should be pulled from the site, rewritten, and only then published again.

In Kobe, pretty much all exception handling is this:

catch (Exception ex)
{
    bool rethrow = ExceptionPolicy.HandleException(ex, "Service Policy");
    if (rethrow && WebOperationContext.Current == null)
    {
        throw;
    }
    return null;
}

These lines appear in just about any method in the application.

They are wrong. In fact, they are wrong on more than one level. Violating DRY is a big offence, but even ignoring that, they are still wrong.

Exception handling is not something that you should take lightly, and there is a strong tie between the way that you write the code and the way errors are handled. The whole purpose of this exception handling is to avoid throwing an exception by setting some configuration parameter.

The problem with this is that this purpose is wrong. This is a case of writing the code without actually trying to understand what the implications are. You cannot do this sort of thing, because the calling code never expects a null, it expect a valid result or an exception.

This approach bring us back to the bad old days of error codes. Not to mention that even if we actually checked for a null return value, what the hell is the error that actually caused this to happen.  Oh, it was put in the log, so now we need to go and look at the log file and try to match timestamps (if we even can).

Exception handling should appear in exactly two places:

• When an error is expected (making a web request call, for example) and there is some meaningful behavior to be done in the case of a failure (such as retrying after some delay)
• On a system boundary, in which case you need to make a decision about how you are going to expose the error to the outside world.

It is important to note that I explicitly used the term system boundary, instead of service boundary, which is more commonly used. Exception handling within a system should not try to hide errors. Error should propagate all the way to the system boundary, because otherwise we lose a lot of important information.

System boundary is defined as whatever is visible to the actual user or other systems.

Oh, and exception handling in the system boundary is a system infrastructure problem, it is not something that you have to write code for all over the place.

When comparing the Kobe code base to the Kobe documentation, I am starting to get a sinking feeling. The documentation is really nice, the code is anything but.

When you put out guidance, the documentation is nice, but it is the code that people are going to look it, learn and try to emulate. And when you put out something that doesn’t meet basic parameters for good software, that is a huge problem.

With Oxite, Microsoft had the excuse of it being slipped out without anyone noticing. This time it is official, it has passed the proper review procedure, and is explicitly marketed as “intended to guide you with the planning, architecting, and implementing of Web 2.0 applications and services.”

Sorry, that is entirely unacceptable.

Putting something like that out there as guidance is actively doing harm to the community as a whole. It means that people who would look at that and try to use what they see there are going to get bitten by bad practices, bad architecture and overly complex and redundant approaches.

Phil Haack said that: “I was part of a high-level review and I felt most of the app that I saw was reasonably put together.”

I submit that high level review of such guidance packages is absolutely not sufficient. You have to get people to do a full review cycle of that, which include the code, the documentation, the reasoning and anything else that is going out there.

From the Kobe documentation:

The Repository pattern is applied to mediate access to the data store through a Repository provider.

From PoEAA:

A system with a complex domain model often benefits from a layer, such as the one provided by Data Mapper (165), that isolates domain objects from details of the database access code. In such systems it can be worthwhile to build another layer of abstraction over the mapping layer where query construction code is concentrated. This becomes more important when there are a large number of domain classes or heavy querying. In these cases particularly, adding this layer helps minimize duplicate query logic.

I think that Repository is a fad, which is why I don’t use it much anymore myself, and looking at the usage semantics of the “repositories” in Kobe, they certainly don’t fit the bill.

Repositories are here to encapsulate query logic, that is not the case with Kobe. Here is a partial view of some of the repositories that it have:

In general, looking at the any of the repositories in detail, I really want to cry. I mean, just take a look:

More to the point, let us look at GroupRepository.AcceptGroupInvite (I removed all the copy & paste crap to concentrate on the actual code):

GroupInvite groupInvite = _context.GroupInviteSet
                                 .Where(gi => gi.GroupInviteId == groupInviteId)
                                 .FirstOrDefault();

if (groupInvite == null)
    throw new DataException("Invalid Invite Id", null);

groupInvite.InvitationStatuses = _context.InvitationStatusSet
                                        .Where(s => s.InvitationStatusName == "Accepted")
                                        .FirstOrDefault(); ;

_context.SaveChanges(true);

There are so many things wrong with this approach that I don’t even know where to start.

You can see how the architects or developers resented having to use an ORM. If they couldn’t have stored procedures in the database, then by Jove and his keyboard, they will write stored procedures in code.

Kobe uses Entity Framework for data access, and they treat it as if it was a dataset. As an OR/M aficionado, I am insolated. This shows complete lack of understanding how Entity Framework work, doing a lot more work than you should, brute force & big hammer approach.

From an architectural perspective, most of what Kobe is doing is calling down to the repository to perform the data access details. Business logic, such as there is, is spread between the data access, the controllers and the service layer, in such a way that make is very hard to have a good idea about what is actually happening in the application.

Data access details are quite pervasive throughout the application, and the whole feeling of the application is of a circa 2001 sample app using stored procedures and hand rolled data access layers.

There is another ASP.Net MVC sample app, this time it is official, passed the proper review procedure, and is explicitly marketed as “intended to guide you with the planning, architecting, and implementing of Web 2.0 applications and services.”

I am saying all of that in order to distinguish it from Oxite, which was non of this things. There have been a couple of reviews of Kobe already. Frankly, I don’t really care for them, mostly because I think that they dealt too much with nitty gritty details of the app that doesn’t really matter much. I don’t much care for extra using or the use of System.Int32 vs. int, the naming convention used or even what sort of HTML formatting they used. I mostly care about the code and architecture. So I decided to take a look myself.

This post is going to go over the nuts & bolts, looking at low level coding details. I am going to have another that is going to look at the architecture and probably a few others that talk about some additional concerns that I have.

Let us start by looking at the code via some tools. Simian reports:

And that is when the similarity threshold is 6 lines, I usually run it with three, if we try that, we get:

Found 5138 duplicate lines in 873 blocks in 67 files

Next, the most important code metric to me in most cases, Cyclomatic Complexity. Two methods literally jump out of the solution and beg for mercy.

• HomeController.Discovery with CC index of 35(!)
• GroupController.Profile with CC index of 22

I am going to show them both in all their glory, and let you be the judge of them:

public ActionResult Discovery(string query)
{
    query = query.Trim();

    query = Server.HtmlEncode(query);

    List<PresentationSummary> presentationsAll = null;
    List<PresentationSummary> presentationsMostPopular = null;
    List<PresentationSummary> presentationsMostViewed = null;
    List<PresentationSummary> presentationsMostDownload = null;
    List<PresentationSummary> presentationsNew = null;

    List<UserSummary> activeusers = null;
    List<UserSummary> allusers = null;
    List<Group> activegroups = null;
    List<Group> allgroups = null;

    int iNewMemberPageCount = 0;
    int iNewGroupPageCount = 0;
    int ipresentationsAll = 0; 
    int ipresentationsMostPopular =  0;
    int ipresentationsMostViewed = 0;
    int ipresentationsMostDownload = 0;
    int ipresentationsNew = 0;

    UserSummary user = _userService.GetUserByName(query);
    if (user != null)
    {
        presentationsAll = _userService.GetAuthoredPresentations(user.UserName);
        presentationsMostPopular = presentationsAll.OrderByDescending(p => p.Favorites).ToList();
        presentationsMostViewed = presentationsAll.Where(p => p.Views > 0).OrderByDescending(p => p.Views).ToList();
        presentationsMostDownload = presentationsAll.Where(p => p.Downloads > 0).OrderByDescending(p => p.Downloads).ToList();
        presentationsNew = presentationsAll.OrderByDescending(p => p.InsertedDate).ToList();

        ipresentationsAll = decimal.Divide(presentationsAll.Count, 10).ToInt();
        ipresentationsMostPopular = decimal.Divide(presentationsMostPopular.Count, 10).ToInt();
        ipresentationsMostViewed = decimal.Divide(presentationsMostViewed.Count, 10).ToInt();
        ipresentationsMostDownload = decimal.Divide(presentationsMostDownload.Count, 10).ToInt();
        ipresentationsNew = decimal.Divide(presentationsNew.Count, 10).ToInt();

        activeusers = new List<UserSummary> { user };
        allusers = new List<UserSummary> { user };
        iNewMemberPageCount = decimal.Divide(allusers.Count, 8).ToInt();

        allgroups = _userService.GetGroups(user.UserName);
        activegroups = allgroups.OrderByDescending(g => g.InsertedDate).ToList();
        iNewGroupPageCount = decimal.Divide(allgroups.Count, 8).ToInt();
    }
    else
    {
        Group group = _groupService.GetGroupByName(query);
        if (group != null)
        {
            presentationsAll = _groupService.GetPresentations(group.GroupName);
            presentationsMostPopular = presentationsAll.OrderByDescending(p => p.Favorites).ToList();
            presentationsMostViewed = presentationsAll.Where(p => p.Views > 0).OrderByDescending(p => p.Views).ToList();
            presentationsMostDownload = presentationsAll.Where(p => p.Downloads > 0).OrderByDescending(p => p.Downloads).ToList();
            presentationsNew = presentationsAll.OrderByDescending(p => p.InsertedDate).ToList();

            ipresentationsAll = decimal.Divide(presentationsAll.Count, 10).ToInt();
            ipresentationsMostPopular = decimal.Divide(presentationsMostPopular.Count, 10).ToInt();
            ipresentationsMostViewed = decimal.Divide(presentationsMostViewed.Count, 10).ToInt();
            ipresentationsMostDownload = decimal.Divide(presentationsMostDownload.Count, 10).ToInt();
            ipresentationsNew = decimal.Divide(presentationsNew.Count, 10).ToInt();

            allusers = _groupService.GetMembers(group.GroupName);
            activeusers = allusers.OrderByDescending(u => u.DateOfJoining).ToList();
            iNewMemberPageCount = decimal.Divide(allusers.Count, 8).ToInt();

            allgroups = new List<Group> { group };
            activegroups = new List<Group> { group };
            iNewGroupPageCount = decimal.Divide(allgroups.Count, 8).ToInt();
        }
        else
        {
            presentationsAll = _presentationService.GetAllPresentationsByKewordTimeLine(query, "Day", "0", 10, 1);
            presentationsMostPopular = _presentationService.GetMostPopularPresentationsByKewordTimeLine(query, "Day", "0", 10, 1);
            presentationsMostViewed = _presentationService.GetMostViewedPresentationsByKewordTimeLine(query, "Day", "0", 10, 1); 
            presentationsMostDownload = _presentationService.GetMostDownloadedPresentationsByKewordTimeLine(query, "Day", "0", 10, 1); 
            presentationsNew = _presentationService.GetNewPresentations(query, "Day", "0", 10, 1);

            ipresentationsAll = decimal.Divide(_presentationService.GetAllPresentationsByKewordTimeLine(query, "Day", "0").Count, 10).ToInt();
            ipresentationsMostPopular = decimal.Divide(_presentationService.GetMostPopularPresentationsByKewordTimeLine(query, "Day", "0").Count, 10).ToInt();
            ipresentationsMostViewed = decimal.Divide(_presentationService.GetMostViewedPresentationsByKewordTimeLine(query, "Day", "0").Count, 10).ToInt();
            ipresentationsMostDownload = decimal.Divide(_presentationService.GetMostDownloadedPresentationsByKewordTimeLine(query, "Day", "0").Count, 10).ToInt();
            ipresentationsNew = decimal.Divide(_presentationService.GetNewPresentations(query, "Day", "0").Count, 10).ToInt();

            activeusers = _userService.GetMostActiveUsers(query, 8, 1);
            allusers = _userService.GetAllUsers(query, 8, 1);
            iNewMemberPageCount = decimal.Divide(_userService.GetMostActiveUsers(query).Count,8).ToInt();

            activegroups = _groupService.GetMostActiveGroupByKeyword(query, 8, 1);
            allgroups = _groupService.GetAllGroupByKeyword(query, 8, 1);
            iNewGroupPageCount = decimal.Divide(_groupService.GetMostActiveGroupByKeyword(query).Count, 8).ToInt();
        }
    }

    ViewData.Add("membersList-mostactive", activeusers);
    ViewData.Add("membersList-all", allusers);
    ViewData.Add("groupsList-mostactive", activegroups);
    ViewData.Add("groupsList-all", allgroups);

    ViewData.Add("presentations-all",presentationsAll);
    ViewData.Add("presentations-mostpopular",presentationsMostPopular);
    ViewData.Add("presentations-mostviewed",presentationsMostViewed);
    ViewData.Add("presentations-mostdownload",presentationsMostDownload);
    ViewData.Add("presentations-new",presentationsNew);
   
    ViewData.Add("Query", query);
    //ViewData.Add("Presentations", presentations);

    ViewData.Add("members-totalcount", iNewMemberPageCount);
    ViewData.Add("groups-totalcount", iNewGroupPageCount);

    ViewData.Add("presentations-alltotalcount", ipresentationsAll);
    ViewData.Add("presentations-mostpopulartotalcount", ipresentationsMostPopular);
    ViewData.Add("presentations-mostviewedtotalcount", ipresentationsMostViewed);
    ViewData.Add("presentations-mostdownloadtotalcount", ipresentationsMostDownload);
    ViewData.Add("presentations-newtotalcount", ipresentationsNew);

    return View();
}

This is… a very busy method, I must say. But in a way, the Profile method is much worse:

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Profile(string gname, string type, string section, string subSection, string page)
{
    if (string.IsNullOrEmpty(gname))
        return new ContentResult { Content = "" };

    if (type != "widget")
        return new ContentResult { Content = "" };

    Group group = null;

    try
    {
        group = _groupService.GetGroupByName(gname);
    }
    catch (Exception)
    {
        return new ContentResult { Content = "" };
    }

    if (group == null)
    {
        return new ContentResult { Content = "" };
    }

    string groupName = group.GroupName;

    AddUserLevelToViewData(groupName);

    int pageNo = 1;
    Int32.TryParse(page, out pageNo);
    if (pageNo == 0)
        pageNo = 1;

    if (section == "div-GroupPresentations")
    {
        List<PresentationSummary> presentations = null;

        switch (subSection)
        {
            case "div-GroupPresentations-RecentltAdded":
                presentations = _groupService.GetRecentlyAddedPresentations(groupName, 5, pageNo);
                break;
            case "div-GroupPresentations-MostViewed":
                presentations = _groupService.GetMostViewedPresentations(groupName, 5, pageNo);
                break;
            case "div-GroupPresentations-MostDownloaded":
                presentations = _groupService.GetMostDownloadedPresentations(groupName, 5, pageNo);
                break;
            case "div-GroupPresentations-All":
                presentations = _groupService.GetPresentations(groupName, 5, pageNo);
                break;
        }

        return View("PresentationsList", presentations);
    }
    else if (section == "div-GroupWall")
    {
        switch (subSection)
        {
            case "div-GroupWall-Messages":
                ViewData["GroupMessages"] = _groupService.GetMessages(groupName, 5, pageNo);
                return View("GroupMessageList", ViewData["GroupMessages"]);
            case "div-GroupWall-MemberRequests":
                ViewData["GroupJoiningRequests"] = _groupService.GetGroupJoiningRequests(groupName, 5, pageNo);
                return View("GroupJoiningRequestList", ViewData["GroupJoiningRequests"]);
        }
    }
    else if (section == "div-GroupInfoExtended")
    {
        switch (subSection)
        {
            case "div-GroupInfoExtended-GroupMembers":
                ViewData["GroupMembers"] = _groupService.GetMembers(groupName, 4, pageNo);
                return View("MembersList", ViewData["GroupMembers"]);
        }
    }

    return new ContentResult { Content = "" };
}

Just look at the code. I thought that the whole point of MVC was to separate the logic from the view. Having the view strongly tied to the controller output is fine by me, but having the controller strongly tied to the HTML format of the page? That isn’t right.

Another thing that isn’t right is HomeController.Index():

public ActionResult Index()
{
    GetFeaturedPresentations(); //*** Dummy call the the Database to activate the Connection.
    List<PresentationSummary> featured = _presentationService.GetFeaturedPresentations();
    List<PresentationSummary> beingViewed = _presentationService.GetPresentationRecentlyViewed();
    List<PresentationSummary> mostDownloaded = _presentationService.GetMostDownloadedPresentation();
    PresentationSummary presentationOfDay = _presentationService.GetPresentationOfDay();

    ViewData.Add("FeaturedPresentations", featured.ToArray());
    ViewData.Add("RecentlyViewedPresentations", beingViewed.ToArray());
    ViewData.Add("MostDownloadedPresentations", mostDownloaded.ToArray());
    ViewData.Add("presentationsOfDay", presentationOfDay);
    ViewData["Tags"] = _presentationService.GetPopularTags();

    return View();
}

Notice the first call?

private void GetFeaturedPresentations()
{
    try {
        //*** Dummy call to the Presentation Service to get the Featured presentations,
        //*** this call is place because, an exception thrown from the Data layered on first hit to the DB (failed to open DB) 
        //*** and the second hit to the DB gets success.
        _presentationService.GetFeaturedPresentations();
    }
    catch (Exception)
    { /*do nothing with this exception*/ }
}

I really like it when you work around a bug instead of actually fix it.

Moving on, let us look at the service layer. Most of it looks like this:

public ADs GetAdById(string adId)
{
    try
    {
        string key = "Ads-" + adId;
        ADs data = cacheService.Get<ADs>(key);
        if (data == null)
        {
            data = provider.GetAdById(adId.ToGuid());
            if (data != null && data.Image != null)
            {
                cacheService.Add(key, data as Object, null, DateTime.MaxValue, new TimeSpan(0, 10, 0), System.Web.Caching.CacheItemPriority.Normal, null);
            }
        }
        return data;

    }
    catch (Exception ex)
    {
        bool rethrow = ExceptionPolicy.HandleException(ex, "Service Policy");
        if (rethrow && WebOperationContext.Current == null)
        {
            throw;
        }
        return null;
    }
}

I am not joking about all of them looking alike, by the way, it is obviously has been cut & paste a lot.

Nitpick, “data as Object” – that is not something you often see. But this is even better (from CacheService):

I like how we have a cacheService but we coupled its interface with System.Web.Caching, or the fact that most of this code is just a very long winded way of calling GetAdById.

But wait, I spared you the method documentation, which is a real masterpiece:

/// <summary>
/// Returns Advertisment.
/// </summary>
/// <param name="adId"> GUID of an Advertisment</param>
/// <returns>Ads</returns>
public ADs GetAdById(string adId)

Yes, the adId is a string which is a guid. We are so lucky to work with VARIANT again, right?

Let us take another method, just to see what is going on in a typical method inside the project. I intentionally avoid the ones that we already looked at. I took a peek at CommunityController and found the Index method:

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Index(string type, string section, string subSection, string page)
{
    if (type != "widget")
        return new ContentResult { Content = "" };

    int pageNo = 1;
    Int32.TryParse(page, out pageNo);
    if (pageNo == 0)
        pageNo = 1;

    if (section == "members")
    {
        List<UserSummary> users = null;

        switch (subSection)
        {
            case "members-new":
                users = _communityService.GetNewUsers(8, pageNo);
                break;
            case "members-mostactive":
                users = _communityService.GetMostActiveUsers(8, pageNo);
                break;
            case "members-all":
                users = _communityService.GetAllUsers(8, pageNo);
                break;
            default:
                users = _communityService.GetAllUsers(8, pageNo);
                break;
        }

        return View("MembersList", users);
    }
    else if (section == "groups")
    {
        List<Group> groups = null;

        switch (subSection)
        {
            case "groups-new":
                groups = _communityService.GetNewGroups(8, pageNo);
                break;
            case "groups-mostactive":
                groups = _communityService.GetMostActiveGroups(8, pageNo);
                break;
            case "groups-all":
                groups = _communityService.GetAllGroups(8, pageNo);
                break;
            default:
                groups = _communityService.GetAllGroups(8, pageNo);
                break;
        }

        return View("GroupsList", groups);
    }
    else if (section == "favourites")
    {
        List<PresentationSummary> favouritePresentation = _communityService.GetCommunityFavorite(10, pageNo);
        return View("PresentationsView", favouritePresentation.ToArray());
    }


    return new ContentResult { Content = "" };
}

Let me see how many things I can find in a cursory examination:

• Hard coding galore
• Long method
• Complex method
• Controller method return several different views

And note that I am still not even trying for the architectural concepts or code quality metrics. That I’m going to leave to another post.

Frankly, I am seeing way too much bad things in the code to overview all of them. I am going to stop with a goodie, though.

Let us explore GroupRepository.GetGroup, shall we?

 private Group GetGroup(Guid groupId, KobeEntities Context)
 {
     try
     {
         Group group = Context.GroupSet
                          .Where(g => g.GroupId == groupId)
                          .FirstOrDefault();

         if (group == null)
             throw new DataException("Invalid Group Id", null);

         return group;
     }
     catch (Exception ex)
     {

         bool rethrow = ExceptionPolicy.HandleException(ex, "Data Policy");
         if (rethrow)
         {
             throw;
         }
         return null;
     }
 }

On the face of it, except for the repeated stupid error handling, there doesn’t seems to be something wrong here, right?

Take note for the different parameter casing on the GetGroup, though. Why is KobeEntities PascalCase? Well, that is because there is also a property called Context on the GroupRepository that you might use by accident. So, what is this Context parameter all about? GetGroup is a private method, who is calling it?

Here is one such callsite:

 public void AddGroupInterests(Guid groupId, string[] interests, Guid userId)
 {
     try
     {
         KobeEntities _context = Context;

         Group group = GetGroup(groupId, _context);
         User user = GetUser(userId, _context);

So, we take the Context property, put it in a _context local variable. Then we pass it to GetGroup, which uses it.

I must say that I am at a loss to understand what was going on in the mind of whoever wrote it. Was he trying to optimize the number of time we access the property?

As I said, I am currently writing a few other posts about Kobe, this was just to get to see the code itself, so we have an impression about its quality.

I am… not impressed.

I just want to cry:

XHEO responded to my open letter. This is my running commentary during reading their post.

Let us start from this:

While I can understand the frustrations of a developer under the gun from a manager, or anxious to meet a deadline we really can't work with so little information - who can?

Here is some of the email exchanges that went by.

Note that every email with an attachment is including details such as screen shots, assemblies, crash dumps, etc.

I want to point out that I did use their tool to generate a support request with all the details about my system that they could possibly want. I submit all exceptions to them as they came, although I run into several instances where the crash was so severe that the error reporting itself fail to kick in.

I know that customers world wide have been conditioned to expect refunds for any reason at any time.

No, customers has grown used to companies respecting signed contracts.

Looking at the timeline that XHEO provides, it seems that they totally ignored the attachments that I sent, which contained full reproductions of the actual problems. That is strange to say, because during my conversation with support, we continually referred to those screen shots, so I fail to see how they can ignore them or claim that I sent an email with just "it doesn't work either".

In fact, here is that particular email:

He is also missing the part that I did updated to the latest version, only to find other bugs:

At this point, I have spent over a week trying to resolve this issue, has been forced to wait for days to get someone from support, got not even a hint of resolution, but what seems like a flurry of "let us try to turn off this setting and see if this works".

I was already investing way too much time into the product, and it was holding back my own work on NH Prof, not to mention the adverse affect on my ability to actually ship something.

I want to specifically respond to this:

The post includes issues never reported to support and in far more detail then ever provided to us. Demonstrating Ayende is quite capable for expressing the information but simply chose not to.

I don't just keep screen shots of broken software around, I extract the images that I have shown in the original post from the emails that I sent them.

XHEO also provide more of the email correspondence that went between us. I recommend that you would read them. Part 1 | Part 2

Someone in the comments pointed out that doing the entire exchange over email was likely a factor. I agree. And I said so:

I never got a reply to this email.

And finally, there is this:

• We have not ignored the terms of the contract and continue to honor our obligations and offer support.

I honestly don't even know how to approach this statement. The contract that we both signed clearly states that their product should work. It doesn't, and they have failed to provide me with a working product. Hence, we fall back to the refund alternative, which they refuse, thereby breaching our contract.

It continues to amaze me, the length some people will go to in order to add additional complexity. Let us take this article: Two-Tier Service Application Scenario (REST). I will leave aside the arguments about this article gross misrepresentations of what terms like domain modeling, entities and REST. Greg already called the DDD argument, and Colin is working on the problems with the representation of REST.

What I want to talk about is friction. I am looking at the code that is shown in the article and I cringed. Hard. Do you want to tell me that it is recommended that people would write code like this?

I am assuming that I would need to write one of those for each of my “entities” I honestly cannot figure out why. Why create specialized behavior when it would be easier, simpler, cheaper and better to handle this generically?

Is there any sort of value in this code? I don’t think so.

It gets better when you see what you need to do in your Application_Start.

So now, not only do I have to create those handlers by hand, I now need to take care to register each them. I’ll leave aside the bug in the routing code vs. the employee route handler code (‘employee’ is not a route value), to focus on a more important subject. Even if I decided that I want to code my way to insanity, why do I give a single class two responsibilities (creating EmployeeHandler and EmployeesHandler).

I am not even trying to ask why I need the route handler in the first place. It seems like it is there just so there would be another layer in the architecture diagram. It looks like that was a common requirement, because here comes the next step toward the road of useless code:

Except for creating additional work for the developer, I cannot think of a single reason why I would want to write this type of code unless I was paid by the line count.

Let me see how many things I can find here that are going to add friction:

• As written, you are going to need one of those for each of the “entities” that you have. I assume that this is so that all the other per “entity” types wouldn’t feel lonely. On last count, We had:
• EmployeeRepository
• EmployeeHandler
• EmployeesHandler
• EmployeeRouteHandler
• EmployeeTranslator
• EmployeeScript
• EmployeeFacade
• Mapping “command” whatever that is, to method calls? So every time that I have to add a new method, I have to touch how many places?
• Why on earth do I need to do explicit error handling? That is why I have exceptions for. Those should do the Right Thing and I should not have to explicitly manage errors unless I know about something specific happening.

Oh, and I saved the best for last. Please take a look at the beast. And unlike the story, there is no beauty.

I truly find it hard to find a place to start.

• Magic numbers all over the place.
• Promote(object[] data) ?! Are we in the stone age again? I really hoped that by 2009 we would be able to get to grip with the notion of meaningful method parameters! For crying out load, you can use the ASP.Net MVC binder to do the work, you don’t have to do it yourself.
• Null reference exception that are just waiting to happen.
• Unless PositionEnum is not an enum (a WTF all on its own), then the code wouldn’t even compile! Enums are value types, you cannot use ‘as’ with them.
• busErrors ?
• First of all, what bus?
• More importantly, are we back in the good ole days of return codes? I thought we were beyond that already!
• Really bad resource management. C# has try/catch/finally for a reason. If an exception is thrown, you are going to leak the transactions. This is truly sad since the text is very careful to point out that you MUST dispose of those resources before you return.

As I said, I am not going to even approach the actual guidance that is offered there. I think that it is invalid as best and likely to be harmful.

From the code sample shown, I would surmise that no one actually sat down and actually coded any sort of system with this. Even the most basic system would crumble under the sheer weight of architecture piled on top of the poor system.

I am saddened and disappointed to see such a thing being presented as guidance from the P&P.

I just tried to to do a major revamp of Rhino Mocks' interface. It was intended to make it easier to work with C# 3.0 and use the smarter compiler to get better syntax.

I shouldn't have bothered. Take a look at this.

	public class TestCsc
	{
		public static void TestMethod()
		{
			Execute(Bar); // fail to compile
			Execute(delegate(int ia, string x) { }); // compiles fine
			Execute((int i, string x) => { return; }); // Compiles fine
			Execute((int i, string x) => { return true; }); // fail to compile
			Execute(Foo);// fail to compile
			Execute(delegate(int ia, string x) { return true; }); // fail to compile
		}

		public static bool Foo(int ia, string x)
		{
			return false;
		}

		public static void Bar(int ia, string x)
		{
		}

		public static void Execute<T, K>(Action<T, K> e)
		{
			
		}

		public static void Execute<T, K>(Func<bool, T, K> e)
		{

		}
	}

Annoyed.

I mentioned before that MS CRM's decision to build all its external API based on web services (that cannot be accessed outside of ASMX proxies) was a big mistake. It is a mistake because you are handing the developer a really clumsy, stupid, API.

I just spent an hour trying to figure out how to set a value to null. An hour!

It couldn't be as simple as this:

customer.new_employeeintouch = null;

It has to be:

customer.new_employeeintouch = new Lookup();
customer.new_employeeintouch.IsNull = true;
customer.new_employeeintouch.IsNullSpecified = true;

Expression Design is supposed to be able to import and export PSD files. PSD is one of the more common file formats, because it is the one that Photoshop uses.

However, the import capability that Expression Design has is limited to... getting the embedded bitmap inside the PSD file. That one is supposed to let you get a thumbnail over PSD files, not as an import tool.

The problem with that is that PSD files are not bitmaps, they are composed of layers, transforms, and a host of other things that I am probably forgetting. Saying that you can import a PSD file by just importing the bitmap is effectively lying.

It may be true in the strict sense of the word, but it is misleading by not specifying that the comparability is so limited as to be useless.

Glenn Block has posted to the ALT.Net mailing list that the P&P team are working on a dependency injection application block.

At this point, I am not really sure what to think. ObjectBuilder was bad. I am still having to deal with fallout from that failure.

The OSS community already has several mature IoC products.

Just in case Microsoft has missed them:

1/ Castle Windsor

2/ Structure Map

3/ Spring.NET

 

If they want to decouple themselves from an IoC implementation, there is a way to do that already in .Net. It is called IServiceProvider, and is already supported by the IoC implementation. A default implementation can be done with 15 lines of code, if they want to provide one.

I would really like to know, why the hell do we need another duplication of OSS?

• Using pszSqlString prefix in .NET - BAD
• Using a naming convention where the company name is prefixed to all local variables - BAD
• 6 lines of code to log an exception - BAD
• Rewriting basic parts of the framework to cover bad coding on your parts - BAD

Here is the result of doing an Add Web Reference on the CRM end point, using the Hebrew version.

This is taking localization to lala land!

Hi, my name is Oren, and I am stupid. I forgot that select() ain't broken holds even if I wrote the select().

Let me tell you the story. We are using Rhino ETL to do ETL stuff, and one of the tasks that we had to do was importing an 80 MB file into the CRM, including all sorts of transformations and joins along the way. Until we got to that part, it was working very well, but with the 80 MB file, it read that and then appeared to be hung.

Rhino ETL is a new project, and it is heavily multi threaded. Since the only way that I know to test multi threading is by test of time, I wasn't overly confident that it got it perfectly right, and it looked like the 80 MB file was triggerring some threading issues. After debugging the issue futilely for a while, and after careless applying of lock() with reckless abandon also failed to do the trick, I decided that I had no alternatives, but to rip out the homegrown threading solution that I had there in favor of something with more robustness.

I choose Retlang for that, and I am really happy with the library, but that isn't the story that I have to tell today. As any who dealt with threading knows, they are complex, nasty and hard. Trying to follow ten threads at the same time in not helpful by any meaning of the word. And trying to replace the threading solution with a radically different model is hard. I have a post or two on big refactoring as a result of that, but again, this isn't the story that I have to tell.

After basing Rhino ETL on Retlang, and getting enough of the tests to pass in a satisfactory manner, I turned back to the real issue at hand, and tried to run the 80 MB file through the new Rhino ETL.

It. Showed. The. Exact. Same. Problem.

At that point, I was on the phone, speaking with the hiring managers about getting a thousand monkeys to type the information in, but I kept tapping the keyboard, probably out of a force of a habit. I couldn't believe that two such radically different implementations would exhibit the same problem. I added logging and tracked down everything. I even added color coded logs, just to be sure that I am seeing everything.

I tracked it down to a log statement that basically said:

Join CustomerWithFoodTypeLookup out 0 rows, Left 103,123 rows, Right 0 rows.

Take a look at this, and try to tell me what the problem is. 

We are joining an empty set to a non empty set, giving... an empty set.

The fix, if you can call it that, was to change the join to:

if Left.FooTypeId = Left.OldFoodTypeId or Right.FooTypeId is null

I rewrote the threading layer because I couldn't track down the root cause of a inner join instead of an left outer join. When I found that, I literally got up and started choking the developer responsible for the script. Then I went and hit myself on the head, repeatedly.