Refactoring: Simplification of SectionBuilder, VolumeImpl and VolumeP…

[PaulRambags]

This is a simplification of the current code. No functionality has changed.
This may be regarded as a first step to eliminate the sheet level.

@bertfrees @kalaspuffar Please review this PR, many thanks!

[bertfrees]

Let me ask first: how many PRs like these are going to follow? :-) I mean, I just want to make sure I don't spend more time than needed on reviewing. I can already see a few things about this commit that I don't like so much, but it all depends on where you want to go with it.

If this PR is a preparation to what is going to follow, I'd rather review it when I've seen more.

Or do you say this PR stands on its own, with code simplification as goal? In that case there are a number of things I would do differently.

[PaulRambags]

Or do you say this PR stands on its own, with code simplification as goal? In that case there are a number of things I would do differently.

Yes that is the case.
Please go ahead and shoot @bertfrees. It doesn't make sense to continue if this is not the right approach. I've tried to unravel and simplify some parts of the current code. I have no plans yet to spend more time on the removal of the sheet level.

[bertfrees]

OK here it goes :-)

The two most important things that make Dotify code hard to work with are in my opinion:

• Fragmentation and the abundance of classes. Classes that often do nothing more than add boilerplate and do not really encapsulate anything. Classes that are often abstract or misleading in terms of what they represent.
• Shared state and mutability.

In other words, the typical problems you encounter with a lot of object-oriented code. We should keep these things in mind when refactoring code. We have to question whether certain classes really need to keep existing.

Applied to SectionBuilder, VolumeImpl and VolumeProvider:

• I think it's probably good that you moved some code out of SectionBuilder. However what is left now is just a class with a single static method. That is rather pointless (unless maybe you want to use this function elsewhere but that is not the case). So we can ditch SectionBuilder.
• I can see some value in the Volume interface, because it is a very natural way of representing a volume, even if it's just a group of sections. It helps making the interfaces for passing around and consuming this data more understandable. However what you have done now is you made it into something mutable for no reason. Just drop the setSections, it is not needed. I would even say it is not needed on the VolumeImpl class either (read on).
• My problem with VolumeImpl is that it does not only implement Volume but also serves as a container for other state needed by VolumeProvider.nextVolume() (namely bodyVolSize and overhead). There is no reason for not simply putting that stuff in VolumeProvider.nextVolume(). It'll mean less boilerplate and less confusion about what the object does. This way VolumeImpl becomes a very simple immutable class with only a constructor that takes the sections, and the getSections method. Or, you can even drop it and replace it with an anonymous class, or because it is a functional interface, a lambda (this is what I would usually do but not everyone likes anonymous classes).