On Jul 9, 2013, at 7:22 AM, Martin Owens wrote:
I've been looking at the align-and-distribute code and want to clear it up for use with my new functionality.
My idea is to move the core function into selection like this:
Selection::align(Geom::OptRect bbox, AlignSide align, bool as_group);
That way align-and-distribute can specify the bbox from all it's options and other functions can align selections. It should reduce repetition and allow my code to call more directly into the effective code without navigating the ui specific code.
I'd like your thoughts before I spend some time on this refactor.
It might also be worth moving distribute too, I have some great UI ideas for how to do distribute with knot handles too.
In general that seems like a good idea.
However, I think that we might want to have the operation outside of the selection class. That falls in line with a principal of favoring non-member functions.
One could ask "is self aligning an inherit property of the selection-ness of a selection?" In this case it seems to me that it is not. That leaves it similar to STL's algorithms. You can #include <algorithm> to get all sorts of helpful things that can operate on containers, but are not member methods of those container classes.
SomeHelper::align(Selection &selection, Geom::OptRect const &bbox, bool as_group);
Notice also that I flipped bbox to be a reference to a constant OptRect (remember declarations are read right-to-left). Among other things, that avoids duplicating structs as calls are made. Also... "SomeHelper" could be the name of some namespace, and not a class. This seems like it should not require an instance of some object to drive the functionality.
And we can utilize "using" to get that namespace out of the way. At the top of some file using the align functionality, we can have
using SomeHelper::align; ... align(selection, bbox, true);
or perhaps give it a convenient abbreviation for use in a given .cpp file:
namespace xyz = SomeHelper; ... xyz::align(selection, bbox, true);
The latter case might be better, as it keeps the functions marked as to where/why they come from.
Then there is the final parameter. When reading the code they boolean does not convey as much context as to what is going on. Some simple enum can work to keep the code more legible and maintainable:
xyz::align(selection, bbox, ALIGN_FLG_GROUP);
Of course, the enum values need to have short meaningful names. I'm not sure I've captured the essences of the meaning of the boolean, so that name probably needs tuning.