designing classes for testability

Question

I am writing a library for OVH api calls, and I am wondering how to make it friendly for unit tests. I have a class APIClient. The constructor of this class initializes the object with all parameters, but then immediately tries to perform authentication. There is also the send method for sending generic authenticated API requests. The thing is: I have some private helper methods used throughough the code, like toHex that transforms bytes into hex, hash that is a shorthand to do a SHA1 hash of a given string, and parseJson that parses a json out of the http response using an external dependency, that is javax.json. Should I move those utilities to another utility class as static methods to be able to test them separately without creating full api client objects? I don't think I will use those methods anywhere else.

Update:

When performing authentication and sending requests, it usually looks like: build json objects, serialize json, create and send http request with appropriate headers, get response, parse response as json, extract data. Although the details differ as all requests after authentication are actually signed. edit2: this is a rest service

Answer 1

Snowman's answer already addresses the issue with the authentication and I agree with this advice.

Concerning the utility methods you've mentioned, I'd recommend sourcing them out into a utility class. Even if you need them only in one place at the moment, it feels wrong to have

static String toHex(byte[] data);

or

static byte[] hashSha1(byte[] data);

in an APIClient as doing these things is not part of an APIClient's genuine business. Putting them in a utility class where they can be public allows you to unit test them in isolation and perhaps re-use them later.

There is no undue cost associated with doing this. Inside the APIClient, you can refer to the static methods in the utility class directly without needing to inject this dependency. These utility functions are so simple that you probably won't ever want to stub them out for testing.

The trickiest part is probably finding a decent place for those utility methods. Sometimes, it might be tempting to throw them all together into a single Util class but this will quickly become messy. So maybe group them semantically into “text functions”, “hash functions”, etc and make sure that it will be intuitive where to look for or maybe add functionality later.

Answer 2

The constructor of this class initializes the object with all parameters, but then immediately tries to perform authentication.

This is the problem. A constructor should construct the object, not invoke functionality. Perhaps the constructor leaves the new object in a state where it can authenticate, but that is all.

If you need to perform tests on this class, you may want to use mock functionality. If authentication is a problem (unit tests generally should not communicate with a remote API, for example), split that responsibility into a separate class. Then inject a mock authentication object.

Generally, you want to mock objects that perform actions that are not useful in unit tests and pass those into the objects you really want to test. So maybe the authentication itself should be encapsulated in a separate class. Create an interface, where there is a "real" implementation and your mock implementation. When constructing an object that must use authentication, you pass in an authenticator that will be a mock in your unit tests, while the production code will use the real authenticator.

In other words, it sounds like one object may have too much responsibility here.

Answer 3

Regarding authentication, just as Snowman says. The constructors responsibility is to construct the object. So you need to move that code into a new method. For that you got two choices, either introduce a public Authenticate() method, or let it be protected and be called from the Connect() method.

The first alternative introduces temporal coupling. It means that the methods have to be executed in a correct order to be able o work (Connect, Authenticate, AnyOtherMethod). For clients that might as well be OK as devs are used to Connect(). However, the authenticate method also brings insecurity. In which cases to I need to authenticate? Do I need to authenticate for all methods?

Better to move it into the Connect method or call it internally in those cases it's needed. There is really no need to do it in the constructor as it still would generate a runtime exception (and not compile time error). So it really do not matter if it's called in the constructor or when you do something else, right?

As for the encryption methods, you are breaking the Single Responsibility Principle. You class got two reasons to change. First is if the server API changes and the second if the encryption changes.

I'm not a big fan of Util classes, they tend to violate SRP and grow fast. Instead you should probably introduce a ApiCrypto class which takes vare of that.

Same goes for the serialization. If you have methods in your client which is dedicated for serialization, move them to a ApiSerialization class and call it from within the client class.

When done, take a step back and enjoy the testable view.