2
\$\begingroup\$

I am currently working on a bigger project where I found a part of it which seems very ugly to me from the code.

I stripped out the algorithm and made a working code, so it can be reviewed.

I have a data structure which contains several containers with several different objects:

struct Data {
    std::vector<TypeA> a;
    std::vector<TypeB> b;
    std::vector<TypeC> c;
}

TypeA, TypeB and TypeC are all different objects with different sizes in the real programm. The comman part is they all have a no:

struct TypeA {
    std::size_t no;
    std::string data;
    std::string data2;
    // more data member etc...
};

struct TypeB {
    std::size_t no;
    std::string data;
    std::string data2;
    // more data member etc...
};

struct TypeC {
    std::size_t no;
    std::string data;
    std::string data2;
    // more data member etc...
};

All Containers are ordered by ascending no:

Data d;

d.a.push_back(TypeA{ 1, "1" , "11" });
d.c.push_back(TypeC{ 2, "2", "12" });
d.b.push_back(TypeB{ 3, "3", "13" });
d.a.push_back(TypeA{ 4, "4", "14" });
d.c.push_back(TypeC{ 5, "5", "15" });
d.b.push_back(TypeB{ 6, "6", "16" });

All Numbers are unique, e.g. there's no no==1 in b or c. The index numbers are consecutive and fill the data structures.

I want to iterate over the whole struct Data by the no of the different data members and do something with the additional shared things the containers have.

To emphasize the ugliness, I sometimes print data1 and the other time data2 but the iteration is exactly the same.

How can the algorithm be improved?

My idea would be writing an custom iterator for Data which iterates by the no. But I don't know what is the best approach.

I'm open to any suggestions.

The complete code:

example.cpp

#include <vector>
#include <limits>
#include <iostream>
#include <string>

struct TypeA {
    std::size_t no;
    std::string data;
    std::string data2;
    // more data member etc...
};

struct TypeB {
    std::size_t no;
    std::string data;
    std::string data2;
    // more data member etc...
};

struct TypeC {
    std::size_t no;
    std::string data;
    std::string data2;
    // more data member etc...
};

// In reality TypeA, TypeB and TypeC are not exactly the same

struct Data {
    std::vector<TypeA> a;
    std::vector<TypeB> b;
    std::vector<TypeC> c;

    // In reality there are not only 3 values 
    // but > 20 to print consecutively by no

    void print_data_in_order_of_no();
    void print_data2_in_order_of_no();
};

void Data::print_data_in_order_of_no()
{
    enum class It_out {
        none,
        a,
        b,
        c,
    };

    auto it_a = a.begin();
    auto it_b = b.begin();
    auto it_c = c.begin();

    std::size_t current_no = 0;
    auto it_out = It_out::none;

    do {
        auto possible_new_no = std::numeric_limits<size_t>::max();
        it_out = It_out::none;

        if (it_a != a.end() &&
            it_a->no > current_no &&
            it_a->no < possible_new_no) {

            possible_new_no = it_a->no;
            it_out = It_out::a;
        }
        if (it_b != b.end() &&
            it_b->no > current_no &&
            it_b->no < possible_new_no) {

            possible_new_no = it_b->no;
            it_out = It_out::b;
        }
        if (it_c != c.end() &&
            it_c->no > current_no &&
            it_c->no < possible_new_no) {

            possible_new_no = it_c->no;
            it_out = It_out::c;
        }

        current_no = possible_new_no;

        switch (it_out) {
        case It_out::a:
            std::cout << it_a->data << '\n';
            ++it_a;
            break;
        case It_out::b:
            std::cout << it_b->data << '\n';
            ++it_b;
            break;
        case It_out::c:
            std::cout << it_c->data << '\n';
            ++it_c;
            break;
        case It_out::none:
            break;
        default:
            throw std::runtime_error(
                "void print_data_in_order_of_no()"
                "impossible enum type"
            );
        }
    } while (it_out != It_out::none);
}

void Data::print_data2_in_order_of_no()
{
    enum class It_out {
        none,
        a,
        b,
        c,
    };

    auto it_a = a.begin();
    auto it_b = b.begin();
    auto it_c = c.begin();

    std::size_t current_no = 0;
    auto it_out = It_out::none;

    do {
        auto possible_new_no = std::numeric_limits<size_t>::max();
        it_out = It_out::none;

        if (it_a != a.end() &&
            it_a->no > current_no &&
            it_a->no < possible_new_no) {

            possible_new_no = it_a->no;
            it_out = It_out::a;
        }
        if (it_b != b.end() &&
            it_b->no > current_no &&
            it_b->no < possible_new_no) {

            possible_new_no = it_b->no;
            it_out = It_out::b;
        }
        if (it_c != c.end() &&
            it_c->no > current_no &&
            it_c->no < possible_new_no) {

            possible_new_no = it_c->no;
            it_out = It_out::c;
        }

        current_no = possible_new_no;

        switch (it_out) {
        case It_out::a:
            std::cout << it_a->data2 <<'\n';
            ++it_a;
            break;
        case It_out::b:
            std::cout << it_b->data2 << '\n';
            ++it_b;
            break;
        case It_out::c:
            std::cout << it_c->data2 << '\n';
            ++it_c;
            break;
        case It_out::none:
            break;
        default:
            throw std::runtime_error(
                "void print_data2_in_order_of_no()"
                "impossible enum type"
            );
        }
    } while (it_out != It_out::none);
}

