Coding Efficiency

Today a new trainee was assigned to me. He just completed his in-firm training in another company and applied for a job.

In order to find out what he knows, I thought it would be a good idea to go through the code of some tools.

We began with a tool that prints invoices. It has two Forms and no architecture at all. Although the code was a mess, he could easily tell how it worked.

After that, we looked at a tool that queried some data, applied some validation to it and if necessary corrected it. This tool used the command pattern for validation / correction and loaded those commands via reflection (about 50 in total). It was way harder for him to tell what was going on. He asked why anyone would do things this complicated and suggested to use a switch block instead.

The third tool was a little more complicated, the UI used the passive view pattern, persistence was dealt with by NHibernate, StructureMap wired it all up and a test suite covered most parts. I wrote it some time ago but could still easily navigate through it as it was well structured but he was totally overwhelmed and unable to say anything other than “it is too complicated”.

Using these good practices has advantages but these advantages are not free.

There are way more developers like my trainee that never heard about good practices and the like than those who heard about these practices let alone applying them. These developers usually just want to get the job done and mostly they do so, on time.

If you work in a very small IT department that only deals with in house stuff, should you use patterns, unit testing and other good practices or should you just do it “the good old way”?

If you do it “the good old way” maintenance costs increase, time to bug fix / feature are less predictable yet everyone should be able work with it.

If you use good practices, maintaining your applications either can’t be done by every “developer from the street” or requires high upfront learning effort. On the other side, after this initial learning phase is over, the maintenance costs should decrease and time to bug fix / feature should be more predictable.

This is in reply to Ayendes post.

For those who haven’t followed the discussion, it all began with the release of NHibernate 2.1.

Shortly thereafter, Patrick Smacchia made this post where he analyzed what had changed in NHibernates code base and measured its quality with NCover. He came to the conclusion that it must be a daily pain to maintain the code.

In reply to that, Ayende said that it is structured, although not by name spaces and the like (Patrick’s analyses made that assumption) but different. To underline that NHibernate is indeed maintainable, he pointed out that several new committers joined last year, that there were many new features while at the same time fewer issues.

Frans Bouma made a comment where he said that even a mess looks like something acceptable to those who work with it. This lead to Ayendes post to which I respond.

I have to say, I agree with Ayende. Rating how maintainable a code base is can only be done by those who actually maintain it.

If you have to deal with an application written in a language you don’t know, would you say it’s unmaintainable just because you’re not familiar with the language? Code bases are to some extent like that as many, especially larger ones, have their very own conventions and those conventions aren’t limited to naming conventions and the like but can span the structural or architectural level too.

The VB6 application I talked about in my refactoring series was very easy to learn as the only used pattern was copy and paste. You’d simply go in, copy what you need and paste it into its new place, everyone could do it, everyone did it. However, the result was an unmaintainable mess.

Therefore I’d say that although learning a code base is required to effectively maintain it, learning and maintaining it are two different metrics that should be looked at independently. The learning cost is a one time cost, fixing a bug or adding a feature isn’t.

Yet another story is ongoing learning, this has directly to do with maintainability as no one can remember every line of an entire code base that has a sufficient size.

If a code base is consistent with its conventions, no matter how strange they may first seem to outsiders, there shouldn’t be a problem as this ongoing learning cost will stay at a low level.

Please note that I’m not saying “that’s a convention” excuses everything. There has to be a structure and concepts have to be followed but this structure or these concepts don’t necessarily have to jump the outsider right in the eye as it is the case in the classical three tier architecture for example.

As I was looking for another open source project for my refactoring series, I saw both, very good and very bad code.

As many projects, this one just wanted to implement three tiers, with the data and the business layer having their own data containers. Although they are equal by content, they are different classes.

I really don’t understand why anyone would do such a thing:

// In some method that needs an "Category" object
Category category = DBMapping(dbItem);

// Somewhere else, revealing what the dbItem is....
private static Category DBMapping(DBCategory dbItem)
{
    if (dbItem == null)
        return null;

    Category item = new Category();
    item.CategoryID = dbItem.CategoryID;
    item.Name = dbItem.Name;
    item.Description = dbItem.Description;
    item.TemplateID = dbItem.TemplateID;
    item.MetaKeywords = dbItem.MetaKeywords;
    item.MetaDescription = dbItem.MetaDescription;
    item.MetaTitle = dbItem.MetaTitle;
    item.SEName = dbItem.SEName;
    item.ParentCategoryID = dbItem.ParentCategoryID;
    item.PictureID = dbItem.PictureID;
    item.PageSize = dbItem.PageSize;
    item.PriceRanges = dbItem.PriceRanges;
    item.Published = dbItem.Published;
    item.Deleted = dbItem.Deleted;
    item.DisplayOrder = dbItem.DisplayOrder;
    item.CreatedOn = dbItem.CreatedOn;
    item.UpdatedOn = dbItem.UpdatedOn;

    return item;
}

Doing so provides nothing, you just violate DRY and thereby create your self some great amount of repetive work. Further, human errors like forgetting a property will arise.

Another thing I encountered rather often is the following:

private IList<Category> GetAllCategories()
{
    return DataLayer.CategoryManager.GetAllCategories();
}

The so called business layer is nothing more than a very thin proxy to the data layer. If you used interfaces for decoupling in both, the business and the data layer, you have to change 3 places in exactly the same way, doesn’t that feel just wrong?

If all you want to do is to allow different data stores, then just do that. There is absolutely no reason to duplicate your data containers in such a case.

You have at least three ways:

Let the different data layer implementations depend on the business layer. Define the data containers and the interfaces to retrieve those in the business layer and let your IoC container do the rest.

Create a separate assembly that contains the data containers. You can still let the business layer depend on the data layer, yet you don’t have to duplicate anything.

Use an ORM, like NHibernate.

If you ever created a project with just 2 parts, usually named ui/gui/presentation and core/logic/service, it’s actually the same. The only difference is that you implement the interfaces by which you retrieve your entities / business object, or however you want to call them, in a separate assembly. Done. Data stores of all kind can be implemented.

By doing so, you can unit test, without having to hit your real data store. Further, you can switch databases, just load a different data layer implementation and its done.