Drag and Drop - Code Review

[Jycerian]

Hey guys, I have basically finished my first game named Gravity Surge (http://en.sfml-dev.org/forums/index.php?topic=15512.0) and now I am busy with my next game and learning more about programming.

My new game will be some sort of level editor/game combo and this is my first attempt for a drag and drop but I have no clue if my code is any good or not because I am not that advanced yet, I would really like to know what your opinions are!

Note : I intentionally made this one file just to learn.



Code :

(click to show/hide)

#include <SFML/Graphics.hpp>

#include <vector>
#include <iostream>

#include <stdio.h>              /* NULL */
#include <stdlib.h>             /* srand, rand */
#include <time.h>               /* time */

#define APP_WIDTH 512
#define APP_HEIGHT 320

int random(int min, int max);

sf::Vector2f randomPosition(sf::Vector2f& tileSize);
sf::Vector2f randomPosition(float tileWidth, float tileHeight);

template <typename T>
void addBlock(std::vector<T>& vector, sf::Vector2f& tileSize);

int main()
{
        sf::RenderWindow window(sf::VideoMode(APP_WIDTH, APP_HEIGHT), "Proto", sf::Style::Close);

        /* initialize random seed: */
        srand(time(NULL));

        sf::Vector2f tileSize(16, 16);
       
        std::vector<sf::RectangleShape> blocks;
        std::vector<sf::RectangleShape>::pointer selected = NULL;

        sf::Vector2i mouse = sf::Mouse::getPosition(window);
       
        bool isMousePressed = false;

        sf::Vector2f oldPosition;

        addBlock(blocks, tileSize);
        addBlock(blocks, tileSize);
        addBlock(blocks, tileSize);
        addBlock(blocks, tileSize);
        addBlock(blocks, tileSize);
        addBlock(blocks, tileSize);
        addBlock(blocks, tileSize);
        addBlock(blocks, tileSize);
        addBlock(blocks, tileSize);
        addBlock(blocks, tileSize);
        addBlock(blocks, tileSize);
        addBlock(blocks, tileSize);

        while (window.isOpen())
        {
                sf::Event event;
                while (window.pollEvent(event))
                {
                        switch (event.type)
                        {
                                case sf::Event::Closed:
                                        window.close();
                                        break;

                                case sf::Event::MouseButtonPressed:
                                        mouse = sf::Mouse::getPosition(window);

                                        // Check to see which block has been clicked
                                        for (std::vector<sf::RectangleShape>::iterator itr = blocks.begin(), end = blocks.end(); itr < end; ++itr)
                                        {
                                                if (mouse.x > itr->getPosition().x &&
                                                        mouse.y > itr->getPosition().y &&
                                                        mouse.x < itr->getPosition().x + itr->getGlobalBounds().width &&
                                                        mouse.y < itr->getPosition().y + itr->getGlobalBounds().height)
                                                {
                                                        isMousePressed = true;
                                                        selected = &(*itr);

                                                        selected->setOutlineThickness(-2);

                                                        // Save old position
                                                        oldPosition = selected->getPosition();

                                                        /* Swap selected block with last position in vector
                                                         * so that it get's renderd last and overlaps all
                                                         * other blocks
                                                         */
                                                        std::iter_swap(blocks.begin() + blocks.size() - 1, selected);
                                                }
                                        }
                                        break;

                                case sf::Event::MouseButtonReleased:
                                {
                                        isMousePressed = false;
                                        if (selected != NULL)
                                        {
                                                sf::Vector2f newPosition;
                                                newPosition.x = std::floor((selected->getPosition().x + tileSize.x / 2) / tileSize.x) * tileSize.x;
                                                newPosition.y = std::floor((selected->getPosition().y + tileSize.y / 2) / tileSize.y) * tileSize.y;

                                                selected->setOutlineThickness(-1);

                                                // Is block on top of another block?
                                                bool occupied = false;
                                                for (std::vector<sf::RectangleShape>::iterator itr = blocks.begin(), end = blocks.end(); itr < end; ++itr)
                                                {
                                                        if (newPosition == itr->getPosition())
                                                        {
                                                                occupied = true;
                                                                break;
                                                        }
                                                }

                                                if (!occupied)
                                                        selected->setPosition(newPosition);
                                                else
                                                        selected->setPosition(oldPosition);
                                        }
                                        selected = NULL;
                                        break;
                                }

                                default:
                                        break;
                        };
                }

                if (isMousePressed)
                {
                        mouse = sf::Mouse::getPosition(window);
                        selected->setPosition(mouse.x - tileSize.x / 2, mouse.y - tileSize.y / 2);
                }

                window.clear();
                for (std::vector<sf::RectangleShape>::iterator itr = blocks.begin(), end = blocks.end(); itr < end; ++itr)
                {
                        window.draw(*itr);
                }
                window.display();
        }
        return 0;
}

int random(int min, int max)
{
        int random = min + (rand() % max - min);
        return random;
}

sf::Vector2f randomPosition(sf::Vector2f& tileSize)
{
        int randX = random(0, APP_WIDTH);
        int randY = random(0, APP_HEIGHT);
        randX = std::floor(randX / tileSize.x);
        randY = std::floor(randY / tileSize.y);
        return sf::Vector2f(randX * tileSize.x, randY * tileSize.y);
}

sf::Vector2f randomPosition(float tileWidth, float tileHeight)
{
        int randX = random(0, 512);
        int randY = random(0, 320);
        randX = std::floor(randX / tileWidth);
        randY = std::floor(randY / tileHeight);
        return sf::Vector2f(randX * tileWidth, randY * tileHeight);
}

template <typename T>
void addBlock(std::vector<T>& vector, sf::Vector2f& tileSize)
{
        sf::RectangleShape block;
        block.setSize(tileSize);
        block.setFillColor(sf::Color::Red);
        block.setOutlineColor(sf::Color::White);
        block.setOutlineThickness(-1);
        block.setPosition(randomPosition(tileSize));
        vector.push_back(block);
}

[dabbertorres]

First thing I noticed is that you're using some C-style headers and some C-style code.

For the headers:

#include <cstdio>
#include <cstdlib>
#include <ctime>
 

is the more C++ way of doing it.

Second, in modern C++, you don't use NULL, srand, rand, and time.
You use nullptr, <random>, and <chrono> (respectively).
Just google those two headers for guides to using them.

Those are the first things I noticed in my quick look over.

[Xornand]

If I could suggest something, calling draw() every time you render a rectangle isn't the most efficient way to do it - that approach just doesn't scale well and could be a major bottleneck if you were drawing hundreds of rectangles every frame.

The approach I would suggest instead, would be to externally create and then load the texture of the rectangle as sf::Texture - or, you could create sf::RenderTexture and draw sf::RectangleShape (filled with the color of your choice) to it only once, upon initialization. The next step would be to create a custom Rectangle class, which would store sf::VertexArray, representing four corners. All the Rectangle instances would be then stored in some sort of container, so on each update you would be iterating over all the rectangles and copying their vertices onto one sf::VertexArray. After copying all the vertices, you could then just make one draw call, passing the sf::VertexArray and sf::RenderStates (with the texture parameter set to the texture of your rectangles).

The code would look something like that:

struct Rectangle
{
        sf::VertexArray vertexArray;
};

void render()
{
        sf::VertexArray vertexArray(sf::Quads);

    // let's assume that Rectangle instances are stored in std::vector<Rectangle>
        std::for_each(rectangles.begin(), rectangles.end(),
                [&](Rectangle& rectangle)
                {
                        for (auto i = 0; i < 4; ++i)
                        {
                                vertexArray.append(rectangle.vertexArray[i]);
                        }
                }
        );

        sf::RenderStates states;
        states.texture = texture.get(); // "texture" is std::shared_ptr<sf::Texture> stored somewhere else

    window.clear();
        window.draw(vertexArray, states); // only one draw call for all the rectangles
        window.display();
}

[eXpl0it3r]

As dabbertorres said:

• Use C++ headers
• Use the <random> header (C++11) for random numbers.
• Use the <chrono> header (C++11) for time related stuff.

Furter more:

• Don't use #define for constants, instead use an actual constant const unsigned int WIDTH = 10
• Use a loop for adding blocks, instead of c&p the same line n times.
• Use std::swap instead of std::iter_swap (see here).
• If you use a class, you could write functions for the event handling, making the event handling block a lot easier to read.
• If you just iterate over all the elements, you could use the range for loop (C++11).
• You'd probably be better of by having a class for the blocks, so you could add properties etc. to it.