OOP decomposition and unit-testing dilemma

Question

Say I have code such as this:

class BookAnalysis {
   final List<ChapterAnalysis> chapterAnalysisList;
 }

class ChapterAnalysis {
   final double averageLettersPerWord;
   final int stylisticMark;
   final int wordCount;
   // ... 20 more fields
 }

 interface BookAnalysisMaker {
   BookAnalysis make(String text);
 }

 class BookAnalysisMakerImpl implements BookAnalysisMaker {
   public BookAnalysis make(String text) {
     String[] chaptersArr = splitIntoChapters(text);

     List<ChapterAnalysis> chapterAnalysisList = new ArrayList<>();
     for(String chapterStr: chaptersArr) {
        ChapterAnalysis chapter = processChapter(chapterStr);
        chapterAnalysisList.add(chapter);
     }

     BookAnalysis book = new BookAnalysis(chapters);
   }

   private ChapterAnalysis processChapter(String chapterStr) {
      // Prepare
      int letterCount = countLetters(chapterStr);
      int wordCount = countWords(chapterStr);
      // ... and 20 more

      // Calculate
      double averageLettersPerWord = letterCount / wordCount;
      int stylisticMark = complexSytlisticAppraising(letterCount, wordCount);
      HumorEvaluation humorEvaluation = evaluateHumor(letterCount, stylisticMark);
      // ... and 20 more

      // Return
      return new ChapterAnalysis(averageLettersPerWord, stylisticMark, wordCount, ...);
   }
 }

In my particular case, I have one more level of nesting (think BookAnalysis -> ChapterAnalysis -> SectionAnalysis) and several more classes on both ChapterAnalysis (think PageAnalysis that each chapter spans) and SectionAnalysis (think FootnotesAnalysis and such) levels. I have a dilemma about how to structure this. The problem is that in processChapter method:

• Both preparation and calculation steps take non-negligible amount of time / resources
• Calculation steps depend on multiple preparation steps

Some concerns:

• The above class, taking into account there are say 20 fields in ChapterAnalysis would be quite long
• Testing the whole one would require a hugely complex preparation method that would test a humongous amount of code. It would be unbearably hard to confirm that e.g. countLetters works as expected, I'd have to unnecessarily replicate almost the whole input just to test two different cases where countLetters behaves differently

Solutions to contain complexity and allow for testabilty:

• Split processChapter into private methods, but then cannot / should not test them
• Splitting into multiple classes, but then I'd need a lot of helper data classes (for each method in calculation phase) or one big kitchen sink (holding all the data from preparation phase)
• Making helper methods package private. While that solves the testing issue in the sense that I can test them, the "I should not" part still applies

Any hints, especially from similar real-world experiences?

Edit: updated naming and added some clarifications based on the current answers.

My main concern with splitting into classes is that it is not linear / one-level. For example, above countLetters produces results that are needed by complexSytlisticAppraising. Let's say it makes sense to make separate classes for both of these (LetterCounter and ComplexSytlisticAppraiser). Now I have to make separate beans for the input of ComplexSytlisticAppraiser.appraise, i.e. something like:

class ComplexSytlisticAppraiserInput {
  final int letterCount;
  final int wordCount;
  // ... 10 more things it might need
}

Which is fine, except that now I have HumorEvaluator for which I need this:

class HumorEvaluatorInput {
  final int letterCount;
  final int stylisticMark;
  // ... 5 more things it might need
}

While this might be done just by listing parameters in many cases, a big issue is return parameters. Even when I have to return two ints, I have to make a separate bean that has those two ints, constructor, equals / hashCode, getters.

class HumorEvaluatorOutput {
   final int letterCount;
   final int stylisticMark;

   public HumorEvaluatorOutput(int letterCount, int stylisticMark) {
      this.letterCount = letterCount;
      this.stylisticMark = stylisticMark;
   }

   public int getLetterCount() {
      return this.letterCount;
   }

   public int getStylisticMark() {
      return this.stylisticMark;
   }

