Repository patterns - Fighting unclean code

Every company you work for is different. Different developers, different styles, different techniques. But one thing I find that is consistent across almost all places I have worked is a constant battle to maintain clean code and some places are worse than others. I have particularly come across this recently and no matter how many times I offer up tips the unclean code seems to creep in. Due to the fact I'm working with some really decent guys that just have a lot of bad habits I am struggling to find the right side of that fine line between sounding like a grumpy picky developer and just putting up with it!

I'm not just talking about different styles here, I'll give you an example. For our plugin development we use the repository pattern so the following type of code is used quite often:

public class AccountRepository
{
    IOrganizationService _service;

    public AccountRepository(IOrganizationService service)
    {
        _service = service;
    }

    /*...*/
}

This is somewhat fine, although the 1 to 1 mapping between entities and repository classes isn't ideal, but it works well for us so far. Unfortunately some of the developers don't fully understand what the repository pattern really is and what it should and shouldn't know of/know how to do. Recently I spotted that the following code was checked in some time back:

public class AccountRepository
{
    IOrganizationService _service;
    Entity Account { get; set; }

    public AccountRepository(IOrganizationService service)
    {
        _service = service;
        Account = null;
    }

    public AccountRepository(IOrganizationService service, Entity Account)
    {
        _service = service;
        this.Account = Account;
    }
    /*...*/
}

The problem this introduces is that my repository is now aware of an account record/entity. This immediately points the finger at the code breaking SRP (Single Responsibility Principle).

Let's refresh our memory on what a repository pattern is. Put simply, the repository should act as a data layer or mediator between our business logic, domain objects and our data source. It should know how to take a query and provide a resulting set of domain objects. It might also be able to add and remove objects, or even update objects. To perform these operations it should be aware of how to connect to the data source, how to map the result of a query to domain objects and it should make that list of objects available to the caller. In some instances you may move the "data mapping" logic out into its own classes, but in Dynamics CRM the Organization Service already provides you with Entity objects (or strongly typed objects if you prefer) so you may not need a separate data mapping abstraction.

Now, going back to the code above, the extra introduction of an existing record in the constructor causes us to add a capability to the repository. It is now record aware before you even run a query. This breaks SRP and most likely some other principles too. You'll find OCP, Open Closed Principle, hanging on a very fine thread as a result too! I am guessing that further down in the code (here where demons lie...) you will most likely see references to this object and decisions being made based on the object and its attributes. Oh you naughty naughty developers! What were you thinking!

So, what do I do? Refactor a load of methods I didn't develop and spend half a day fixing other peoples code? Send out a tip (yet another one!) saying why this is bad and look like Mr Grumpy (yet again!)? Or do I ignore it and hope it goes away in time...

Decisions decisions!

What security groups do you belong to within Active Directory?

If you want to see what security groups you belong to within Active Directory open up a command prompt and run the following:

gpresult /V


This is really useful if you are having problems installing a piece of software (such as MS Dynamics CRM) and are hitting weird security problems. Quite often when installing Dynamics CRM you will have asked for and believe you have domain administrator rights, but the installer keeps telling you something like:

Action Microsoft.Crm.Setup.Server.GrantAspNetServiceAccountAccessAction failed. ---> System.Runtime.InteropServices.COMException: The security ID structure is invalid


Running the above command will give you a nice list of exactly what you have:

The user is a part of the following security groups
---------------------------------------------------
    Domain Users
    Everyone
    SophosUser
    BUILTIN\Users
    BUILTIN\Administrators
    Remote Desktop Users
    NT AUTHORITY\INTERACTIVE
    CONSOLE LOGON
    NT AUTHORITY\Authenticated Users
    etc...

If broken it isn't, fix it you shouldn't...

From speaking to some old friends of mine I heard about a cataclysmic implosion of a project I used to work on. This was a project that initially started as an extremely badly coded asp.net application (with horrors like functions with 1000+ lines of code). A project that we spent over a year on trying to upgrade to use much better patterns and practices and add tonnes of test coverage on while changing/adding features to the application. It was by no means perfect when we brought it over the finishing line, but it was a very solid and decent bit of code. The damn thing even won an award!

Anyway, it led me on to writing this blog post. And my tip of the day is:

"If broken it isn't, fix it you shouldn't..."

