Suggestions to improve code in WidgetBase.h / .cpp
Robert Hairgrove
code at roberthairgrove.com
Fri Dec 31 10:51:16 GMT 2021
Not all of these are bugs, and the suggestions and questions are
certainly open to discussion.
In widgetbase.h/.cpp:
========================================
Class QGraphicsObjectWrapper inherits QGraphicsObject, which inherits
QObject,
but there is no Q_OBJECT macro declared.
Since there are no signals or slots declared, it might not be necessary.
But is this allowed by Qt? Does it perhaps lead to undefined behavior?
========================================
In class WidgetBase:
These "getter" functions should all be const:
bool WidgetBase::autoResize();
bool changesShape();
bool WidgetBase::hasDocumentation();
bool autoResize();
bool changesShape();
========================================
All of these should probably have const overloads (similar to
UMLObject):
ActivityWidget* asActivityWidget();
ActorWidget* asActorWidget();
ArtifactWidget* asArtifactWidget();
AssociationWidget* asAssociationWidget();
etc.
========================================
In the implementation of
WidgetBase& operator=(const WidgetBase& other)...
1. Does it matter that the Qt base classes (QObject, etc.) of a
WidgetBase cannot be copied? What about issues of parent/child
hierarchies?
2. The implementation should guard against self-assignment.
3. The pointer member m_umlObject is copied (shallow copy). Can two
different widgets
really have the same UMLObject on the same diagram? Of course, if
these are on different
diagrams, the same UMLObject can have different representations by
different widgets.
4. If assignment is allowed, there should be a copy constructor, or why
not...?
========================================
In the implementation of boundingRect():
/**
* @return The bounding rectangle for this widget.
* @see setRect
*/
QRectF WidgetBase::boundingRect() const
{
int halfWidth = lineWidth()/2;
return m_rect.adjusted(-halfWidth, -halfWidth, halfWidth,
halfWidth);
}
Suggested improvement, since we are using floating-point rects:
QRectF WidgetBase::boundingRect() const
{
// int halfWidth = lineWidth()/2;
qreal halfWidth = lineWidth()/2.0;
return m_rect.adjusted(-halfWidth, -halfWidth, halfWidth,
halfWidth);
}
========================================
Protected attributes:
All (or most) of the member variables have getter and setter functions.
IMHO they should be private, at least where there are getters and
setters.
More information about the umbrello-devel
mailing list