0

We have decided to create an HTTP logger which can be reused across multiple projects, we created a utility which looks like this.

// pseudo-code
public class HttpLog{
      private readonly string uri;
      private readonly RestClient client;

      public HttpLog(string uri){
          this.uri = uri;
          // notice the initialization of rest client
          this.client = new RestClient();
      }

     void write(object data){
         this.client.uri = this.uri + endpoint;
         this.client.postAsync(data);
     }           
}

The consumer should provide the URI and we have exposed public write methods to log the data, however, we are unable to unit test our HttpLog class since it initializes the rest client. We are not using dependency injection since we are creating utility.

Any help would be greatly appreciated on how to refactor or unit test .write() method.

We can think of two methods

  • Constructor overload (which is not an efficient way just to unit test)
  • making client property as public {get; set} which also breaks OOP principle.

Please let us know if there is a better way to unit-test this code.

The answers below stated to use constructor overloading or making the property to public

Why I am not preferring dependency injection or constructor overload because of I strongly believe the consumer/client should not care or worry about the implementation details. They should be as much as abstraction possible. If you make them in constructor overload then you are making a way to pollute the abstraction.

For example, if you are using RestClient or HttpClient they don't ask you to provide HTTP implementation on how to write data, they simply ask you URI and data to post that is what a true abstraction is to end user.

Please correct me If my assumptions are wrong

Nkosi
  • 191,971
  • 29
  • 311
  • 378
