5
\$\begingroup\$

I am coding a web based solution which consists of N number of tiers, these are:

  • UI
  • Web API
  • Business

The question I have is related to validation, repository and CRUD operations. I am adopting the command / query pattern whereby I have my repositories for data retrieval only (see below) and its query dependencies injected in. I have seen people use repositories for CRUD too, but then I get the impression it is doing too much thus warrants further abstraction.

public interface IAddressRepository
{
    Address GetById(int id);

    IEnumerable<Address> GetByPersonId(int personId);

    IEnumerable<Address> GetByPostalCode(string postalCode);
}

public AddressRepository(IGetAddressByIdQuery getAddressByIdQuery
                       , IGetAddressesByPersonIdQuery getAddressesByPersonIdQuery
                       , IGetAddressesByPostalCodeQuery getAddressesByPostalCodeQuery)
{
    this.getAddressByIdQuery = getAddressByIdQuery;
    this.getAddressesByPersonIdQuery = getAddressesByPersonIdQuery;
    this.getAddressesByPostalCodeQuery = getAddressesByPostalCodeQuery;
}

Should my repository return validation errors? For example, the GetById(int id) method will be checking if the id > 0 then make a DB call or simply return a new instance, likewise with a postal code lookup. However, I also think going by the SOLID design principle everything should be autonomous thus this approach of returning no validation errors is the correct choice. I choose to NOT return null on all my methods simply because checking for null is considered as an anti-pattern.

The encompassing of the CRUD logic. I have seen uses of a Manager and/or Service classes, for example:

public interface IAddressManager
{
    IEnumerable<Error> Delete(Address address);

    IEnumerable<Error> Save(Address address);

    IEnumerable<Error> IsValid(Address address);
}

public AddressManager(IAddressValidator addressValidator, IAddressCommand addressCommand)
{
    this.addressValidator = addressValidator;
    this.addressCommand = addressCommand;
}

Is the general consensus to have a manager class which encompasses these operations but have the dependencies (the commands injected in)? This way, before I call save(), I validate the object before persisting it. I can also control this behavior via TDD.

Query code example:

public interface IGetAddressByIdQuery
{
    Address Execute(int id);
}

public class GetAddressByIdQuery : IGetAddressByIdQuery
{
    private readonly string connectionString;

    public GetAddressByIdQuery(string connectionString)
    {
        this.connectionString = connectionString;
    }

    public Address Execute(int id)
    {
        using (var conn = new SqlConnection(this.connectionString))
        {
            return conn.Get<Address>(new { Id = id });
        }
    }
}

The data access tier is not apparent as at the moment as the business library is fairly small. I am a firm believer in not creating several libraries for the sake of it. Only create them when it is warranted to do so, i.e. a library is getting too big.

What are your thoughts as to whether or not a better approach / pattern exists?

\$\endgroup\$
  • \$\begingroup\$ Are you using an ORM layer like Entity-Framework? \$\endgroup\$ – Jeroen Vannevel Apr 11 '14 at 11:39
  • \$\begingroup\$ Yeah, using Dapper. I have a clear separation though whereby I have entities representing rows in a table and a contract assembly for public facing. Automapper is then used for conversion. \$\endgroup\$ – Dr Schizo Apr 11 '14 at 13:18
  • \$\begingroup\$ Can you clarify a little what IGetAddressByIdQuery and likewise are in your constructor? \$\endgroup\$ – Jeroen Vannevel Apr 11 '14 at 13:23
  • \$\begingroup\$ These are query objects, i.e. have a single method called Execute(int id). Reason for this is to clearly separate each query (updated post to include this code). \$\endgroup\$ – Dr Schizo Apr 11 '14 at 13:26
1
\$\begingroup\$

I'll address your separate classes for the queries first. I definitely see where you're coming from with this approach and I can dig it, but it gives too much duplication as it is.

