3
\$\begingroup\$

I'm trying to create a node editor "framework" (IDK what to call it, but essentially there is no GUI and should be implemented using these classes), similar to rete.js, blueprints(maybe an exception since there are events involved), imnodes, etc; and am creating a pin class/struct.

A pin, in this case, is the small things you actually connect (those circles on the sides of the actual nodes which can be inputs or outputs).

I'm using a single header, since almost everything I look into recommends using it for templated classes.

I think there are many problems in the code in general, but another thing that has been bugging me is the naming of classes, enums, members, etc., especially the naming of PinType, InputOutputCount (including the members itself), IOCount.

#pragma once

#include <string>
#include <vector>

enum PinType {//Whether or not the pin takes in values from other pins(input) or gives the value to other pins(output). TODO: Name change.
    INPUT,
    OUTPUT
};

enum InputOutputCount {//How many stuff the pin can take in or give out. TODO: Name change.
    NOTHING = 0,//Is this redundant?
    ONE = 1,
    MULTIPLE = -1
};

struct PinAbstract {//An abstract superclass for the actual Pin class, in order to store it in vectors, since Pin is a templated class.
    PinType Type;
    InputOutputCount IOCount = ONE;
    virtual ~PinAbstract() = 0;//Don't know much about destructors, but this was the only thing that worked for me.
};

template <class T>
struct Pin : public PinAbstract {//The actual pin class
    std::string Title = "Title";//The title of the pin
    T* Value = nullptr;//The value passed or being given.
    std::vector<Pin<T>*> Connections = {};//The other pins this pin is connected to. NOTE: same template type since two connected pins should be of same type. So a string input only takes in from a string output.
};

template <class T>
void ConnectPins(Pin<T> *pin1, Pin<T> *pin2) {//NOTE: same template type for the same reason above.
    if (((pin1->Type == OUTPUT && pin2->Type == INPUT) || (pin1->Type == INPUT && pin2->Type == OUTPUT)) && ((int)pin1->IOCount - pin1->Connections.size() != 0 && (int)pin2->IOCount - pin2->Connections.size() != 0)) {
/*Only connect input with output and vice-versa AND make sure that connection doesn't exceed max connections of any one of the pins*/
        pin1->Connections.push_back(pin2);
        pin2->Connections.push_back(pin1);
    }
}

Any sort of help is greatly appreciated. Including things that are not even code, including naming fixes, general strategies, etc.

\$\endgroup\$

2 Answers 2

Reset to default
2
\$\begingroup\$

Input and output count

enum InputOutputCount {
    NOTHING = 0, //Is this redundant?
    ONE = 1,
    MULTIPLE = -1
};

Yes, I think a pin that does not allow anything to connect to it is redundant. This could be replaced with a simple bool that indicates whether multiple connections are allowed or not.

Note that it doesn't make sense to restrict the number of connections to an output pin, there is nothing ambiguous about that. However, for input pins there is ambiguity: what should you do if there are multiple, different values on the input? Do you pick one, sum them, average them, multiply them? So there it does make sense to restrict the input to just one connection.

Make Pin::Value a reference

Does it make sense for Value to be a null pointer? After setting the pointer, can the pointer change? If the answer to both of these questions is no, then it is better to make it a reference, like so:

template <class T>
struct Pin: PinAbstract {
    ...
    T &value;
    ...
};

You should add a constructor that initializes the reference, so that it is not possible to construct a concrete Pin without a correct reference. You can also use this to ensure the title is always set to something useful, like so:

template <class T>
struct Pin: PinAbstract {
    const std::string Title;
    T &value;
    std::vector<Pin> Connections;

    Pin(T &value, const std::string &title): Value(value), Title(title) {}
};

Return an error if a connection cannot be made

Your function ConnectPins() silently ignores errors when trying to connect two incompatible pins together. Consider adding some way of reporting an error. It can be as simple as returning a bool indicating success, or you could throw an exception. This could then be used by the UI to display an error message.

Pass pins by reference to ConnectPins()

Similar to why you might want to use a reference instead of a pointer for Pin::Value, since you should never pass a NULL pointer to ConnectPins(), it is better to pass them by reference:

template <class T>
bool ConnectPins(Pin<T> &pin1, Pin<T> &pin2) {
    if (pin1.Type != pin2.Type)
        return false;
    ...

    pin1.Connections.push_back(&pin2);
    pin2.Connections.push_back(&pin1);
    return true;
}
\$\endgroup\$
1
\$\begingroup\$

One thing that you should avoid, if possible, is the use of raw pointers, from my point of view you should convert them on shared pointers by default and once your software evolves check if you need a more specific type, such as a unique pointer. On the other hand, if you can not avoid the use of a raw pointer is fine if you have tests that cover that, but as a good practise I would recommend to use shared/unique pointers by default to avoid memory issues and try to stay away of raw pointers as much as you can.

\$\endgroup\$
2
  • \$\begingroup\$ Really sorry for the late reply, I asked this question at late night, with the expectation to receive answers much later. But thanks a lot for the answer. I don't know much about smart pointers, shared pointers, etc., but I will try to learn about it and how to use it, especially since it's considered safer than raw pointers. I will keep the post open for now, for any more possible answers. Thank you nonetheless! \$\endgroup\$ Commented Jun 3, 2020 at 16:33
  • \$\begingroup\$ Also, another question, what exactly needs to be a shared pointer? All of the pointers in the code, just the value pointer, or the Pin<T>*? \$\endgroup\$ Commented Jun 3, 2020 at 18:52

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.