This is a follow-up on my highly opinionated mail about software design. Be warned, it's much longer than chapter one. Sorry for that.
This mail contains concrete examples from the recent yast2-storage rewrite in order to touch some solid ground (simplistic improvised examples didn't work very well in chapter 1). Please, please, PLEASE, don't take it as an attack to the old code or their writers. Once again, this is just as an explained dump of my very own mental checklist of things I care when writing or reviewing code. Meant as a starting point for discussion... but not via mailing list, since we want to get to a conclusion at some point. :-) 1) Responsibility distribution We all agree this is a key aspect of OOP design. But we seem to usually disagree on the result. Single responsibility principle is a nice guideline here... when not taken too far. Going for examples, the original SpaceMaker had the obvious responsibility of resizing and deleting partitions to make space, but also of holding the list of free slots (something I moved to Devicegraph) and of deciding/knowing what was the target size (:desired vs :minimum) for the following steps (something I moved one level up to the Proposal class). So the resulting SpaceMaker was more dumb... while other classes became... well, less dumb. We also had a separate DiskAnalyzer class that other classes queried to know stuff like what's the list of existing windows partitions, which media is being used for installation (we don't want to install on top of ourselves) and that kind of stuff. I "hided" the DiskAnalyzer class behind Devicegraph. So now other classes query those questions directly to the devicegraph object. After all those changes, somebody could argue that Devicegraph responsibility became to broad, against the single responsibility principle. For me it makes sense that devicegraph knows its own list of free slots or which one of its own disks are being used for installation. Simply because it makes sense(TM) and I "feel" it's its responsibility. I want my API to hide the fact that some of those calculations are delegated to a DiskAnalyzer class. The main advantage is that the "users" of Devicegraph don't have to know several other classes just to ask things about the system layout... that leads me to the next point. 2) Keep the dependencies between your classes to a minimum. Another topic in which one could thing that we are going to agree easily... but was not the case. The best thing I can do in that regard is just recommending the chapter titled "Managing dependencies" from the Sandi Metz book[1]. She simply put into words my view about the topic. Including a fairly obvious but still useful checklist on when an object has a dependency. That happens when the object knows: - The name of another class - The name of a message that it intends to send to someone other than self. - The arguments that a message requires and/or the order of those arguments Everytime I see that in a class, my dependencies alarm rings and I start asking myself: Does it make sense? How can avoid that? I really try hard to fight every dependency until I accept it must be there. And lately I find myself using dependency injection more and more. 3) Don't let singleton objects pollute your APIs In some situations, the singleton pattern is a necessary evil. But please restrict the evilness as much as possible. The original code had StorageManager.instance.probed (or similar stuff) in too many places. Several classes (DiskAnalyzer, PartitionCreator, SpaceMaker and StorageProposal) knew were to find the probed devicegraph by the holy grace of Singleton. The modified code hides that singleton behind the Devicegraph.probed class method and reads the probed devicegraph only once. Devicegraph is the only class that knows there is a singleton StorageManager. The devicegraph returned by it is then simply passed as argument to any other class needing it. Thus, DiskAnalyzer (and others) doesn't know anymore about the existence of StorageManager.instance, it just receive a devicegraph to analyze (the fact that this devicegraph is always the probed one is not DiskAnalyzer's business). The example in (4) has also to do with singleton objects. 4) Messages-driven design I can also become quite picky about who sends messages to who (like I did in [2]). In fact, that is an example of two thing I consider wrong. One is sending the messages to the wrong receiver. Another one is pollution caused by singleton. You send messages to an instance of FakeProbing to produce an effect on StorageManager.instance.probed, but there is no reference to StorageManager during that interaction. It just works because FakeProbing and the caller both know about StorageManager being singleton. Talking about messages, emitters, receivers and and all that can look too philosophical. But for me is the key factor when defining an API (I even draw UML interaction diagrams sometimes!). And for me, ending up with a sane API is the key reason for having code reviews. 5) A minor one: don't abuse well known names That's something I do all the time, but I'm trying to improve that. It really hurts the reviewer mind when something is called "factory", "singleton", "strategy", "presenter" and so on but doesn't follow the corresponding Factory, Strategy, WhatsNot patterns. To stop doing that when writing code is one of my new year's resolutions (because I suffer it when reading other's people code). 6) Single responsibility principle... again In (1) I said I can be considered slightly relaxed when applying the single responsibility principle to objects, at least to a purist eyes. I must clarify that I really observe that principle when writing methods within a class, so I end up with a lot of small methods that rely on each other. Not sure if that makes my code easier or more difficult to follow. It depends on the reader, I guess. Well, I think this is enough for a second mail. I guess I have already covered my most important reasons to "slow down" the delivery of already working code in the yast2-storage rewrite. Cheers. [1] http://www.sandimetz.com/products [2] https://github.com/yast/yast-storage/pull/193#discussion_r53643452 -- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: [email protected] To contact the owner, e-mail: [email protected]
