2
\$\begingroup\$

I'm still relatively new to C++ and I've written a program that could do with being a bit shorter and faster.

I use this same set of for loops many times to cycle through the whole tree of values. I'll use the JSON format to show you a much shortened version of the tree (in reality the tree has another layer of depth with many more values):

{
  "WorldOne": {
    "Earth": {
      "Japan": "Tokyo",
      "America": "New York"
    },
    "Moon": {
      "Dark Side": "Crater One"
    }
  }
}

I then parse this and fill a large vector with these values. The format of the vectors can be seen in the loop below.

My program then, depending on the parameters I set, cycles through to grab the data contained within.

This is the loop I am using over and over again:

for ( auto cWorld : fWorldVec ) //vector of world pointers
        {
            for ( auto cPlanet : ( cWorld )->fPlanetVector ) 
            {
                for ( auto cCity : cPlanet.fCityVector )
                {
                     for (auto cValue : cCity.fValueVector) 
                     {
                          // Then I do stuff
                          // e.g: 
                          ConstructSomething( ( cWorld )->getWorldId(), cPlanet.getPlanetId(), cCity.getCityId(), cValue.getValueId() );
                          // I know the above JSON didn't contain values,
                          // but it does in my code for reasons that aren't worth mentioning
                          // This loop is used so many times throughout my code
                          // It would be wonderful to simply be able to grab the data much faster
                     }
                }
            }
        }

I apologise if this question is too vague, but I really need a speed boost or even to make this loop shorter. I'd like any improvements on the format of my tree or how to condense these loops.

I was thinking along the lines of making some kind of address table with pointers - but I can't quite think of a way to do it.

\$\endgroup\$
6
  • \$\begingroup\$ Close voted as not real code. \$\endgroup\$
    – JaDogg
    Sep 25, 2014 at 9:39
  • \$\begingroup\$ What does that mean? \$\endgroup\$
    – fiz
    Sep 25, 2014 at 9:40
  • 1
    \$\begingroup\$ We consider code shortened beyond recognition of the "original" program to be not suited for a review. Please read through our help center for more information. You could make your question on-topic by adding the code you commented out as // Then I do stuff. We will also review that "Stuff", though ;) If you have other questions, feel free to visit Code Review Meta. \$\endgroup\$
    – Vogel612
    Sep 25, 2014 at 9:45
  • \$\begingroup\$ Then I do stuff,e.g:,ConstructSomething doesn't look like real code \$\endgroup\$
    – JaDogg
    Sep 25, 2014 at 9:45
  • \$\begingroup\$ It's not that the code is not real, it's just that it's not complete. This can be easily corrected. The actual question is interesting. \$\endgroup\$
    – iavr
    Sep 25, 2014 at 11:23

2 Answers 2

1
\$\begingroup\$

I cannot think of anything to make it faster.

However, recursion along with some sort of polymorphism and (optionally) visitor pattern can give you the same result without nested loops. Here is an simplified example based on static polymorphism (my code is more abstract than yours but is complete; you might try the same in your question):

template <typename T>
struct vec : std::false_type {};

template <typename T, typename F>
typename std::enable_if <!vec <T>{}>::type
loop(const T& x, F f) { f(x); }

template <typename T, typename F>
typename std::enable_if <vec <T>{}>::type
loop(const T& v, F f)
{
    for (auto &x : v.items())
        loop(x, f);
}

struct A
{
    std::vector <int> v;
    const std::vector <int>& items() const { return v; }
};

struct B
{
    std::array <A, 4> a;
    const std::array <A, 4>& items() const { return a; }
};

template <> struct vec <A> : std::true_type {};
template <> struct vec <B> : std::true_type {};

struct stream
{
    template <typename T>
    void operator()(T&& x) { std::cout << x << " "; }
};

int main()
{
    B b{{{{{1,2,3}}, {{5,6}}, {{7}}, {{8,9}}}}};
    loop(b, stream{});
}

It takes a bit more infrastructure but after that usage is very simple and code is shorter - but not faster.

Type trait vec could be discarded by automatically detecting whether an object has method items() (in which case, loop() recurses down the tree). In this case, the only additional effort in each class would be to define items().

\$\endgroup\$
2
\$\begingroup\$

There is no improvement which can be done on the code you show us. If the function constructSomething really needs to be called for each combination of the parameters, there is nothing better you can do.

You should try to optimizie the function, maybe it is possible to cache some part of the construction which is common if one of the parameters is fixed.

For example you can pop the computation cWord->getWorldId() in the outer loop, so that you make it once for every Word (instead of once for every City). But I think this is a slight improvement.

\$\endgroup\$

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