[yast-devel] Refactoring madness: chapter one
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-095293409316827171c... -- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On Tue, 23 Feb 2016 13:10:30 +0100
Ancor Gonzalez Sosa
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-095293409316827171c...
-- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
On 02/23/2016 03:42 PM, Josef Reidinger wrote:
On Tue, 23 Feb 2016 13:10:30 +0100 Ancor Gonzalez Sosa
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 :)
Please, don't focus on the non-sense semantic of the bad examples. I just wanted to have names that were more meaningful than "Object.operation(operand)" to illustrate the avoidance of I/O arguments. Of course, "calculator" and "sum" would not be the way to go, they are just random picked names.
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.
Once again, that was not the point. Just change "Calculator" by "Foo" and "sum" by "bar" and re-read the whole thing. :-)
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)
Once again, out of my point but I will take the bait. :-) The example is based on this class https://github.com/yast/yast-storage/blob/new-proposal/src/lib/storage/propo... It was proposed as a separate class by HuHa and I agreed (although I proposed some API changes) in order to follow the single responsibility principle. We didn't want to put that specific responsibility (one of the steps of the proposal) into Devicegraph, but in a separate place (inside the Proposal namespace). But as said, don't focus too much on the low quality of the semantic of the examples.
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-095293409316827171c...
-- Ancor González Sosa YaST Team at SUSE Linux GmbH -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org
participants (2)
-
Ancor Gonzalez Sosa
-
Josef Reidinger