int main()
{
    Data d;

    d.a.push_back(TypeA{1, "1" , "11"});
    d.a.push_back(TypeA{ 4, "4", "14" });

    d.b.push_back(TypeB{ 3, "3", "13" });
    d.b.push_back(TypeB{ 6, "6", "16" });

    d.c.push_back(TypeC{ 2, "2", "12" });
    d.c.push_back(TypeC{ 5, "5", "15" });


    d.print_data_in_order_of_no();

    d.print_data2_in_order_of_no();

    std::cin.get();
}

The code for review is the Data struct and its members.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Is no used for something else or just for indexing/identification? If the latter, move no out of TypeX and join TypeA, TypeB, TypeC into a separate structure and use this structure in the container. \$\endgroup\$
    – Cornholio
    Commented Jan 31, 2019 at 16:05
  • \$\begingroup\$ its only used for indexing. So you mean like a struct which have TypeA TypeB TypeC were only one member is occupied at the time? \$\endgroup\$
    – Sandro4912
    Commented Jan 31, 2019 at 16:28
  • 2
    \$\begingroup\$ I think Cornholio meant something like a std::vector<std::variant<TypeA, TypeB, TypeC>> (where those three types have no no member since they are all explicitly ordered inside the container). \$\endgroup\$
    – hoffmale
    Commented Jan 31, 2019 at 16:54
  • \$\begingroup\$ I don't think he wants a std::variant here, since he needs all of the TypeA, TypeB, TypeC containers to be valid at the same time. \$\endgroup\$
    – serkan.tuerker
    Commented Jan 31, 2019 at 19:38
  • \$\begingroup\$ std::variant was exactly what i was looking for the whole day. In combination with std::visit i can perform any operation necessary. \$\endgroup\$
    – Sandro4912
    Commented Jan 31, 2019 at 20:25

1 Answer 1

Reset to default
2
\$\begingroup\$

As mentioned in a comment, I found std::variant to be a good solution for my issue:

I also added a template visitor it can save writing a lot of visitors if in all data structures the same thing needs to get accessed.

#include <vector>
#include <limits>
#include <iostream>
#include <string>
#include <variant>

struct TypeA {
    std::size_t no;
    std::string data;
    std::string data2;
    // more data member etc...
};

struct TypeB {
    std::size_t no;
    std::string data;
    std::string data2;
    // more data member etc...
};

struct TypeC {
    std::size_t no;
    std::string data;
    std::string data2;
    // more data member etc...
};

// In reality TypeA, TypeB and TypeC are not exactly the same

struct Data {
    std::vector<std::variant< TypeA, TypeB, TypeC>> abc;

    std::vector<TypeA> a;
    std::vector<TypeB> b;
    std::vector<TypeC> c;

    // In reality there are not only 3 values 
    // but > 20 to print consecutively by no
};

struct Visit_data
{
    std::string operator()(TypeA& a) const { return a.data; } 
    std::string operator()(TypeB& b) const { return b.data; }
    std::string operator()(TypeC& c) const { return c.data; }
};

struct Visit_data2
{
    std::string operator()(TypeA& a) const { return a.data2; }
    std::string operator()(TypeB& b) const { return b.data2; }
    std::string operator()(TypeC& c) const { return c.data2; }
};

struct Visit_template_data
{
    template<typename T>
    std::string operator()(T& a) const { return a.data; }
};

int main()
{
    Data d;

    d.abc.push_back(TypeA{ 1, "1" , "11"});
    d.abc.push_back(TypeC{ 2, "2", "12" });
    d.abc.push_back(TypeB{ 3, "3", "13" });
    d.abc.push_back(TypeA{ 4, "4", "14" });
    d.abc.push_back(TypeC{ 5, "5", "15" });
    d.abc.push_back(TypeB{ 6, "6", "16" });

    for (auto& x : d.abc) {
        std::cout << std::visit(Visit_data(), x);
    }
    for (auto& x : d.abc) {
        std::cout << std::visit(Visit_data2(), x);
    }

    for (auto& x : d.abc) {
        std::cout << std::visit(Visit_template_data(), x);
    }

    std::cin.get();
}
\$\endgroup\$
2
  • \$\begingroup\$ I was going to suggest using the Visitor pattern as well as Variant, but you totally beat me to it! See also the std::visit example on CPP Reference for a way to create a an overloaded visitor, which might help you. \$\endgroup\$
    – Toby Speight
    Commented Feb 1, 2019 at 8:37
  • \$\begingroup\$ You have no idea how I patted my head when i realized I could refactor many poorly and overly complicated parts in the code by switching to std::variant. \$\endgroup\$
    – Sandro4912
    Commented Feb 1, 2019 at 9:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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