2

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

  • For splitting helper methods off into "helper" classes, you need to ask yourself two questions: 1) What is the chance that I need this same method also elsewhere in my software, and 2) What is the risk that a fault in this method will pass through the other testing/inspection rounds without detection and how much would that hurt. – Bart van Ingen Schenau Mar 28 '16 at 12:18
  • well, there is no chance that they will be used somewhere else, or a very low one. About the second question, not sure, I have no experience at all writing unit tests. I only know that those functions are all used in constructing requests or parsing responses. – Michał Zegan Mar 28 '16 at 12:28
0

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.

  • This is a library that does a very specific thing. chances for having a need for more such methods are actually near 0, as other things in this library will just be highlevel wrappers for the low level APIClient. – Michał Zegan Mar 31 '16 at 9:52
  • @MichałZegan Well, as I hoped to express in the answer already, I'd still consider sourcing these utility functions out cleaner even if they happen to be only used by a single class. – 5gon12eder Mar 31 '16 at 19:56
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.

  • There is a drawback of separating authentication logic. the drawback is that the class is going to be thread safe. authenticating in the constructor makes it safe by definition. This authentication logic is not complicated enough to separate it into an interface. I was thinking about mocking just the http client dependency/request execution. – Michał Zegan Mar 31 '16 at 9:47
  • also, authentication actually consists of two api calls that require no user interaction, for getting the consumer key, something like the oauth token. then there is token validation, and that step is already separated into an interface. is that enough? – Michał Zegan Mar 31 '16 at 9:50
  • @MichałZegan synchronization can always be handled externally. Until a reference to the object is available to multiple threads it is single-threaded and thread-safe. – user22815 Mar 31 '16 at 14:23
0

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.

  • When writing this answer, I probably forgot about the fact that I don't have a method for json serialization now, just one for parsing json. but it is a wrapper for 3 or 4 calls of javax.json, so I am not convinced it requires separation at all. – Michał Zegan Mar 31 '16 at 9:54
  • It's impossible for me to know how complex your functions are and if they need separation. I can only give you suggestions, it's up to you to see what actually needs to be done. – jgauffin Mar 31 '16 at 10:17
  • that is the reason I give updates. I am trying to check if my way of thinking is wrong or not. – Michał Zegan Mar 31 '16 at 10:19
  • Ok. It's still hard to tell. depends on how much code there are for each part. A simple measure is just to write unit tests and try to get full test coverage. It is hard? Divide it into smaller classes. – jgauffin Mar 31 '16 at 10:37
  • There is one advantage of authenticating in constructor. I actually have a private authenticate method called from the constructor. the advantage is, it helps thread safety of the object. I do not have to worry about other threads calling authentication, especially that my intention would be for the method that sends requests to call authenticate if not already done. – Michał Zegan Mar 31 '16 at 10:46

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.