I assume you'll have more entities than just Addresses so using your approach, you would have to create a new interface for each entity you want to get by its ID. If you instead added some generics, you can do all of this with one interface:

interface IGetEntityByIdQuery<T> {
    T execute(int id);
}

Now you can have these implementations:

class GetAddressByIdQuery : IGetEntityByIdQuery<Address> {
    Address execute(int id) { }
}

class GetOrderByIdQuery : IGetEntityByIdQuery<Order> {
    Order execute(int id) { }
}

However even though I first said I can dig it, this will still be very cumbersome to work with. Having to create a new class for the retrieval for each entity for each parameter? That's too much.

I can agree on a setup where you combine them by entity:

interface IGetEntity<T> {
    T GetEntityById(int id);
    T GetEntityByPersonId(int id);
}

Now on to your remark about CRUD:

I have seen people use repositories for CRUD too but then I get the impression it is doing too much thus warrants further abstraction.

The entire idea behind repositories is CRUD. In fact, your GetXByY classes are simply executing the R part in CRUD. Repositories exist to provide communication from your model to your datasource, which is what we understand under CRUD.

Another red flag here should have been your name choice: when you have a class named XManager you have to really overthink your design and be sure that this is the way to go. Often it is an indication something is wrong because in essence, a "manager" class doesn't convey much meaning.

What does this class do? Other names for such encompassing classes are a lot more informative, for example XFactory indicates the class will construct instances of X. A manager.. manages?

Therefore these actions should be added to your repository. This will now keep these related actions (CRUD) together which prevents any confusion with other programmers.

That being said: it might very well be a suitable name for a class, this is definitely not a general rule. I have had to use XManager classes as well before, but this was more because I simply couldn't come up with a better one (and no, it wasn't a repository ;)).

For more reflection upon the subject, I suggest reading this post.

As for your final question on the topic of errors: why are you doing validation logic in your data access layer? That is way too late!

You should have the guarantee that when an object is constructed, it has valid data. The entire idea behind getters and setters (aka: properties) is that the actual data is hidden and that the data can be validated.

When you construct a new object of type Address, it should be validated. This can be done in several places: properties, constructor, the factory that creates Address objects using some service layer, etc. There are many options to do this validation, but once the object is constructed you should have the implicit guarantee that the object contains valid data.

Lastly, for the boundary checks (like id > 0): this is up to your own preference. You could add a service layer in front of your repository that validates the incoming data and if you have several validation requirements then this might be a good solution. If on the other hand you only have this one boundary check then it's a little minor.

But I haven't come across the explicit recommendation to use a service layer in front of your data access layer just for validation yet, so as far as I'm concerned this is up to yourself.

\$\endgroup\$
  • \$\begingroup\$ I definitely agree with you on the fact that the data should go through validation before the repository gets a handle of it. However, the generic approach I can see some benefit to it but also the evil side to it as well. I find with generics, you often find yourself making it too generic and tries to cater for too much, i.e. an object that fits all shapes and sizes. Do you agree? \$\endgroup\$ – Dr Schizo Apr 11 '14 at 14:30
  • \$\begingroup\$ Sure, you should be careful to make sure you don't have a DoEverything repository, but there is a balance to strike here. You would have a lot of duplicate code for no real reason (and many, many classes for some CRUD actions with just a few entities). With the approach I proposed I don't think there's any of the evil side in it yet. You can split it up as you want ofcourse, but I think your current approach is doing exactly that: nearing the evil side of Separation of Concerns. \$\endgroup\$ – Jeroen Vannevel Apr 11 '14 at 14:38
  • 1
    \$\begingroup\$ Essentially, what I have done now is encompassed all my query and command objects in a repository (to encapsulate dependency injection) and implemented a generic IRepository<T> which exposes, GetById(), Create(), Update() and Delete() operations. Will see how this goes. This approach still allows me to use TDD. \$\endgroup\$ – Dr Schizo Apr 11 '14 at 15:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.