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