   @Override
   public String toString() {
      StringBuilder sb = new StringBuilder();
      sb.append("HumorEvaluatorOutput [letterCount=");
      sb.append(letterCount);
      sb.append(", stylisticMark=");
      sb.append(stylisticMark);
      sb.append("]");
      return sb.toString();
   }

   @Override
   public int hashCode() {
      final int prime = 31;
      int result = 1;
      result = prime * result + letterCount;
      result = prime * result + stylisticMark;
      return result;
   }

   @Override
   public boolean equals(Object obj) {
      if (this == obj)
         return true;
      if (obj == null)
         return false;
      if (getClass() != obj.getClass())
         return false;
      HumorEvaluatorOutput other = (HumorEvaluatorOutput) obj;
      if (letterCount != other.letterCount)
         return false;
      if (stylisticMark != other.stylisticMark)
         return false;
      return true;
   }
}

That's 2 vs 53 lines of code - yikes!

So all this is fine, but it:

• Is not reusable. Vast majority of these will be used only to make the code testable. Think about analyzers such as: BookAnalyzer, CarAnalyzer, GrainAnalyzer, ToothAnalyzer. They share absolutely nothing in common
• Making 20 classes out of 1 would not yield much except allow for testing
• You can argue that whether it's split into classes or methods, the differene from the point of making the parts small enough to be understood and manipulated is not that big
• On the other hand, there will be a significant amount of noise and indirection added if I were to go proper OOP with testability in mind. Compare:
  • Manage 10 files = 10 analyzers * 1 files with 20 private methods
  • 800 files = 10 analyzers * (20 interfaces, 20 implementions, 20 input and 20 output beans)
  • 400 files if we remove input / output beans and go some other route (such as one big I/O bean per analyzer hack) Note that hundreds of files are going to be very short, mostly boilerplate - probably majority of logic will be under 10 lines each (== private method in the first case)
• There's significant overhead in this. If I were to call a private method 1m times, creating additional input and output beans is adding up...

Probably doing the right thing is, well, doing the right thing. Just wanted to see if there are some other options I can pursue that I'm missing. Or is my logic just purely bad?

Edit: Additional update based on the comments. We can make HumorEvaluatorOutput shorter, not a huge issue:

class HumorEvaluatorOutput {
   final HumorCategoryEnum humorCategory;
   final int humorousWordsCount;

   public HumorEvaluatorOutput(HumorCategoryEnum humorCategory, int humorousWordsCount) {
      this.humorCategory = humorCategory;
      this.humorousWordsCount = humorousWordsCount;
   }

   public HumorCategoryEnum getHumorCategory() {
      return this.humorCategory;
   }

   public int getHumorousWordsCount() {
      return this.humorousWordsCount;
   }
}

That's 2 vs 17 lines of code - still yikes! It's not much when you consider a single example. When you have 20 different analyzers (BookAnalyzer, CarAnalyzer, ...) with 20 different sub-analyzers (for Book as above: ComplexSytlisticAppraiser and HumorEvaluator and similar for all other analyzers, obviously much different categories), the 8 fold increase in code adds up.

As for BookAnalyzer vs CarAnalyzer and Book vs Chapter sub-analyzers - actually, I need to compare BookAnalyzer vs CarAnalyzer, as that's what I will have. I will definitely reuse Chapter sub-analyzer for all chapters. I will however not re-use it for any other analyzers. I.e. I'll have this:

BookAnalyzer
  ChapterSubAnalyzer
  HumorSubAnalyzer
  ... // 25 more
CarAnalyzer
  EngineSubAnalyzer
  DrivertrainSubAnalyzer
  ... // 15 more
GrainAnalyzer
  LiquidContentSubAnalyzer
  FiberContentSubAnalyzer
  ... // 20 more

Going by the above, instead of 1 class per analyzer, I now have to create 20 interfaces, 20 extremely short sub-classes with 20 input / output beans and none will ever be re-used. Analyzing Books and Cars is rarely using the same approach and same steps anywhere in the process.

Again - I'm fine with doing the above, but I just don't see any benefit except to allow for testing. It's like driving Toyota Thundra to your next-door neighbor's party. Can you do it just to be the same as all other people coming to the party? Certainly. Should you do it? Ehhh...

So:

