4

I'm working on a program that solves a certain type of systems of equations. The main data objects are Equation, Variable, Solution. Then I have this interface, which represents all things that I want to expose to the UI layer.

public interface ISystemSolver
{
    // Configuration
    int NumberBase { get; set; }
    bool CaseSensitive { get; set; }
    bool LeadingZeros { get; set; }

    // State
    IReadOnlyList<int> ForbiddenValues { get; }
    IReadOnlyList<char> ForbiddenVariables { get; }
    event EventHandler OnTooManyVariables;
    bool AreThereTooManyVariables { get; }

    // Variables
    IEnumerable<Variable> Variables { get; }
    void RemoveVariable(int index);
    void AddVariable(char variable, int value);

    // Equations
    IEnumerable<Equation> Equations { get; }
    void RemoveEquation(int index);
    void AddEquation(string equation);

    // Solving
    IEnumerable<Solution> Solve();
}

The class implementing it would certainly violate the single responsible principle. How do I avoid this?

I had this idea: Extract particular regions into interface. For example, the 3 methods that are dealing with variables would form an interface called IVariables.

But then, wouldn't it be a violation of the law of demeter when I need to for example add a variable? Because I would need to call SystemSolver.Variables.AddVariable(...)

Deduplicator
  • 8,591
  • 5
  • 31
  • 50
Patrik Bak
  • 277
  • 1
  • 7
  • For `Law of Demeter` you will introduce method `SystemSolver.AddVariable()`, which will call local `Variable.AddVariable()` – Fabio Aug 04 '17 at 22:16
  • 6
    Note that SRP doesn't mean "should do only one thing." SRP means "should have only one area of responsibility." The "area of responsibility" in this class is "solving a system of equations." If you decide to break it into separate classes, do it because it improves the maintainability of your code or provides some other tangible benefit, not because it might violate some principle. – Robert Harvey Aug 04 '17 at 22:24
  • "The class implementing it would certainly violate the single responsible principle." That's not necessarily true. Without more context we can't provide any useful guidance. – whatsisname Aug 04 '17 at 23:36
  • Consider this: is solving an equation a single responsibility? From the higher level of abstraction, probably this is true. Looking at the individual members, you are looking at a lower layer of abstraction, where the answer is probably false. Such is the contradictory and subjective nature of "responsibility". – Frank Hileman Aug 04 '17 at 23:59
  • 2
    To be frank, whether or not this interface follows SRP is likely the least of the problems with it. Just a consideration of all the error cases this interface allows should make you reconsider it, and minimizing or eliminating these would be more productive in a quite tangible way than trying to make it conform to some vague principle. – Derek Elkins left SE Aug 05 '17 at 01:34
  • 2
    If you are not sure that interface can be created, that would provide future extensibility point, then I would argue not using interface at all. Just expose concrete class, and don't complicate the design with interface that has exactly same API as the single class that implements it. – Euphoric Aug 05 '17 at 04:48
  • Why do you have equation + variable handling inside the solver interface? Are you solving equations that are completely different from each other? (e.g. differential equations vs linear equations vs quaternion equations) – Rufflewind Aug 06 '17 at 03:45
  • @Rufflewind Well. In fact, it's a tool for algebra puzzles like TWO+TWO=FOUR, where distinct letters (variables) represents distinct digits. I provide an option to specify the values to reduce the number of solutions. [like this](https://image.prntscr.com/image/3ZSgZJbLR42Iozx38TDdKg.png) – Patrik Bak Aug 06 '17 at 11:25
  • So where exactly do your various solvers differ on? – Rufflewind Aug 06 '17 at 21:19
  • @Rufflewind I don't think there will be more than one implementation. As Euphoric says, I could have just the single class that implement this functionality. – Patrik Bak Aug 07 '17 at 10:52

1 Answers1

9

First of all, thank you for thinking about your design and trying to follow some principles. I don't enjoy wading through code produced by those that don't.

Law of Demeter

Let me demystify the Law of Demeter for you. It's not a dot counting exercise.

SystemSolver.Variables.AddVariable(...);

Doing that is the same as doing this:

Variables vars = SystemSolver.Variables;
vars.AddVariable(...);

