I can't decide how to refactor code right. Let's say I have a complex hypothetical logic piece:

public class ImageService
     public void UploadImage(VectorData data) {
         var image = ConvertVectorDataToImage();
         var thumbnail = CreateThumbnail(image);

(If something in example is out of single responsibility, let's pretend, that I could make up a better example)

If I make UploadImage visible (public), then writing tests become complicated. They tend to be long, hard to read and if you would like to avoid code duplication, then logic in tests appears.

If I make all those small pieces visible, the API becomes messy (because in my example, the only interesting thing to everyone is UploadImage method). Even worse, after some time people will tend to use those small methods, because they are public, not because they should be used outside the class.

So my question is, how do you balance between visibility and test complexity? Or am I doing something wrong in first place?

  • 1
    One thing to note is I would make as much code as possible reuseable, in which case public isn't a problem. Why a private SaveImage method? Refactor it so you can reuse it in any project. – Steven Jeuris Apr 20 '11 at 10:31
  • 2
    Well, i think it depends. One thing, that it requires much more effort to publish methods, that are not intended to be published - they may depend on inner class state, may be required to be called in some sequence - all those problems are solvable, but i think publishing them just because of some day they will be reused breaks principle "you ain't gonna need this", plus instead of giving class to consumer with one usable method and leaving no doubt how to use it, you give class with bunch of methods and user needs to figure which one to use. – Giedrius Apr 20 '11 at 11:51

First you need to keep that you should favor composition over inheritance. This means that instead of trying to avoid code duplication by subclassing you'll need to compose the behavior from a set of classes that implement that behavior.

You need not worry about the visibility of all those classes, as the interfaces will be the only visible API the clients will see.

If other developers reuse some of the code, isn't that just a good thing? That means you've succeeded in creating reusable building blocks.

  • I liked idea about interfaces as additional access modifier, and I guess that kind of answers my question. Regarding reuse, I was talking just about reuse in tests, if you have complex method, that requires 100 lines just to initialize test for one case, for second case you will try to reuse all that initialization, that reuse will need some corrections in parameters or smth - and here you go, you have logic in your tests, which you should prefer to avoid. – Giedrius Apr 20 '11 at 11:53
  • Yes, that's why TDD works. It encourages you to split your code up into smaller reusable chunks. It also helps minimizing the Arrange part of your unit tests. – Mark Seemann Apr 20 '11 at 12:03

You should start using a mocking framework and code to interfaces.

Use atomic interfaces that describe the tasks that you want to perform as far as possible use SRP:

IImageConvertor { ConvertVectorDataToImage }
IThumbnailWorker { CreateThumbnail; SaveThumbnail }


Now your class that does the image upload should have data members for each of these interfaces:

public class ImageService
    IImageConvertor _imageConvertor;
    IThumbnailWorker _thumbnailWorker;

and then in your method you call

public void UploadImage(VectorData data) {
     var image = _imageConvertor.ConvertVectorDataToImage();
     var thumbnail = _thumbnailWorker.CreateThumbnail(image);

Now your mocking framework takes over, so in testing you create mock objects for your interfaces and your tests just verify that these interfaces have the correct methods called and that they are called in the correct order. You don't actually do any image manipulation, or any file access. This keeps your tests fast and really helps to keep your code decoupled and modular.

You can test the objects that underlie each of those interfaces separately. In general people don't test things like file access, because they are generally handled by the language or some third party API, all you are doing is wrapping them and any bugs are picked up pretty quickly in unit testing. Whether you think that is a good idea or not is another matter.

public class MyClass {
  void defaultMethod() { ... } // visible only to classes from the same package

public class MyClassTest {
  void defaultMethodTest() { ... testInst.defaultMethod(); ... } // works fine
  • 1
    while this works, it kinda is cheating imho. Cheating in the sense that the problem of a class mixing up responsibilities isn't fixed, but bandaided over (using default visibility to restrict outside calls, but still available to tests). – Chii Apr 20 '11 at 12:49
  • @Chii - I completely agree and in general I favor the Mark Seemann's answer. The approach I suggest is (a) doing exactly what author wants, and (b) is sometimes a "least blood" way to add unit tests post-factum, after the code has been written. – bobah Apr 20 '11 at 12:59

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.