• Is it really better to make 500 lines in 10 files into 5000 lines in 800 files (probably not the fully correct numbers, but you get the point) just to follow OOP and enable for testing?
• If not, how are other people doing it and still keeping on the side of not breaking OOP / testing "rules" (e.g. by using reflection to test private methods which should not be tested in the first place)?
• If yes and everyone else is doing it like that, fine. Actually, a sub-question then - how do you manage to find what you need there and follow the flow of the app in all that noise?

Answer 1

This question may be more suited to CodeReview. That said, it feels like you already know the solution is to break down the class into smaller classes so that they are easier to test.

Looking at BookMakerImpl, it already seems to be doing at least two distinct jobs. It is both splitting text into sections and performing analysis on those sections. On second glance, it's also unclear if you have a naming issue. Chapter doesn't really represent a chapter (as I would expect) in the code sample you've provided. It's actually representing your analysis results for a given chapter (you don't seem to pass the chapter text into it's constructor, although this may be an omission in your posted code).

One approach you might take to simplify the testing (assuming I am correct about what Chapter represents, if not the approach would be similar, but the names and elements would obviously need to change) is to extract the analysis into one (or more classes). With the code you've provided it looks like you'd be able to create something like a ChapterTextAnalyser class. This would take in a string (in the example provided it would be the chapter text) and then return the results in something like a ChapterAnalysis (replacing your current Chapter class).

If you have similar analysis between Chapters and other sections then this structure might need reworked to make sense in the given domain and share functionality where appropriate, but essentially you could have something similar to this (pseudo code)...

class BookAnalyserImpl implements BookAnalyser
    // Pass in analyser factory and book parser
    // to constructor so mocked version can
    // be used for testing
    public BookAnalyserImpl(TextAnalyserFactory textAnalyserFactory,
                            BookParser bookParser) {
        if(null != textAnalyserFactory) {
           mTextAnalyserFactory = textAnalyserFactory;
        } else {
           mTextAnalyserFactory = new AnalyserFactoryImpl();
        }
        // Same for bookParser
    }
    BookAnalysis analyse(String bookText) {
        BookAnalysis bookAnalysis = new BookAnalysis();
        ChapterAnalyser chapterAnalyser = mTextAnalyserFactory.GetChapterAnalyser();

        foreach(chapterText in mBookParser.splitIntoChapters(bookText)) {
            bookAnalysis.AddChapterAnalysis(chapterAnalyser.analyse(chapterText));
        }
    }
}

class TextAnalyserFactoryImpl implements TextAnalyserFactory {
    ChapterAnalyser GetChapterAnalyser() {...}
}

class ChapterAnalyserImpl implements ChapterAnalyser {
     ChapterAnalysis analyse(String chapterText) { ... }
}

As you've said, this will result in you having many more classes. This isn't, in itself a bad thing, if the classes make sense and have distinct responsibilities.

If you don't like the idea of having a lot of classes, then you could simply push the analysis out into another class that has a public interface.

class BookAnalyser {
    ChapterAnalysis analyseChapter(String text)  { ... }
    PageAnalysis analysePage(String text) {...}
    // ...
}

This dodges the private testing issue by making the methods you want to call a part of the function of the class you're calling them on.

In response to some of your edits:

Firstly, it's important to remember that OOP is optional, it's perfectly valid to take an alternate approach to solving your problem.

Are you really writing software that is analysing Books, Cars, Grain and Teeth? It feels somewhat contrived, which makes the problem space difficult to buy into and hence understand, which is magnified by the fact that your coding examples are incomplete. Whilst in the current iteration of your problem domain there is no visible commonality between the analysers it's not hard to imagine areas where the analysis could be similar. Porosity analysis for example could be applied to Grain, Teeth and the Pages of a book to give meaningful information. However, your book analysis is based on plain text input so this is unlikely to be part of your problem domain, at least for Book.

Is it really better to make 500 lines in 10 files into 5000 lines in 800 files (probably not the fully correct numbers, but you get the point) just to follow OOP and enable for testing?

