2
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ Each layer of abstraction should have it's costs be outweighed by the benefits. If you cannot describe in one sentence why you have that layer of abstraction, it probably shouldn't be there. YAGNI. \$\endgroup\$ – Dan Pantry Sep 9 '14 at 14:09
  • \$\begingroup\$ As Dan said, You Aren't Gonna Need It. How far should you go with abstractions? As far as you need to. Go further when you need to go further. Also, if when describing that layer of abstraction in a sentence you aren't allowed to use "might" or "may". \$\endgroup\$ – Nick Udell Sep 10 '14 at 13:05
3
\$\begingroup\$

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/

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thanks heaps, too bad I can't vote up yet. As for the IDnsUserBlacklistService, my mistake, just a mispelling since I refered above to IUserBlacklistService. @omittones solution also looks fine but I'll have to inherit my implementation class to mock out the DNS part. \$\endgroup\$ – Mickaël Derriey Sep 11 '14 at 8:04
0
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I completely disagree with your statement that "abstraction of one method is a little to much". The problem is, you've now changed the public API of the class for tests. There's no real reason to make GetDnsRecords virtual and if you want to get DNS records in some other class, you're now faced with refactoring this class again. \$\endgroup\$ – craftworkgames Sep 11 '14 at 3:56
  • \$\begingroup\$ I assumed the DnsService won't be used in other parts. The virtual is to allow method overriding for the purpose of testing. I was only concetrating on YAGNI rule. \$\endgroup\$ – omittones Sep 11 '14 at 7:39
  • \$\begingroup\$ Your solution is actually pretty clever and YAGNI is important but you've also got to consider resistance to change. Ideally each class should have a single responsibility and once implemented, shouldn't change. The real problem with making the method virtual for testing is that when you want to pull DNS out of the class you've broken both the class AND the test! I've made these types of mistakes plenty of times, and still do, I just happened to spot this one because I've seen it enough times before. \$\endgroup\$ – craftworkgames Sep 11 '14 at 12:41

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.