The basic concept I put forward is if something isn't broken then why change it? If you're changing something just because it's not agile enough, or you don't like the code, then you're not really following agile principles, or at least not following one of my favourites: Take the first bullet. Some people call it "fool me once, shame on you, fool me twice, shame on me". Effectively, if a requirement or story "fools" us into not making that abstraction, or into writing code that doesn't protect us from that bug, then that's the first bullet. When we get hit by that bullet it's time to abstract away or protect that code so that we don't get hit again.

Here's a really basic example: Let's say I ask you to write some code to sort a list of contacts by their name. You might write me a sorter class that takes in the list of contacts and returns a list sorted by their name. You might even go as far as passing in a parameter so that you can sort it by any field you want.

Then comes a second requirement - I want to sort it by email address as well as name, because I have 20 joe bloggs. Ok, so we didn't account for the ability to sort by multiple fields. One solution to this problem is add a parameter to allow you to add a second field to sort by. But what if we get 3 fields? Or what if the new field didn't have the same way of sorting as name? A better solution would be to introduce Comparer objects or something of that manner. That way you let the invoker tell you how to compare the objects and that way you've taken the first bullet. What we have in effect done is followed up by closing off a possibility of further sorting issues. (For more information on this look up OCP, or Open/Closed principle).

This is exactly what didn't happen in this "cataclysmic" project. Things were deemed "not agile enough" so changes were being made to modules and pages and classes that weren't taking any "bullets". Next thing you know things stopped working, because the whole project architecture started to change and remould over time. This is probably very much expected. So this (every so slightly) changed the situation into "broken it is (now), so fix it you bloody well should!". Their decision? Rewrite the whole project. My jaw left a dent on the floor when I heard that bit, no word of a lie... Such madness!

Finally, if you're astute you may have recognised that my blog entry title was very much inspired by an excellent blog by Tess Ferrandez! (If broken it is, fix it you should)

Convention over Configuration - Case sensitivity?

Today I had a debate with a fellow employee about the naming conventions of some fields on a CRM form. It's a slightly off the norm of "convention vs configuration" arguments, instead it was more of a "Convention A vs Convention B" type argument. Take this example, the particular field that was added was a web resource. For other reasons we have some javascript that passes over the form to try locate these web resources. I designed the javascript so that it will always look for web resources that start with the following naming convention:

WebResource_DS*

So, if we follow this convention we don't need to configure anything. Note that DS is a recognised acronym in our organisation. Unfortunately we ran into some problems in our testing environment, some of the controls weren't being detected. I take a look at the controls and they've been named:

WebResource_ds*

Now, my gut reaction is to fix the script to allow for case sensitivity... but after a quick think this feels wrong to me. If we're following convention shouldn't it be case sensitive? For example, if we introduce camel casing on database field names then "productname" is incorrect, it should read "productName".

So instead of changing it I argued that it should follow the convention of _DS including upper case. I argued that if you follow "convention over configuration" it should include case sensitivity. This should be especially the case for known acronyms (if the convention states all upper-case for acronyms). The debate that he put forward is if you've ever read something like "the design of everyday things" everything should be intuitive, so 'DS' or 'ds' shouldn't matter.

I see his point... but I stick by my guns!

Mocking out Environment.Exit

Question I came across recently on Twitter.

"How do I write a test when there's an Environment.Exit?! :("

Very good question... My answer was basically rewrite the code! A really quick way to do this is create a static action that will allow you to mock it out. What if I created a class called MyEnvironment with this Action:

    public class MyEnvironment
    {
        public static Action Exit = exitCode => Environment.Exit(exitCode);
    }

In code I can just replace the call with this:

    MyEnvironment.Exit(0);

And now, in my testing I can do something like this:

    MyEnvironment.Exit = exitCode => { };

Voila! No more interrupted tests :)

How to post code in blogger!

Ok, after struggling initially with posting code in blogger I managed to figure out how to do this. When I started this blog I didn't realise the following tags existed:

<pre name="code">
code goes here!
</pre>

This is much better and results in better formatted posts. The only real "gotcha" is you'll have to replace some "html" style tags with their html safe equivalent. For example, "<" becomes "&lt;"

Now to go back and fix my blog...

How do you copy the text from a standard windows message box?

Press CTRL+C

Easiest tip EVER!