I'm not a huge fan of lines of code as a metric for software complexity. Your initial comparison was 2:53, which you've dropped to 2:17 in C#, the ratio would be closer to 2:9, although really the difference is 5 (number of lines boilerplate) + 2 * number of fields (one line for assignment and one for get). Is using if..else... (5 lines) significantly more verbose/less clear than the 1 line ternary operator? It's very subjective, I've worked in places that had coding standards saying you couldn't use the ternary operator.

Is 4000000 lines of code better than 5000, it seems unlikely. But I'm also sceptical that if you broke the problem domain down to extract distinct functionality and commonality that this would be the outcome.

Consider this line from your code

HumorEvaluation humorEvaluation = evaluateHumor(letterCount, stylisticMark);

You're not doing anything with humorEvaluation evaluation, however it seems like this represents a distinct thing. It seems like this would be passed into the ChapterAnalysis that's constructed in the method and stored there. This removes the need to store the individual fields that make up the HumorEvaluation at the chapter level. This distinct type also seems like it represents the same concept that you're representing with the HumorEvaluatorOutput. There's no need to represent the same concept twice unless you get some benefit from doing so. Here you don't appear to be getting that, so throw it away.

If not, how are other people doing it and still keeping on the side of not breaking OOP / testing "rules" (e.g. by using reflection to test private methods which should not be tested in the first place)?

I don't like directly testing private methods. They are an implementation detail and testing them directly is brittle. From the testers perspective it shouldn't matter if a private method exists, or it is written all in line in the method being called from the test. What is important is the measurable side effects of the code as a whole. You say:

Testing the whole one would require a hugely complex preparation method that would test a humongous amount of code. It would be unbearably hard to confirm that e.g. countLetters works as expected, I'd have to unnecessarily replicate almost the whole input just to test two different cases where countLetters behaves differently

The reality is that from the tests perspective what is important is that the state of the constructed Chapter is correct. If countLetters works as expected then it will be. If not, then the test will fail. In particular, if countLetters didn't return the number you're expecting then the relationship between your expected averageLettersPerWord and expected wordCount on the Chapter class would be incorrect.

Looking at the some of your methods , countLetters, countWords they have distinct inputs and outputs and don't modify the state of the class that they are in. As I've said before, this suggests that they could be in a different class where it made sense for them to be public.

class GenericTextAnalyserImpl implements GenericTextAnalyser {
    int countLetters(String text);
    int countWords(String text);
    int complexSytlisticAppraising(int letterCount, int wordCount);
    // ...
}

countLetters is presumably something that might be used by other analysers (Book, Chapter, Page etc). These methods no longer have to be duplicated into these other classes and testing of these methods becomes trivial. It also allows testing of elements such as Book to be reduced since you can Mock the calls to ensure that the correct calls are being made rather than having to replicate the whole call structure for every single variation you want to test.

If yes and everyone else is doing it like that, fine. Actually, a sub-question then - how do you manage to find what you need there and follow the flow of the app in all that noise?

If you turn 5000 lines into 4000000 by adding 800 extra classes then you probably don't find your way around. If you create a sensible set of classes that break your problem down into areas and elements within those areas then it's usually not that hard.

Answer 2

You are right that it is not generally considered good practice to test private methods. It is, however, important to understand why this is the case, because only then you can judge if it applies in your case.

The main argument against testing private methods is the expected increased maintenance effort for the test code: If the software is properly designed, private elements are more likely to be changed than public elements. Therefore, the effort to keep test code building and working correctly is smaller if the tests use the public API only. (They also should preferably only use black-box wisdom about the code under test, but that's a different story...)

Looking at your example, I take it that the method countLetters is probably private. The name, however, gives me the impression that this method might implement a well understood and stable concept within your code. If that is the case, some alternative design option would be to factor out this method into a class of its own - it wouldn't be private then.

This thought, however, is not meant to suggest that you factor out this function (you could do so, but that is not my point). The point is to make it clear that it all boils down to the question of how stable some piece of code is expected to be.

This expectation with respect to stability has to be weighed against the efforts for testing: Testing the private elements can be easier in the first place (which saves you effort), but it may cost you in the long term. It may still be, that the long term costs will never outweigh the short term wins. This you have to judge.