3
\$\begingroup\$

I decided to write a rain effect to learn how to use SFML, and I would like to get some review on my code: i want to know how you would have write the program, and how to improve my code. This program accepts one parameter to change the wind direction.

main.cpp

#include <iostream>
#include "Graphics.hpp"

void help() noexcept
{
    std::cerr << "Param:" << std::endl;
    std::cerr << "Wind Direction:       none:0, left:1, right:2" << std::endl;
}

bool isNumber(const std::string& s) noexcept
{
    return !s.empty() && std::find_if(s.begin(), s.end(), [](char c) { return
        !std::isdigit(c); }) == s.end();
}

int main(int argc, char * argv[]) noexcept
{
    if(argc != 2)
    {
        help();
        return -1;
    }

    std::string arg1 = argv[1];
    if(!isNumber(arg1) || arg1.size() > 1 || (std::stoi(arg1) < 0 ||
        std::stoi(arg1) > 2))
    {
        std::cerr << "The parameter must be a number between 0 and 2" <<
            std::endl;
        return -1;
    }
    Graphics graphics(std::stoi(arg1));
    graphics.run();

    return 0;
}

WaterDroplet.hpp

#ifndef DEF_WATER_DROPLET_HPP
#define DEF_WATER_DROPLET_HPP

#include <iostream>
#include <random>
#include <chrono>
#include <SFML/System.hpp>
#include "Config.hpp"

//min size: Vector2f(15, 1), max size: sf::Vector2f(35, 3)
//rotation: depends of the wind direction, no: 90, left: 135, right: 45
class WaterDroplet
{
public:
    WaterDroplet(unsigned short windDirection);

    ~WaterDroplet();

    unsigned short getRotation() const;

    sf::Vector2f getSize() const;

    sf::Vector2f getPosition() const;

private:
    unsigned short rotation_;
    sf::Vector2f size_;
    sf::Vector2f position_;
};

#endif //DEF_WATER_DROPLET_HPP

WaterDroplet.cpp

#include "WaterDroplet.hpp"

WaterDroplet::WaterDroplet(unsigned short windDirection)
{
    unsigned int seed = std::chrono::system_clock::now().time_since_epoch()
        .count();
    std::default_random_engine eng(seed);
    if(windDirection == static_cast<short>(Config::WIND_DIRECTION_NO))
    {
        std::uniform_int_distribution<int> distPositionX(10,
            static_cast<int>(Config::WINDOW_X));
        position_.y = 0;
        position_.x = distPositionX(eng);
        rotation_ = 90;
    }
    else if(windDirection == static_cast<short>
        (Config::WIND_DIRECTION_LEFT))
    {
        std::uniform_int_distribution<int> distPositionX(0,
            static_cast<int>(Config::WINDOW_X));
        std::uniform_int_distribution<int> distPositionY(0,
            static_cast<int>(Config::WINDOW_Y));
        position_.x = distPositionX(eng);
        position_.y = distPositionY(eng);
        rotation_ = 135;
    }
    else
    {
        std::uniform_int_distribution<int> distPositionX(0,
            static_cast<int>(Config::WINDOW_X));
        std::uniform_int_distribution<int> distPositionY(0,
            static_cast<int>(Config::WINDOW_Y));
        position_.x = distPositionX(eng);
        position_.y = distPositionY(eng);
        rotation_ = 45;
    }

    std::uniform_int_distribution<short> distSizeX(15, 35);
    std::uniform_int_distribution<unsigned short> distSizeY(1, 3);
    size_.x = distSizeX(eng);
    size_.y = distSizeY(eng);
}

WaterDroplet::~WaterDroplet()
{

}

unsigned short WaterDroplet::getRotation() const
{
    return rotation_;
}

sf::Vector2f WaterDroplet::getSize() const
{
    return size_;
}

sf::Vector2f WaterDroplet::getPosition() const
{
    return position_;
}

Graphics.hpp

#ifndef DEF_GRAPHICS_HPP
#define DEF_GRAPHICS_HPP

#include <SFML/Graphics.hpp>
#include "WaterDroplet.hpp"
#include "Config.hpp"

class Graphics
{
public:
    Graphics(unsigned short windDirection);

    ~Graphics();

    void run() noexcept;

private:
    void processEvents() noexcept;

    void update() noexcept;

    void render() noexcept;

    float velocity_ { 0.5 };
    unsigned short windDirection_;
    sf::RenderWindow window_;
    std::vector<std::unique_ptr<sf::RectangleShape>> rectangleShapes_;
    sf::Clock clock_;
    sf::Clock movementWaterDropletClock_;
};

#endif //DEF_GRAPHICS_HPP

Graphics.cpp

#include "Graphics.hpp"

Graphics::Graphics(unsigned short windDirection) : windDirection_(windDirection)
    , window_(sf::VideoMode(static_cast<int>(Config::WINDOW_X), static_cast
        <int>(Config::WINDOW_Y)), "Rain")
{

}

