On 02/24/2016 05:19 PM, Josef Reidinger wrote:
On Wed, 24 Feb 2016 15:29:28 +0100 Ancor Gonzalez Sosa
wrote: 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.
For me DeviceGraph is the most obvious risk point for being "god" object. So from your examples. Free slots is something that can be part of proposal as there is not much other users. It can be also computed dynamically outside of DeviceGraph class.
I forgot to add that it is actually something specific to the proposal, because those new methods are indeed added to devicegraph using a refinement that is only used during proposal-related code. So: a_devicegraph.candidate_spaces #=> NoMethodError module Example using Proposal::RefinedDevicegrah a_devicegraph.candidate_spaces #=> A valid array of spaces end
Regarding disk used in installation I think it make more sense to have object Installation, that knows it and keep DeviceGraph really only graph of devices without other knowledge ( like that there is something like installation as libstorage can be used also outside of installation or e.g. for kiwi which work with teoretic disk only, so you can later specialize Installation for more use cases ).
Hmmm, that could make sense... but at some point in entered in "just finish this" mode and stopped thinking further ahead.
So too sum it I agree with simple responsibility purpose and in some case more then one responsibility is fine unless class became too big, but I really disagree with mixing different abstraction levels in one class and for me holding devices, searching ( or caching ) free slots and knowing what disk use installation as very different abstraction level. -- 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