HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Cart-creation class using SFML

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
sfmlcartusingclasscreation

Problem

My code is for a simple "cart-creation" class and sample use in SFML. The Cart class inherits sf::RectangleShape and will end up having more member functions (isFull, numberOfWheels, etc.) There will also be a conveyor belt class in the transvac folder. Currently, though, the Cart class is really just a special sort of sf::RectangleShape.

I have had very little experience with design patterns and I would like to know how I can use them in general to make the Cart class more effective. I think the Cart class may be a Decorator, but I'm not sure.

Also, the other important things that I would like to look at are: use of header files, OOP, source tree layout, and general best practices.

Compile with:

g++ -I ./include -Wall -Wextra -Werror -std=c++11 -c ./include/transvac/cart.cpp -o ./obj/cart.o
g++ -I ./include ./src/main.cpp -Wall -Wextra -Werror -std=c++11 -lsfml-graphics -lsfml-window -lsfml-system ./obj/cart.o -o ./bin/cart-mover


Directory tree:

/cart-mover
/bin
cart-mover
/include
/transvac
cart.hpp
cart.cpp
/obj
cart.o
/src
main.cpp


cart.hpp:

`// cart.hpp

#include

#ifndef __CART_HPP_INCLUDED__
#define __CART_HPP_INCLUDED__

enum Orientation {VERTICAL, HORIZONTAL};

// TransVac namespace
namespace tvc {

class Cart : public sf::Drawable {
// The Cart class is basically a glorified sf::RectangleShape.

public:
// Constructor- create with position, color, and orientation.
Cart(const sf::Vector2f& cart_pos_, sf::Color cart_color_ = sf::Color::Black, Orientation cart_or_ = VERTICAL);

// The Position and FillColor setters and getters are just wrappers for sf::RectangleShape member methods.
void setPosition(const sf::Vector2f& cart_pos_);
const sf::Vector2f& getPosition();

void setFillColor(sf::Color cart_color_);
sf::Color getFillColor();

// Not rel

Solution

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 
class ShapeDecorator : public sf::Drawable {
  public:
    static_assert(std::is_base_of::value
                  || std::is_base_of::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 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 
class Cart : public ShapeDecorator {
  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) { ass

Code Snippets

// 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_); }
~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));
    }
};
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!
        }
    }
};

Context

StackExchange Code Review Q#60053, answer score: 3

Revisions (0)

No revisions yet.