How far should I go with abstraction?

Question

I've been trying for a while to write clean and testable code, with the help of DI and abstractions.
I found myself guilty of service-itis, creating 3 or 4 interfaces for something that clearly doesn't need it. So I decided to relax, and only abstract what really needs it.

I'm faced with a problem where I'm not sure what is the best way to solve.
Let me explain, the requirements are pretty straightforward.

The application must determine, when a user wants to sign up, if he's blacklisted, based on a couple of information about the user
The service providing the information about the possible blacklisting of the player is external, and not accessible from our development platforms.

OK, I see here a need for an interface to abstract the blacklisting service so I can test properly the actions that rely on the fact that a user may be blacklisted, so I come up with a cery basic interface:

public interface IUserBlacklistService
{
    Boolean IsUserBlacklisted(IsUserBlacklistedRequest request);
}

public class IsUserBlacklistedRequest
{
    public String FirstName
    {
        get;
        set;
    }

    public String LastName
    {
        get;
        set;
    }

    public DateTime BirthDate
    {
        get;
        set;
    }

    public String BirthCity
    {
        get;
        set;
    }

    public String BirthCountry
    {
        get;
        set;
    }

    public String BirthDepartment
    {
        get;
        set;
    }
}

Now, the actual service I have to deal with works this way:

The blacklist is exposed as a DNS server, meaning I have to make a DNS query, asking for hashofsomeoftheuserinformation.somedomain.tld, something like firstnamelastnamebirthdate.somedomain.tld
If the provided information matches entries in the blacklist, the DNS server replies with TXT records containing:

• A set of information for French user
• Another set of information for other users

I then have to match this information with the information the user filled to know if he's blacklisted or not.

At this point, I'm thinking of creating the DnsUserBlacklistService class implementing the IUserBlacklistService, but also of creating another abstraction representing only the DNS query, let's call it IDnsService, which will look like:

public interface IDnsService
{
    IEnumerable<String> GetRecords(String queriedDomain);
}

This, in my mind, will allow me to safely test:

• That the queried domain is the right one, so I know I build the hash correctly from the user information
• That the matching between the returned TXT records and the user information is correct

So the class DnsUserBlacklistService would look like this:

public class DnsUserBlacklistService: IUserBlacklistService
{
    private readonly IDnsService _dnsService;
    private readonly String _domain;

    public DnsUserBlacklistService(IDnsService dnservice, String domain)
    {
        _dnsService = dnsService;
        _domain = domain;
    }

    public Boolean IsUserBlacklisted(IsUserBlacklistedRequest request)
    {
        var subDomainToQuery = GetSubDomainToQuery(request);
        var fullDomainToQuery = String.Format("{0}.{1}", subDomainToQuery, _domain);

        var dnsRecords = _dnsService.GetRecords(fullDomainToQuery);
        if (dnsRecords != null && dnsRecords.Any())
        {
            // Match between records and user information
            // Note: I know I shouldn't write pseudo-code, but this part
            // is really unrelevant to the question
        }

        // If no DNS records, assume the user is not blacklisted
        return false;
    }

    private String GetSubDomainToQuery(IsUserBlacklistedRequest request)
    {
        return String.Concat(
            request.FirstName,
            request.LastName,
            request.BirthDate.ToString("yyyyMMdd")
        );
    }
}

It seems to me that having everything in the DnsUserBlacklistService class wouldn't allow me to test thoroughly the process. I could have some internal methods being exposed to the test project with an [InternalsVisibleTo] attribute, like the hash construction, but that doesn't abstract me from the actual DNS query, which is the one external service I don't own.

In the end, my question is: am I going too far with abstractions?
Maybe some thoughts on this particular example could help me decide in the future where to stop.

Answer 1

For the purposes of testing, the most important abstractions exist on the boundaries of the system. For example, files, databases, web services, randomization and the system clock all come to mind.

The reason is that these things change state. Files and databases are persistent, web services may or may not be available, randomization and clocks produce different results each time they are executed.

Therefore, you've actually done the right thing by abstracting the DNS Service. You can easily mock out a test implementation of the IDnsService interface and inject it into the DnsUserBlacklistService. This type of interface is called a 'role interface'.

However, what I think you could do better is NOT use the IDnsUserBlacklistService interface. This is called a 'header interface' because it defines all of the methods of the implementation. This means you haven't really abstracted the idea of a blacklist service. A better interface here would be just an IUserBlacklistService and it might look something like this:

public interface IUserBlacklistService
{
    Boolean IsUserBlacklisted(IsUserBlacklistedRequest request);
}

The reason this is better is because classes that use this interface don't need to know anything about the fact that it's implemented using DNS. They only care about weather or not a user is blacklisted, not how that data is fetched.

Imagine for example you wanted to provide a file based blacklist service, you could re-implement the interface in a FileSystemUserBlacklistService and inject it into the same classes.

The rule of thumb is that interfaces are about the consumer, not the implementer.

There's a really good discussion about interfaces here: http://blog.ploeh.dk/2010/12/02/Interfacesarenotabstractions/

Answer 2

I assume that you don't have to test DnsService since it's externally owned, and that the IDnsService.GetRecords will only be used in this particular class. Because of that, I would eliminate DnsService and use a protected virtual method in DnsUserBlacklistService to query the actual server.

public class DnsUserBlacklistService
{
   protected virtual IEnumerable<string> GetDnsRecords(string query) { 
       //implementation
   }

   //rest of the method
}

This way you can replace it with test implementation in your tests. The general rule of thumb is to never use inheritance where aggregation will suffice, but I think abstraction of one method is a little to much.

I would also not use [InternalsVisibleTo] since your DnsUserBlacklistService also exposes a single public method, and would treat the entire class as a single testable unit.