Generalizing these lines of code?

Question

While programming, I ran into a wall with some code. It looks like this:

And that's the problem. I took a pretty screenshot to reduce my guilt. The pretty colors do not make up for the lack of maintainability. I have close to no idea how to generalize code like this.

What I tried?

Well, consider the periodicity of the 3rd and 6th arguments. It aligns with the periodicity of the other arguments too. Something like this would allow us to convert this code into a loop with 9 lines if we use an array. That's an improvement as we go down by 66%. However, that's not good enough. It would be best if this were changed to have 1 line. That would at least make it a bit more maintainable.

Is this really a problem?

Well, let's put it this way. That code up there may as well be wrong.

Answer 1

Well, it took some time to analyze the patterns.

Of course, first I used http://www.onlineocr.net/ to get the text from the screenshot. Then I started match highlighting to spot patterns.

• You can see that make_cube takes effectively two (x,y,z) tuples
• There are three groups or lines that end in the same z value
• These three groups consist of three subgroups that end in the same (y,z) tuple
• The x, y and z values enumerate the same pairs of values for each group.

This makes it "obvious" material for a generation loop. After some 20 minutes of refactoring I was down to

for (auto&& zs : { tie(rmin_z, imin_z), tie(imin_z, imax_z), tie(imax_z, rmax_z) })
    for (auto&& ys : { tie(rmin_y, imin_y), tie(imin_y, imax_y), tie(imax_y, rmax_y) })
        for (auto&& xs : { tie(rmin_x, imin_x), tie(imin_x, imax_x), tie(imax_x, rmax_x) })
{
    *out++ = make_cube(get<0>(xs), get<0>(ys), get<0>(zs), get<1>(xs), get<1>(ys), get<1>(zs));
}

But you'll notice the regularity in the loop ranges. Actually we have a sequence like

coord const sequence[] = { rmin, imin, imax, rmax };

and we select consecutive pairs: (rmin, imin), (imin, imax), (imax, rmax)

// we take all consecutive pairs (warning: ignoring the `(rmax, rmin)` closing pair here)
vector<pair<coord, coord>> pairs;
transform(begin(sequence), prev(end(sequence)), back_inserter(pairs), [](coord const& it) { return std::make_pair(*(&it+0), *(&it+1)); });

Now we can loop it more directly. I've also invented a simple Cube type that allows us to pretty print the result of the generator loop so you can verify the results in DEBUG mode:

for (auto zs : pairs) for (auto ys : pairs) for (auto xs : pairs)
    *out++ = Cube { { xs.first.x, ys.first.y, zs.first.z }, { xs.second.x, ys.second.y, zs.second.z } }; 

Live On Coliru

#include <iostream>
#include <algorithm>
#include <vector>
#include <array>

int main() {
#ifdef NDEBUG
    typedef double T;
    struct coord { T x,y,z; };

    coord rmin { 0,  1,  2 },
          imin { 3,  4,  5 },
          imax { 6,  7,  8 },
          rmax { 9, 10, 11 };
#else
    typedef const char* T;
    struct coord { T x,y,z; };

    coord rmin { "rmin_x", "rmin_y", "rmin_z" },
          imin { "imin_x", "imin_y", "imin_z" },
          imax { "imax_x", "imax_y", "imax_z" },
          rmax { "rmax_x", "rmax_y", "rmax_z" };
#endif
    using namespace std;

    // the source sequence
    coord const sequence[] = { rmin, imin, imax, rmax };

    // we take all consecutive pairs (warning: ignoring the `(rmax, rmin)` closing pair here)
    vector<pair<coord, coord>> pairs;
    transform(begin(sequence), prev(end(sequence)), back_inserter(pairs), [](coord const& it) { return std::make_pair(*(&it+0), *(&it+1)); });

    // Now we build cubes. The `make_cube` interface implied it requires two
    // coordinates to be constructed:
    struct Cube { coord p1, p2; };
    std::array<Cube, 3*3*3> cubes;

    // generate!
    auto out = cubes.begin();
    for (auto zs : pairs) for (auto ys : pairs) for (auto xs : pairs)
        *out++ = Cube { { xs.first.x, ys.first.y, zs.first.z }, { xs.second.x, ys.second.y, zs.second.z } }; 

    // debug print
    for(auto const& c : cubes)
        std::cout << "make_cube(" << c.p1.x << ", " << c.p1.y << ", " << c.p1.z << ", " << c.p2.x << ", " << c.p2.y << ", " << c.p2.z << ")\n";
}

Conclusions:

1. My code looks more complicated. It probably is. But it's much easier to see whether typos were made

2. Regarding the question

   Is this really a problem?

   Well, let's put it this way. That code up there may as well be wrong

   Indeed, I have a bit of a doubt whether you covered all your cases. See the first comment:

   // we take all consecutive pairs (warning: ignoring the `(rmax, rmin)` closing pair here)