3
\$\begingroup\$

I am creating a GUI library to better learn C++ and wanted to see if there is anything to improve/simplify.

And I also wanted to be able to call an event of a parent and then for it to iterate all its children and those children to iterate their children and so forth, but I did not want to have a foreach loop in every subclass thus the IterateEvents loop. Is there a better way?

#pragma once

#include <functional>
#include <memory>
#include <vector>

#include <iostream>

namespace bui {
    class Element {
    public:
        int x = 0;
        int y = 0;
        int width = 0;
        int height = 0;
        Element* parent;
        std::function<void(void)> function;

        std::vector<Element*> children;

        Element(int x, int y, int width, int height, Element* parent) {
            this->x = x;
            this->y = y;
            this->width = width;
            this->height = height;

            if (parent != nullptr) {
                parent->children.push_back(this);
            }
        }

        ~Element() {

        }

        template <typename T>
        bool pointInElement(T x, T y) {
            return (x > this->x && y > this->y && x < this->x + this->width && this->y < this->y + this->height);
        }

        virtual void Event() = 0;
        virtual void Draw() = 0;


        void IterateEvents() {
            this->Event();
            for (auto &i : children) {
                i->Event();
            }
        }
    };

    extern std::vector<Element*> TopLevelWindows;

    class TopLevelWindow : public Element {
    public:
        TopLevelWindow(int x, int y, int width, int height, Element* parent) : Element(x, y, width, height, parent) {
            TopLevelWindows.push_back(this);
        }

        void Event() {
            std::cout << "TopLevelWindow\n";
        }

        void Draw() {
            //Draw stuff
        }

        ~TopLevelWindow() {

        }
    };

    class Window : public Element {
    public:
        Window(int x, int y, int width, int height, Element* parent) : Element(x, y, width, height, parent) {
        }

        void Event() {
            std::cout << "Window\n";
        }

        void Draw() {
            //Draw stuff
        }

        ~Window() {

        }
    };

    class Button : public Element {
    public:
        Button(int x, int y, int width, int height, Element* parent) : Element(x, y, width, height, parent) {
        }

        void Event() {
            std::cout << "Button\n";
        }

        void Draw() {
            //Draw stuff
        }

        ~Button() {

        }
    };
}
\$\endgroup\$
4
\$\begingroup\$

Point and Size structures

I would go even further than @glampert and create Point and Size structures as well as a Rectangle one. Experience has shown that this kind of small classes are extremely useful and can be used at many places in a GUI library (for example, getting the coodinates of the mouse can return a Point, computing the centre of a rectangle too, etc...). Moreover, once you have Point and Size classes, you can overload some of their operators to implement expressive and unambiguous operations such as multiplication by an integer.

In the long run, you will see that it is easier to reason with these objects, and you can even return them from functions while you can't return x and y if they're not in a structure.

Back to the Rectangle

Here is a concrete example of what your class does but should be actually performed by a Rectangle class instead:

template <typename T>
bool pointInElement(T x, T y) {
    return (x > this->x && y > this->y && x < this->x + this->width && this->y < this->y + this->height);
}

This should look like this, assuming that you have a Rectangle member named box and that your Rectangle class has a method named contains which takes a Point and returns a bool:

template<typename T>
bool pointInElement(const Point<T&>& point) const {
    return box.contains(point);
}

Constructor initialization list

Whenever possible, try to use the constructor initialization list. As it is now, your code constructs an Element instance, then assigns values to its fields. If you leave the job to the constructor initialization list, your object will be directly constructed with the appropriate values:

Element(int x, int y, int width, int height, Element* parent):
    x(x), y(y),
    width(width), height(height) {

    if (parent != nullptr) {
        parent->children.push_back(this);
    }
}

As you can see, it even allows to drop the this-> needed to differentiate the variables. The syntax only allows the member names on the left and the name resolution picks the parameters first on the right.

About the virtual destructor

I bet that you will want runtime polymorphism with your widgets. In this case, follow @glampert's advice: make the destructor virtual and public. If it does not do anything, the simplest thing to do is to explicitly default it:

virtual ~Element() = default;

Now, if a destructor is public and virtual in a base class, then the derived class automagically have a virtual destructor, you don't have to write anything. Therefore, the simplest thing to do would be to simply remove the destructors from TopLevelWindow, Window and Button if they don't do anything more than Element's destructor; you program will then be correct but simpler.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Well, using members instead of free functions is not a good idea, it increases coupling, as that function only accesses public members anyway. \$\endgroup\$ – Deduplicator Oct 16 '15 at 9:39
3
\$\begingroup\$

These members:

int x = 0;
int y = 0;
int width = 0;
int height = 0;

Are good candidates to be moved into an auxiliary Rect structure, then you can also make pointInElement() and all intersection tests members of it. This should make Element more sleek and focused on the UI-part of things.

struct Rect {
    int x = 0;
    int y = 0;
    int width  = 0;
    int height = 0;
    // methods for intersection testing, etc...
};

Also, a bit of personal preference here, but I find it easier to work with (xMin, yMin, xMax, yMax) than with (x, y, w, h) when it comes to handling 2D elements. Intersections and collision tests are a little simpler to code with the min/max bounds layout.


When you are creating a base class that will be inherited from (Element), you have two options regarding the destructor:

  1. Public virtual if pointers to your classes will be deleted via pointers to the base class (Element).

  2. Protected and non-virtual if you will never allow deleting a child class through a pointer to the base.

Choose one of the above and update your destructor declaration. Looking at your code is not really clear who owns the class instances, so I'll not make a guess.

The child classes currently have empty destructors, so you can just omit them and let the compiler supply the defaults.

By the way, you should consider smart pointers to better convey ownership. Sure you might not be able use a smart pointer in the internal hierarchy, but the end user who allocates UI Elements should store them by smart pointer. You also have to decide how to handle deleting a hierarchy. E.g.: If I delete a Window, are the Buttons deleted with it? This should be made very clear in your code.


Other nitpicks:

  • Don't this-> qualify member access. This is never done in C++ because it can hide undesirable shadowing of names. Plus, it makes code a lot more verbose, gratuitously.

  • Be sure to const-qualify methods that aren't changing member data. Read this for an explanation with example. For instance: It would be quite unexpected if Draw() mutated states inside the object. You don't expect things to change once you render them to screen. That would be a good candidate for const. Get*() methods are also the obvious candidates for this.

  • A little subjective, but the lines where you're declaring the constructors are a bit long. They don't fit in the code snippet window here, so other readers of your code might also experience that problem. Having to scroll horizontally is annoying and distracting, which can lead to overlooking problems in the code. Be sure to break lines that are too long so that they will fit in a reasonable screen size.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ As an aside, it's useful to keep special member-functions trivial as long as you can: Define them in the class itself as explicitly defaulted (= default;), unless you don't have to declare them at all. \$\endgroup\$ – Deduplicator Oct 16 '15 at 7:42

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.