Hah, I didn't even notice the name discrepancy in the last one. I was mostly looking at the
size discrepancy. It also lacks setting
type, as well as any code to set
text or the
click handler.
I would also recommend not using initializer functions with 15 parameters. That would be horrible to use. I was thinking about your class hierarchy a bit yesterday though, and it got me thinking about a few possible alternate solutions.
For instance, you have a
Button class, which handles different types of buttons. The type of button is controlled by the
mType member variable, which is set by the
type method. It seems highly unlikely the type of a button would change after construction. Rather than set the type with a
type method, I think it should be set during construction only, with no way to change it afterwards. Now you could do that by moving the initialization to the constructor as a prameter, or you could eliminate the field entirely by creating subclasses of button. Using the subclass approach, the type is then encoded in the type system, using the subclass name, rather than a member variable.
ToggleButton irradiateColonists;
Button closeReactorControlWindow;
Another thought I had on a slightly different dimension, is some buttons have only
text set, while others have only
image set. Every button seems to have at least one of those set. I'm not sure it would make sense to create a button without one of those fields, as otherwise it has no way to indicate to the user what the intent of the button is. I'm not sure if you ever set both, though it seems reasonable. For most buttons, the text or image would remain constant for the lifetime of the button. For a few, it seems reasonable for either the text or an image to be updated after the button is created, perhaps in response to changing conditions.
class Button {
// ...
// Makes sense to allow update after construction
void text(const std::string& text);
void image(const std::string& path);
// ...
};
Now, if a button needs at least one of
text or
image set (and probably only has one set), it makes sense to set it in the constructor. You might even argue that you don't really have a valid button without one of those fields set. To set one of those two fields, the class could offer two constructor overloads. If you're taking the subclass approach, these constructors would likely go in the base class. One complication, is both text and image are currently being set as simple text strings, and so you can't overload on the type alone. That could be fixed though by wrapping one of them in a type, which makes a lot of sense for the image. In particular, the image is already wrapped in a type. When you pass an image path, the Button doesn't directly store the path. Instead, it constructs an Image object using the path. An alternative then, is to accept an
Image object as a parameter, and that would then allow for overloaded constructors.
class Button {
Button(std::string text);
Button(Image image);
};
Note: The
Image object doesn't look overly heavy, so whether you accept it by value (copy constructed), by rvalue reference (move constructed), or by lvalue reference (borrowed) probably doesn't matter.
Another required bit of functionality for a button, is the click handler. If there's no click handler, there's also no point in having a button. This handler is unlikely to change after construction, so it's an ideal field to set in a constructor. I'm not entirely sure of the type you're using here, so I'm just going to pretend it's
ClickHandler. I would recommend updating the previous two constructors to the following:
class Button {
Button(std::string text, ClickHandler clickHandler);
Button(Image image, ClickHandler clickHandler);
};
Side note: I think the existing click handler setting code needs some slight refactoring to bring it inline with the
Law of Demeter. In particular, the button should take a pointer to the handler function directly, and assign that to whatever internal object will call the handler when a click event arises. I don't think it should give direct access to the complicated looking
Signal object from NAS2D that raises events. This
Signal object is directly exposed by the return value of the
click() method, leaving the caller to call
connect or
disconnect on it themselves. I think that
connect and
disconnect call should be wrapped into the button itself.
Finally, since it relates, I think purely presentational aspects, such as size and position should not be set in the constructor. Or at the very least, should not be required construtor parameters. There are many ways to set both size and position. They might be hardcoded constants, read from an XML file, calculated by some kind of layout manager, or maybe even uses some kind of CSS like language. They may need to be updated at runtime, particular if window resizing is supported (or will someday be supported). Finally, reasonable values might not be know at the time of control creation. You might need to know how many controls will be displayed before calculating the size and position to display them, and that may require constructing all the controls first. A dynamic display can't update until it knows how many components it has.