Either way, this can be good or bad depending on the relationships here. If SystemSolver and Variables are intimate friends that were intentionally and explicitly designed to be used this way, such as with a Domain Specific Language that is trying to let you construct something, then sure fine knock your self out. You're using this as it was intended and this can be very powerful and expressive.

But if you're just randomly grabbing stuff because you happen to be able to find it this way you're poring super glue all over the legos. That is, you're deciding that these separate classes must have an intimate and immovable relationship. Making that decision should not be the responsibility of clients using those classes. That should be up to the designer of those classes. If you do this, and someone decides to change the relationship, don't cry because your client broke. You were never promised that this would keep working.

I could go on but I've talked about Demeter before.

Dependency Injection

If you don't want to create such an intimate relationship how are you going to get what you need? You let it be handed to you.

SystemSolver ss = new SystemSolver();
Variables vars = new Variables();
UI ui = new UI(ss, vars);

There, now UI knows about what it needs but they don't even know about each other. Does that make sense? Maybe Variables are meaningless without SystemSolver making them. If so then maybe they do have an intimate relationship. If so then design them that way so dotting around to find what you need is not a violation of Demeter.

Single Responsibility Principle

When something has only one responsibility it should only have one reason to change. That doesn't mean when you look inside you only find one thing. It means everything you find inside is focused on the same thing.

Think about your car. Almost everything in your car is a car thing. The seats are car seats. The tires are car tires. The dash board is a car dash board. The coffee cup is a, wait whats that doing in here? While a lot of things can fit in a car some things aren't things you want to find in there when you buy the car.

Changing the cup shouldn't effect the car. Changing the car shouldn't effect the cup. At most changes to either should only effect the cup holder. That's why we really care about following SRP. It limits the scope of change. I've also talked about this before so I'll stop here. (Man have I been on this site for that long?)

Why You're Not Violating SRP

"[ISystemSolver] represents all things that I want to expose to the UI layer."

That is a single responsibility. It's a big one. But it's still single. You want to break it up, which is good, but SRP isn't why.

You have its methods broken into 5 different groups that are each about one idea. That to me signals 5 responsibilities. Is that bad? No. These responsibilities are all still focused on providing the UI what it needs.

But those 5 should be allowed to cluster together. This reminds me of

The Facade Pattern

Each of those 5 groups could be a separate class. Objects of which are added to the UI's facade class that delegates it's work to each of them. This allows things to separate but keeps the complexity of the structure hidden from the UI. It also would make the Law of Demeter something you no longer have to worry about because everything you need is right there.

Tell, don't ask

You are using a lot of getters and setters. Understand that these break encapsulation and lead to writing procedural code. It's much more powerful to tell objects what to do and let them decide how to do it rather then ask them something and make a decision based on what you learned. I wish I'd written this one but I'm glad I read it.

candied_orange
  • 102,279
  • 24
  • 197
  • 315
  • Thank you for this great answer...Just to clarify: "Each of those 5 groups could be a separate class." So the interface ISystemSolver would have 5 getters, one for each "grouping" class. So I would have calls like `SystemSolver.Variables.AddVariable(...)`, which is fine, because class Variables exists only within a system of equations as it's helper class. – Patrik Bak Aug 05 '17 at 05:33
  • "You are using a lot of getters and setters. Understand that these break encapsulation and lead to writing procedural code." I'm aware of this, but I can't really imagine how I would code this differently. All my getters seem natural to me - they expose information that I directly react on in the UI. How could it be improved in this way? – Patrik Bak Aug 05 '17 at 05:33
  • That's a good question. One I'd be happy to write an answer for it you post it. – candied_orange Aug 05 '17 at 05:43
  • Should I post a new question or just edit this one? – Patrik Bak Aug 05 '17 at 06:09
  • A new one please. Edits should fix mistakes, not change subjects. – candied_orange Aug 05 '17 at 06:10
  • Okay :) And since you haven't reacted on the first comment, I assume I understood it correctly :) Thanks – Patrik Bak Aug 05 '17 at 06:17
  • Yes that looks fine to me. The law of Demeter was never meant to force you to flatten everything to one dot. It's meant to make you realize what you're getting into when you don't. – candied_orange Aug 05 '17 at 06:34
  • Let us [continue this discussion in chat](http://chat.stackexchange.com/rooms/63388/discussion-between-patrik-bak-and-candiedorange). – Patrik Bak Aug 06 '17 at 06:21