For this Hackweek two main objectives were proposed: (a) learn, understand and become familiar with proposal code, and (b) improve some classes with a special code smell. Let's see each one.
(a) This was harder than I expected. Proposal implementation has numerous related classes and its correct undestanding took me a while. The first two and a half days was dedicated to this task. Algorithms for deleting partitions, distributing free space or selecting best proposal were studied. Right now I have a good understanding of the code and I feel confortable working on it.
(b) Previous this week, that is, during my first two weeks at YaST teem (yes, this Hackweek came in my third week at SUSE), my workmate Ancor and me talked about different parts of the proposal that could improve. Specially, classes as
PlannedVolumeList seem to have more responsibilities than they should. In the rest of the Hackweek time I was working on that. Next, implemented improvements are described.
Add proposed classes
One of these code smells is that
PlannedVolume class was been used for two different purposes. On one hand,
VolumesGenerator populates an instance of
PlannedVolumeList with the desired planned volumes. A planned volume represents a certain volume specification, that is, the range of min and max sizes that it should have, etc. This is the input for
DeviceGraphGenerator, who has to propose partions and logical volumes to allocate these planned volumes. On the other hand,
PlannedVolume is also used for proposing the final partitions or lvs to create and so it has logic for both uses. To avoid that, two new classes have been introduced:
ProposedLv. Really, there is a third new class,
ProposedDevice, that acts as base class.
class ProposedPartition < ProposedDevice ... end class ProposedLv < ProposedDevice ... end
With this approximation,
DeviceGraphGenerator takes planned volumes and generates proposed devices. The rest of classes under
PartitionCreator, etc) always use proposed devices.
PlannedVolumeList class has been collecting a lot of responsabilities that it should not have. For example, the extra space distribution among the partitions of a disk or among the lvs of a volume group. This logic is better placed in the classes for creating partitions and lvs. For that, an
ExtraSpaceAssigner mixing has been created. This mixing is included by
PartitionCreator distributes the extra space of a
FreeDiskSpace among the partitions associated to this free space.
LvmHelper class does the same with the lvs and the extra space of their volume group.
module ExtraSpaceAssigner def distribute_space(proposed_devices, ...) ... end ... end class PartitionCreator include ExtraSpaceAssigner ... end class LvmHelper include ExtraSpaceAssigner ... end
Partitions have to be sorted in most convenient way before creating them.
AssignedSpace class represents an association between a
FreeDiskSpace and a set of proposed partitions for this space. It seems reasonable that
AssignedSpace should be the responsable for sorting its partitions. The logic for that has been moved from
class AssignedSpace def initialize(disk_space, proposed_partitions) ... sort_partitions! end ... def sort_partitions! ... end end
Of course, tests have been updated to reflect all that changes. Currently, all tests are green.
One more thing remains for completing all planned improvements. With new introduced classes,
PlannedVolumeList can be meaningless, so it should be removed at all and, of course, the current code that uses it should be fixed. Also, the doc of all modified methods has to be updated.