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!