2

Let's say I'm writing a class that tracks a single stock ticker. In this simplified example, the StockTracker class holds a string that tells me the trend direction and a variable window that holds x amount of historical prices.

Now I want to expose this class so whenever a new stock price comes in, I can call a function to update the stats and trend for this stock. So I make a public function that calls two private functions, each having one responsibility each

class StockTracker(val window: List<Double>, val trendDirection: String) {

    private fun updateWindow(xxx) {
        // Long business logic goes here
    }

    private fun updateTrend(xxx) {
        // Long business logic goes here
    }

    fun ingestNewRate(newRate) {
        updateWindow(newRate)
        updateTrend(newRate)
    }
}

In this scenario:

  1. Am I following the single responsibility principle? Technically the function ingestNewRate is doing two things: updating stats and updating the trend. Is this fine?
  2. When unit testing this, would I have to test the 2 private functions?
  3. Should I be exposing updateWindow and updateTrend in new classes so that they can be a publically available function that can be tested?
  4. Is there anything else I should be looking out for in this example that is potentially bad practice?
2
  • 2
    Does this answer your question? How to determine if a class meets the single responsibility principle? – gnat Aug 30 '20 at 5:03
  • #2: must test private code, but ... #3: No. Choose public function arguments so that internal correctness can be inferred from results/output/side effects. #1: Look for tell-tale signs, there are many. Then intended design, requirements & domain objects mediate judgement. Objectively, exposing state is a code smell. SRP exposes functionality and hides state. "maximize coherence and minimize cohesion" is not about reuseability, just sayin'. Pro Tip: Embrace tiny classes, building into complex classes. Resulting clarity, managed complexity, testability, quality can feel like magic; no kidding. – radarbob Aug 31 '20 at 17:29
6

The problem here is not about SRP or do-one-thing. This is about how you're storing your state.

Under SRP StockTrackeris perfectly fine. SRP is not against having different methods or composing "functions". It's against lumping together, thru composition or whatever else, different reasons to change the code. No class should serve more than one master. Two masters can demand conflicting changes to the code. There is a problem here but that's not it.

ingestNewRate() follows do-one-thing so long as the code underneath focuses on ingesting a new rate. updateWindow and updateTrend both seem to focus on ingesting the new rate. Just into different things. The name ingestNewRate has a singular focus. Just don't betray the name. This isn't the problem either.

Here's the problem:

You are storing redundant information. Redundant state means potentially conflicting state. If window: List<Double> shows that the trend direction is up but trendDirection: String show's that it's down then which is correct?

What you're violating here is called Single Source of Truth. It's only very loosely related to SRP but it's a different idea. This is about avoiding conflicting state by storing that state only once. Not in two potentially conflicting ways.

What you need is a public method that expresses some useful work that the object has done with that state. If you were only storing window: List<Double> then you could calculate "up" or "down" from the last two doubles. Since "up" and "down" are no longer internal state this doesn't break encapsulation. That method could be toString(). It could be calculated against window: List<Double> every time it's called.

Write your tests against that public toString(), not any private methods, and you're free to change your private structure without breaking your tests. The outside world doesn't need to know you changed anything. We call that refactoring. Don't prevent refactoring by writing tests that lock you into particular implementations. Tests should prove what code does. Not how it does it.

0
2

This seems not really about Uncle Bob’s Single Responsibility Principle, which is about classes and their reason to change.

But it is definitively about the do-one-thing rule, recalled by the same author, and which is about functions and methods.

In this regard you did well: your top function does one thing: ensuring that the stock price is added. This involves several operations, that are organized according to functional decomposition, delegating to other functions the doing of each of these more detailed operation. And moreover you respected the single level of abstraction rule.

All your other questions are not directly related to these principles:

  • tests depend on your test strategy. The unit test is meant to test the elementary unit of functionality delivered. I’d understand it as testing the public functions. The private functions are behind the scene and only relevant if you know the internals;
  • what to expose publicly is yet another question, and only you can decide: could the stats or trend calculations make any sense on their own?
  • Finally, even if you keep your auxiliary functions private, you could encapsulate them in a strategy. Not for SRP, not for Do-One-Thing, not for tests, but because it makes your design more flexible. This makes only sense if you could envisage using alternate implementations/formulas/algorithms. In this case the auxiliary methods would be public within the strategy class, and hence unit testable on their own.
-3

The two functions updateWindow and updateTrend should be implemented in classes that implement an interface (lets say an IUpdatable interface).

IUpdatable
  Update(...)

StockTrackerWindow : IUpdatable
  Update(...) 

StockTrackerTrend : IUpdatable
  Update(...) 

The StockTrader class can then delegate the updating of window and trend, and anything else in the future, to these concrete classes.

class StockTracker(val window: List<Double>, val trendDirection: String) {
 
    List<IUpdatable> updatables;

    fun ingestNewRate(newRate) {
        updatables.ForEach(Update(newRate));
    }
}

In this way the StockTracker has a single responsibility, to update each of the updatables. The concrete classes StockTrackerWindow and StockTrackerTrend each have a single responsibility and can be unit tested separately.

In terms of testing you can now used Mock to inject whatever behavior you want into the StockTracker; even mocks that throw exceptions.

The design of StockTrader now depends on abstractions.

4
  • Btw, the interfaces might be better named IStockTraderUpdatable. – null Aug 30 '20 at 17:09
  • 1
    why did someone down vote this? – null Aug 30 '20 at 18:20
  • I didn't downvote but I suspect I know what's going on here. You've focused on the word update and have proposed an event driven observer pattern solution. However, strings and collections aren't the kind of things you send updates to, so this idea is falling a little flat. – candied_orange Aug 31 '20 at 6:04
  • in the original question the comment '// Long business logic goes here' was very very smelly. It indicated that many there was more responsibilities there. The original poster sees it too. Their questions suggested they are thinking similar thoughts. – null Aug 31 '20 at 18: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.