On Tue, 23 Feb 2016 13:10:30 +0100 Ancor Gonzalez Sosa <[email protected]> wrote:
> 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 In general both looks strange for me. 1) it use external calculater, I do not see reason for it, do we want to have more kind of calculators? even C++ prefers own operators, so I expect something like total += 3 in ruby or total = total + 3 in C++ world. I am sure there is some team members that do not agree with me :) > > Of course, in some situations this is even better... but that's not > the point in my argumentation > total = total.sum(3) this looks strage. It is add, for me sum(3) is 3 not object plus sum of arguments. btw. I think we should avoid as much side-efects of methods as possible, as it makes using code hard. > > 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. imperative versus declarative :) > > That is, this > creator = PartitionCreator.new > parts = creator.partitions > is preferred over > creator = PartitionCreator.new > creator.create_partitions > creator.partitions both do not look like object oriented approach. I know that libstorage do not use it, but for me clear API is partitions = Partitions.create_on(disk) > > 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). I agree with this and I am sure there is someone who disagree with it as I already pointed that out in past :) > > 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". I think it is important to agree on some design decisions otherwise using library that mix such styles is nightmare as you never know in advance ( so without reading doc ) if given method returns something, have sideeffects, etc. Josef "ready to be criticized" > > Cheers. > > [1] https://github.com/yast/yast-storage/pull/188 > [2] > https://github.com/yast/yast-storage/pull/188/files#diff-095293409316827171cb0dd88b3a6950R113 -- To unsubscribe, e-mail: [email protected] To contact the owner, e-mail: [email protected]