Graphics::~Graphics()
{

}

void Graphics::run() noexcept
{
    while(window_.isOpen())
    {
        processEvents();
        update();
        render();
    }
}

void Graphics::processEvents() noexcept
{
    sf::Event event;
    while(window_.pollEvent(event))
    {
        switch(event.type)
        {
            case sf::Event::Closed:
                window_.close();
                break;
            default:
                break;
        }
    }
}

void Graphics::update() noexcept
{
    if(clock_.getElapsedTime().asMilliseconds() > 10)
    {
        WaterDroplet waterDroplet(windDirection_);
        std::unique_ptr<sf::RectangleShape> rectangleShape =
            std::make_unique<sf::RectangleShape>
            (waterDroplet.getSize());
        rectangleShape->setPosition(waterDroplet.getPosition());
        rectangleShape->setRotation(waterDroplet.getRotation());
        rectangleShape->setFillColor(sf::Color(0, 65, 128));
        rectangleShapes_.push_back(std::move(rectangleShape));
        clock_.restart();
    }

    for(size_t i = 0; i < rectangleShapes_.size(); ++i)
    {
        movementWaterDropletClock_.restart();
        sf::Time time = movementWaterDropletClock_.getElapsedTime();
        float elapsedTime = time.asSeconds();

        if(windDirection_ == static_cast<unsigned short>
            (Config::WIND_DIRECTION_NO))
                rectangleShapes_[i]->move(0, velocity_ + elapsedTime);
        else if(windDirection_ == static_cast<unsigned short>
            (Config::WIND_DIRECTION_LEFT))
            rectangleShapes_[i]->move(-velocity_ - elapsedTime,
                velocity_ + elapsedTime);
        else
            rectangleShapes_[i]->move(velocity_ + elapsedTime, velocity_
                + elapsedTime);

        if(rectangleShapes_[i]->getPosition().y > static_cast<int>
            (Config::WINDOW_Y) + 10)
            rectangleShapes_.erase(rectangleShapes_.begin() + i);
    }
}

void Graphics::render() noexcept
{
    window_.clear();
    for(const auto& rectangleShape: rectangleShapes_)
        window_.draw(*rectangleShape);
    window_.display();
}

Config.hpp

#ifndef DEF_CONFIG_HPP
#define DEF_CONFIG_HPP

enum class Config
{
    WINDOW_X                    = 800,
    WINDOW_Y                    = 800,
    WIND_DIRECTION_NO           = 0,
    WIND_DIRECTION_LEFT         = 1,
    WIND_DIRECTION_RIGHT        = 2
};

#endif //DEF_CONFIG_HPP
\$\endgroup\$
3
\$\begingroup\$

This is really straightforward and easy to understand. Nice work! I have a few suggestions.

Use The Right Type

It's a little odd that you used an enum for the Config type. Usually an enumeration is a set of options that are all related. But WINDOW_X and WINDOW_Y are completely unrelated to WIND_DIRECTION. I would suggest that WINDOW_X and WINDOW_Y should be named constants. I would do this:

const sf::Vector2i kWindowSize(800, 800);
enum class WindDirection
{
    WINDOW_DIRECTION_NO    = 0,
    WINDOW_DIRECTION_LEFT  = 1,
    WINDOW_DIRECTION_RIGHT = 2
};

If you want to put it in a namespace, that would be reasonable, too. Although, looking at the numerous static_cast<int>s you have, it seems like it would be easier to just make them named constants with the correct type, as well. (And I note that sometimes you cast them to int, sometimes short, and sometimes unsigned short. That indicates that something isn't the right type.)

Naming

For the most part your naming is really good. I would change Graphics to Renderer, as Graphics is a very generic term. These days it pretty much means anything on the screen. The name Renderer tells you what the object actually does.

Simplify

I think the code in the for loop in Graphics::update() could be made simpler. I would call rectangleShapes_[i]->move on it only once. Something like this:

for(size_t i = 0; i < rectangleShapes_.size(); ++i)
{
    movementWaterDropletClock_.restart();
    sf::Time time = movementWaterDropletClock_.getElapsedTime();
    float elapsedTime = time.asSeconds();

    float xOffset = 0;
    float yOffset = velocity_ + elapsedTime;
    if (windDirection_ == Config::WIND_DIRECTION_LEFT)
    {
        xOffset = -velocity_ - elapsedTime;
    }
    else if (windowDirection_ == Config::WIND_DIRECTION_RIGHT)
    {
        xOffset = velocity_ + elapsedTime;
    }
    rectangleShapes_[i]->move(xOffset, yOffset);

    if(rectangleShapes_[i]->getPosition().y > Config::kWindowSize.y + 10)
    {
        rectangleShapes_.erase(rectangleShapes_.begin() + i);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your answer! Yes, I could have cast the window dimensions to unsigned int, and the wind directions to unsigned short, but I didn't know that I could to make an enum with a well-defined type. \$\endgroup\$ – Julien Oct 4 '17 at 6:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.