user3205479
  • 1,225
  • 1
  • 13
  • 35
  • You need to research [dependancy injection](https://stackoverflow.com/questions/130794/what-is-dependency-injection). `RestClient` should be a dependancy and it should be injected into your class. This allows you to [mock it](https://stackoverflow.com/questions/2665812/what-is-mocking) and isolate your unit test from external resources, e.g. HTTP – Liam Oct 17 '18 at 08:25
  • What principle does `public {get; set}` break? I'd suggest you look into [SOLID principles](https://en.wikipedia.org/wiki/SOLID) – Liam Oct 17 '18 at 08:27
  • @Liam If I make a property as a public, I can set the value even to null accidentally anywhere outside the class which leads to code maintainability problems right? The scope or accessor is globally visible the chances of changing it is high – user3205479 Oct 17 '18 at 08:36
  • No...? Why would that lead to code maintainability problems? If null is a valid value then you need to test for that. If it's invalid then you should test for null and throw an exception to the consumer. This all sounds like you have a poor [single responsibility for you class](https://en.wikipedia.org/wiki/Single_responsibility_principle). – Liam Oct 17 '18 at 08:39
  • Also DI happens in the constructor. So all dependencies **need** to be injected at creation. So there should be no scope to make a dependency `null`. If there is (references maybe) this again makes me think you class design is poor, possibly overly decomposed. If you use dependency injection (and I recommend you do) I'd also recommend you use a dependency framework like [Simple Injector](https://simpleinjector.org/index.html) (my personal favourite but there are others). – Liam Oct 17 '18 at 08:41
  • @liam If you think class design is poor, can you show one? Can you refactor the above class with well-design – user3205479 Oct 17 '18 at 09:20
  • @Liam could you please tell me the reason to have private, protected and public modifiers? I think that answers why i don't want to make my property as public (just to make it testable) – user3205479 Oct 17 '18 at 09:21
  • @Liam please refer Flater answer why I dont want to make it public – user3205479 Oct 17 '18 at 09:26
  • I'm a bit confused as to what your asking. I think what your getting at is that you don't want to declare `RestClient client` as public. No one is saying this should be public, what we're saying is that `RestClient` is a dependancy and as such (to facilitate testing among other things) it should be injected into your class in the **constructor**. This property `private readonly RestClient client;` can remain as is, just don't do this `this.client = new RestClient();` instead do what [nvoigt is suggesting](https://stackoverflow.com/a/52850367/542251) – Liam Oct 17 '18 at 09:45
  • @Liam `Also DI happens in the constructor.` Pedantically, that is not a globally correct statement. Using publically settable properties is _also_ a form of dependency injection. It simply suffers from other OOP and code quality issues, which makes it an inferior option when compared to using constructor parameters; but that doesn't mean it's not a form of dependency injection. – Flater Oct 17 '18 at 09:53
  • Yeah, I know some frameworks support this, but they're wrong. I will always push [Simple Injector](https://simpleinjector.org/index.html) because it pushes correct design over easy shortcuts – Liam Oct 17 '18 at 10:00
  • 1
    @user3205479 can you confirm if `RestClient` is a custom class or from 3rd party library? Do not want to be making assumptions here – Nkosi Oct 17 '18 at 10:17
  • 1
    @Nkosi It is third-party library in C#. also please see updated question – user3205479 Oct 17 '18 at 10:22
  • 1
    @user3205479 ok with that clarified then I would agree with already stated answers. explicitly injecting abstraction (interface) into dependent class would be the approach to take. – Nkosi Oct 17 '18 at 10:25
  • 1
    tight coupling to implementation concerns will make it difficult to unit test the dependent class in isolation. – Nkosi Oct 17 '18 at 10:33
  • @Nkosi So exposing the the dependent classes in constructor is fine to consumers. This breaks the abstraction of hiding details to consumers – user3205479 Oct 17 '18 at 11:21

2 Answers2

1

First things first: what are you testing? Your method, or the REST service itself?

Since you say "we are unable to unit test our HttpLog class", I infer you're trying to test your class.

Therefore, you should test it without the REST service (which is an external dependency). The REST client should be injected as a dependency, so it can then easily be mocked.

We are not using dependency injection since we are creating utility.

That is not a valid argument for skipping dependency injection.

Note: I infer from your statement that you know how to implement dependency injection, you've simply chosen not to. I'm going to omit an actual example of dependency injection so this answer can focus on the core problem: your decision not to use dependency injection.

  1. Constructor overload (which is not an efficient way just to unit test)

This defeats the point of testing. You're creating a different code path for your test and your (real) runtime, which means the test no longer (fully) tests the runtime code execution.

Currently, your constructor only instantiates the rest client, so you're not compromising much. But the same would not apply if the constructor did more than just that. Secondly, you'll be unable to detect any regressions if you're testing a different constructor than you actually use at runtime.

  1. making client property as public {get; set} which also breaks OOP principle.

Your second suggestion directly proves that you are willing (and trying) to inject a dependency. You're simply trying to inject it via a publically settable property instead of a constructor parameter.

You are correct that using a publically settable property is not a good decision, as it opens the door to other issues.
Comparatively, using a constructor parameter allows the same functionality (publically choosing the client) without compromising the encapsulation (not being able to change the client during the object's lifetime).

Therefore, the answer is to use dependency injection.

Flater
  • 10,874
  • 4
  • 33
  • 53
1

We are not using dependency injection since we are creating utility.

That is not a reason. If you use dependencies and want to be able to test it properly, you should use dependency injection. That doesn't mean you cannot provide a default implementation for normal users.

Provide a constructor overload. I have no idea why you think this would be "inefficient".

Example:

public class HttpLog{
      private readonly string uri;
      private readonly RestClient client;

      public HttpLog(string uri) : this(uri, new RestClient()){
      }

      public HttpLog(string uri, RestClient restClient){
          this.uri = uri;
          // notice the initialization of rest client
          this.client = restClient;
      }

     void write(object data){
         this.client.uri = this.uri + endpoint;
         this.client.postAsync(data);
     }           
}
nvoigt
  • 61,531
  • 23
  • 73
  • 116
  • To make my code unit testable is it good practice to expose the properties which break OOP? – user3205479 Oct 17 '18 at 08:25
  • What part of this breaks OOP? – Liam Oct 17 '18 at 08:26
  • I don't think it's either or. Not testing is bad, breaking OOP is bad, I'm sure there is a way to achieve both. This solution does not expose anything for example. – nvoigt Oct 17 '18 at 08:27
  • 2
    I like the constructor overload suggestion; but do want to point out a possible future issue with unit tests losing their purpose, should the constructors not refer to eachother in the future. Your test and runtime would be using different constructors and you're therefore no longer testing the "actual" constructor anymore. That may be an issue if there is logic in the constructor that warrants testing. This is not a problem in the current code (where only the rest client is being set), but may be an issue when this solution is applied to code with larger cunstructor bodies. – Flater Oct 17 '18 at 08:29
  • @Flater While true, that's an issue all throughout your program with every single method. That is what code coverage is for, to make sure your tests actually cover what you want covered. – nvoigt Oct 17 '18 at 08:31
  • @Flater This issue is usually overcome by using a dependency injection framework like [Simple Injector](https://simpleinjector.org/index.html), etc. – Liam Oct 17 '18 at 08:31
  • @nvoigt If I use constructor overload `public HttpLog(string uri, RestClient restClient)`, consumer can provide his own implementation which also works. but if the consumer provides a different version of RestClient then it breaks the utility. Do I need to expose the internal implementation details to the consumer? Why should a consumer care whether I am using RestClient or HttpClient? – user3205479 Oct 17 '18 at 08:32
  • @Liam: I'm fully on board with dependency injection (as per my own posted answer here), I was merely pointing out a relevant caveat if any future readers were to apply this answer to their own code. – Flater Oct 17 '18 at 08:32
  • 1
    I think we're agreeing @Flater :) – Liam Oct 17 '18 at 08:33
  • 1
    @user3205479 `but if the consumer provides a different version of RestClient then it break the utility` Then you are violating the [Liskov Substitution Principle](https://www.tomdalling.com/blog/software-design/solid-class-design-the-liskov-substitution-principle/). When you have a `RestClient` input parameter, **ANY** derivation of `RestClient` should be accepted and not break the application flow. That is a much larger breaking of OOP compared to the other things you've mentioned. – Flater Oct 17 '18 at 08:34
  • 1
    Uh, yes, a *wrong* RestClient will break it. But a *wrong* Uri will break it too and even in your own implementation. That is what happens with bad input. You get bad output. – nvoigt Oct 17 '18 at 08:38
  • @Flater How can we use DI in utility, could you please explain with some code? – user3205479 Oct 17 '18 at 09:28
  • @user3205479: How is a utility class any different from another class that would render it unable to be tested? I can't provide feedback on your assumption unless you explain your justification for making said assumption. – Flater Oct 17 '18 at 09:29
  • @Flater correct me if I am wrong I have already added `HttpLog` please feel to free refactor the code to make it unit testable. Can you explain what changes to make to the code and sample snippet to unit test it as @Nvoigt has shown with constructor overloading in his answer. can you also add some snippet so that it is easy to understand – user3205479 Oct 17 '18 at 09:37
  • 1
    @user3205479: You have asserted that utilities are not unit testable/do not require dependency injection. That is simply not correct. I've already said it's not correct. If you want to know _why_ your assertion is not correct, then I need you to explain how you came to that (erroneous) conclusion in the first place. Showing you a testable example is a one-time fix, but it won't help if you keep believing that utilities should not be unit tested or have injected dependencies. – Flater Oct 17 '18 at 09:51
  • @Flater I am just saying the consumer classes should not have injected dependencies in construtor since those are exposed to client but yes the inner classes which are utitlized in utility can have injections. I totally agree with that for inner classes. I am talking about the classes which are exposed to end consumer. The classes which are exposed to end user should have dependency injection in constructor? If you are building rest API in MVC, user just provides endpoint and the dependencies are resolved by framework. You dont ask the user to provide the service implementations right? – user3205479 Oct 17 '18 at 10:06