Do unit tests unnecessarily obfuscate code, or is there a better way? [closed]

Question

So I have been working in an organization that puts a fair amount of pressure on devs to write and maintain unit tests. While it's not something I've done a whole lot in the past, I like the idea and believe any serious project should have some level of unit testing, especially for self container class libraries that lend themselves to such testing.

However, I've also found that what was once very simple, readable code is made into a monstrosity of factories and interfaces. At the simplest case, a service wrapper:

No unit tests

class ShazamService
{
   private string url;

   public ShazamService(string url) { this.url = url; }

   string IdentifySong(byte [] mp3Data)
   {
       return HttpHelper.Upload(url, mp3Data).Response;
   }
}

class Program
{
   public static int Main(string [] args)
   {
      var svc = new ShazamService("http://www.shazam.com");
      Console.Writeline(svc.IdentifySong(args[0].ToByteArray());
   }
}

Unit testable version

public interface IShazamService
{
   public string IdentifySong(byte [] mp3Data);
}

public class ShazamClassFactory
{
   private string url;

   public ShazamClassFactory(string url) { this.url = url; }

   public IShazamService GetInstance(bool test)
   {
      if (test)
      {
         return new ShazamServiceTest(this.url);
      }
      else
      {
         return new ShazamService(this.url);
      }
}

class ShazamService
{
   private string url;

   public ShazamService(string url) { this.url = url; }

   string IdentifySong(byte [] mp3Data)
   {
       return HttpHelper.Upload(url, mp3Data).Response;
   }
}

class Program
{
   public static int Main(string [] args)
   {
      var factory = new ShazamClassFactory("http://www.shazam.com");
      var svc = factory.GetInstance(false);
      Console.Writeline(svc.IdentifySong(args[0].ToByteArray());
   }
}

Not only is the code significantly longer in the 2nd one, but (to me) it's less clear - from Main I don't even know the type of the return value from CreateInstance if I need to look at an implementation detail, so I can't even F12 through the logic as easily. Also what would have been 1 file for the service now becomes 4 (factory, interface, 2 implementations), with header, documentation, etc. Lastly, if I decide I want to change the constructor from string url to string url, SongGenre genre, I now need to check out, update, and check in 4 separate files, updating constructors, datamembers, documentation, etc for each.

Is this method of promoting unit testing the norm? Are there less intrusive options? And, is it worth it? To my mind, by complicating the code, you increase dev time, and make errors more likely to sneak in, all for unit testing using fake objects that will only sorta-kinda test the code you're using.

Answer 1

The code is unclear because it is badly written.

Dependency injection is done by injecting the class you want in a setter or a constructor, not by hardcoding the different options and using a GetInstance(bool) method to get your testing action.

Instead it should look more like this:

public class ShazamClassFactory
{
   private string url;
   private IShazamService _shazamService;

   public ShazamClassFactory(string url) { this.url = url; }

   public void SetShazamService(IShazamService service) {
      _shazamService = service;
   }

   public string GetSong(){
      return _shazamService.IdentifySong(url.ToByteArray());
   }
}

Now you can use it like this:

var factory = new ShazamClassFactory("http://www.shazam.com");
factory.SetShazamService(new ShazamTestService());
var song = factory.GetSong();

Answer 2

The problem I see here is that it's not immediately clear what you're trying to test.

If you are writing code that uses a ShazamService then you can pass either a concrete implementation or a test implementation, depending on whether it's a unit test or not.

The use of a factory should be used if you need to control when an object gets created, and should not (imo) be the default pattern when passing in dependencies.

For your instance, a better option could be.

Service Interface

public interface IShazamService
{
    string IdentifySong(byte [] mp3Data);
}

Actual Live Interface

public class LiveShazamService : IShazamService
{
    private readonly string _url;

    public LiveShazamService(string url)
    {
        _url = url;
    }

    public string IdentifySong(byte [] mp3Data)
    {
        return HttpHelper.Upload(url, mp3Data).Response;
    }   
}

Test Interface (probably lives in your test project)

public class MockShazamService : IShazamService
{
    private readonly string _testData;

    public LiveShazamService(string testData)
    {
        _testData = testData;
    }

    public string IdentifySong(byte [] mp3Data)
    {
        return _testData;
    }   
}

Test Code

[Test]
public void ShouldParseTitleOfSong()
{
    // arrange
    var shazamService = new MockShazamService(
        "<html><title>Bon Jovi - Shock to the Heart</title></html>");

    var parser = new ShazamMp3Parser(shazamService);

    // act
    // this is just dummy input, 
    // we're not testing input in this specific test
    var result = parser.Parse(new byte[0]);

    // assert
    Assert.AreEqual("Bon Jovi - Shock to the Heart", result.Title);
}

Production Code

public class ShazamMp3Parser
{
    private readonly IShazamService _shazamService;

    public ShazamMp3Parser(IShazamService shazamService)
    {
        _shazamService = shazamService;
    }

    public ShazamParserResult Parse(byte[] mp3Data)
    {
        var rawText = _shazamService.IdentifySong(mp3Data);

        // bla bla bla (up to the viewer to implement properly)
        var title = rawText.SubString(24, 50);  

        return new ShazamParserResult { Title = title };
    }
}

Usage of Production Code

public static int Main(string [] args)
{
    var service = new LiveShazamService("http://www.shazam.com");

    var parser = new ShazamMp3Parser(service);

    var mp3Data = args[0].ToByteArray();

    Console.Writeline(parser.Parse(mp3Data).Title);
}

In this example, I am showing how to test code that depends upon the IShazamService (the ShazamMp3Parser), this lets you unit test the parsing of the title without having to make an internet connection and pull live data. The mock service lets you simulate data and unit test how your parsing code works.

I did not implement the factory as I don't feel it's necessary in this scenario, but if you wanted to control when the service is instantiated, you could write a factory interface, followed by two implementations, one that constructs the live service and one that constructs the test one.

If you get brave later on, or you get sick of writing mock classes all over the place, you can use a mocking framework (like moq) to make your unit test writing faster.

[Test]
public void ShouldParseTitleOfSong()
{
    // arrange
    var mockShazamService = new Mock<IShazamService>();

    mockShazamService.Setup(x => x.IdentifySong(It.IsAny<byte[]>()))
                     .Returns("<html><title>Bon Jovi - Shock to the Heart</title></html>");

    var parser = new ShazamMp3Parser(mockShazamService.Object);

    // act
    var result = parser.Parse(new byte[0]);

    // assert
    Assert.AreEqual("Bon Jovi - Shock to the Heart", result.Title);
}