• 1

• 3.4k
• 17
• 23

Cart's Design

Cart's interface

Right now, Cart is just a glorified sf::RectangleShape, as you mention. It's probably a good idea to modify its interface so that its methods have cart-ish behavior instead of rectangle-ish behavior. For example, instead of setPosition, you probably want moveForward and moveBackward; instead of getOrientation/setOrientation, you'll want something like getDirection/turnRight/turnLeft. Carts don't change colors for no reason; you may wish to remove setFillColor, or perhaps rename it to repaintCart.

const correctness

When a method is not intended to modify an object (e.g. it inspects the state of the object), you should mark it with const. This has two important benefits:

• When a method is const, the compiler prevents you from accidentally modifying the object's state in the method's implementation.

• When a method is const, you can call the method on a const Cart. This allows you to pass a const Cart& to functions that should be able to inspect a Cart's state but not to change it. It also allows you to call the method from other const methods: For example, with your current implementation you would be unable to call getPosition from within draw.

Design Patterns

It's not clear to me how you expect your classes will interact with one another. I can see that Cart has a similar interface right now to sf::RectangleShape; do you intend for your code to treat it like a drawn rectangle? Do you expect the rest of your code to use it with some interface in common with other components you create, or do you expect the rest of your code always to know that it's dealing with a Cart?

You specifically ask about the Decorator pattern. There are two ways that could be relevant to Cart. If you think there's some cart-y property that you could add to any Drawable, or more likely to any Shape or Transformable, then you could implement Cart as some decorator of that interface. For example, you could do this:

// T must be one of ShapeDecorator or sf::Shape. Our job here is complicated by the fact
// that Shape has an extensive nonvirtual interface that would be dangerous to override.
template <typename T>
class ShapeDecorator : public sf::Drawable {
 public:
 static_assert(std::is_base_of<sf::Shape, T>::value
 || std::is_base_of<ShapeDecorator, T>::value,
 "T must be one of sf::Shape or ShapeDecorator");
 ShapeDecorator(T* shape) : shape_(shape) { assert(shape_); }

I've chosen to use a raw pointer, with the understanding that the owner of this ShapeDecorator ensures that shape will outlive it. Another possible design would pass and store a std::shared_ptr<T> instead, but it is my opinion that shared_ptr can lead to muddled ownership relationships and tangled designs. See e.g. this programmers.SE question for more discussion).

 ~ShapeDecorator override = default;
 void draw(sf::RenderTarget& target, RenderStates states) const override {
 shape_->draw(target, states);
 }
 // Define the interface that you might want decorators to override as virtual
 virtual void setPosition(const sf::Vector2f& position) {
 shape_->setPosition(position);
 }
 // Define the interface ShapeDecorator will need to provide but that you don't
 // want decorators to override as non-virtual. 
 void setTexture(const Texture* texture, bool resetRect=false) {
 shape_->setTexture(texture, resetRect);
 }
 // etc.
 private:
 T* const shape_;
};
template <typename T>
class Cart : public ShapeDecorator<T> {
 public:
 Cart(T* shape) : ShapeDecorator(shape) {}
 // Whatever overrides you need to make this shape into a Cart.
 void setPosition(const sf::Vector2f& position) override {
 // Carts always seem to be below and to the right of where they actually are.
 ShapeDecorator::setPosition(position + sf::Vector2f(1.0, 2.0));
 }
};

Another way Decorator could be relevant to Cart is if you have some properties that you might add willy-nilly to your various components. For example, you may decide that you want to be able to make your components blink. You'd do this by defining a base Component class, a subclass of Component called Cart, another subclass of Component called ComponentDecorator, and then a specific BlinkComponentDecorator. Since we're defining our own object hierarchy, we can make sure that the only important interfaces that we have are virtual, thus avoiding that nasty template problem we had before:

class Component : public sf::Drawable {
 public:
 ~Component() override = default;
 // Whatever API you think is necessary.
};
class Cart final : public Component {
 public:
 void draw(sf::RenderTarget& target, RenderStates states) const override {
 // Whatever you do to draw a cart
 }
 // ...
};
class ComponentDecorator : public Component {
 public:
 ComponentDecorator(Component* component) : component_(component) { assert(component_); }
 void draw(sf::RenderTarget& target, RenderStates states) const override {
 component_->draw(target, states);
 }
 // Wrap the rest of Component's API
 private:
 Component* component_;
};
class BlinkComponentDecorator final : public ComponentDecorator {
 public:
 BlinkComponentDecorator(Component* component) : ComponentDecorator(component) {}
 void draw(sf::RenderTarget& target, RenderStates states) const override {
 if (std::time(nullptr) % 2) {
 ComponentDecorator::draw(target, states)
 } else {
 // Don't draw anything!
 }
 }
};