I recently invested quite some time (out of the sprint, my fault) refactoring the code of the new proposal. I didn't change the approach, the main algorithm or the produced result. So finally I changed everything for everything to stay the same. [1]
I did it just because I disliked a number of things about the code organization. Code organization is a subjective question, so I though I should write a couple of mails with my reasons to make all those changes. Disclaimer: this is a highly opinionated (and veeery long) email. In this mail, I will focus on stuff that I think is kind of specific to "the Ruby way" of doing things. I will keep more general OOP stuff (like single responsibility, class dependencies, messages-driven design, etc.) out of the discussion for the time being, although it also motivated many of the introduced changes. 1) Avoid input/output arguments when possible. That is, in Ruby this puts total # => 2 total = calculator.sum(total, 3) puts total # => 5 is preferred over this puts total # => 2 calculator.sum(total, 3) puts total # => 5 Of course, in some situations this is even better... but that's not the point in my argumentation total = total.sum(3) 2) Try be as functional as possible. With methods that returns something, no matter if that thing is implemented through storage or through computation. That is, don't tell your objects what to do, tell them what you want to get. That is, this creator = PartitionCreator.new parts = creator.partitions is preferred over creator = PartitionCreator.new creator.create_partitions creator.partitions 3) Don't hesitate to create "volatile" objects and drop them, relying on the garbage collector to do its job. I will elaborate on this one. The strategy for the proposal was: use the layout read from the system (a.k.a. probed) as a starting point. Create a copy of it and run step1 on it with a set of parameters. If it failed, then discard the result and start again with a copy of "probed" trying different parameters for step1. On the other hand, if step1 was successful, consolidate that intermediate result and do a similar thing with step2 (rolling back to post-successful-step1 when needed). Good strategy... but I completely disagreed with the implementation. We have a Yast::StorageManager singleton class that acts as an interface to libstorage and that holds a catalog of devicegraphs (indexed by name). The original implementation of the proposal included the registration of three devicegraphs on that catalog: "proposal" (the working copy), "proposal_base" (the consolidated intermediate result) and "staging" (used to calculate the actions). Those devicegraphs were created as copies of any other and then passed down as I/O arguments to specialized classes performing the steps. It was usually not that obvious who had the responsibility of rolling back "proposal" to the content of "proposal_base" and so on. It was also not clear who was responsible of cleaning up the catalog after the whole process took place. My refactoring does not use that catalog at all. Moreover, it contains minimal references to that singleton object. Like here [2], in which the reference is "hidden" after a Devicegraph.probed method (in which dependency injection is used, btw). The new code creates "volatile" (in the sense of "unnamed", not indexed in a catalog) devicegraph objects. Those objects gets discarded when they are replaced with another copy resulting of a successful step (no more I/O arguments to step1) or simply when they prove to not be longer worth holding (for example, step1 failed). No globally named and registered stuff, just "a lot" of work for the garbage collector (actually trivial for the ruby GC capabilities). That's all for this mail. Sorry for the long text (and for threatening you with even more chapters), but I think that we need to agree in best practices and, even more important, on how much relevance do we want to give to them. Like "was such an investment of time in changing a perfectly working code really worth it?". I wanted to give some of my (opinionated) reasons to reply "yes". Cheers. [1] https://github.com/yast/yast-storage/pull/188 [2] https://github.com/yast/yast-storage/pull/188/files#diff-095293409316827171cb0dd88b3a6950R113 -- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: [email protected] To contact the owner, e-mail: [email protected]
