1
\$\begingroup\$

I have a linearized 2D array u (block_height * block_width) containing the values of a physical quantity over a regular 2D mesh. I need to downsample boundaries (top, bottom, left, right) of this array for communication with another process. I've refactored some code that does this, and I'd like people's thoughts on whether the new code is actually an improvement.

Old code:

if (dir == LEFT)
    for(int j = 1; j <= block_height; j += 2)
        left_edge[j/2] = (u[index(1,j)]   + u[index(2,j)] +
                          u[index(1,j+1)] + u[index(2,j+1)]) / 4;
else if (dir == RIGHT)
    for(int j = 1; j <= block_height; j += 2)
        right_edge[j/2] = (u[index(block_width-1, j)]   + u[index(block_width,j)] +
                           u[index(block_width-1, j+1)] + u[index(block_width, j+1)]) / 4;
else if (dir == UP)
    for(int i = 1; i <= block_width; i += 2)
        top_edge[i/2] = (u[index(i,1)]   + u[index(i,2)] +
                         u[index(i+1,1)] + u[index(i+1,2)]) / 4;
else if (dir == DOWN)
    for(int i = 1; i <= block_width; i += 2)
        bottom_edge[i/2] = (u[index(i,block_height-1)]   + u[index(i,block_height)] +
                            u[index(i+1,block_height-1)] + u[index(i+1,block_height)]) / 4;

New code:

double *boundary;
int k, fixed_dim;
int *x, *y;
switch (dir) {
case LEFT: case RIGHT: count = block_height; x = &fixed_dim; y = &k; break;
case UP:   case DOWN:  count = block_width;  y = &fixed_dim; x = &k; break;
switch (dir) {
case LEFT:  boundary = left_edge;   fixed_dim = 1;                break;
case RIGHT: boundary = right_edge;  fixed_dim = block_width - 1;  break;
case UP:    boundary = top_edge;    fixed_dim = 1;                break;
case DOWN:  boundary = bottom_edge; fixed_dim = block_height - 1; break;
}
for (k = 1; k <= count; k += 2)
  boundary[k/2] = (u[index(*x, *y)]   + u[index(*x+1, *y)] +
                   u[index(*x, *y+1)] + u[index(*x+1, *y+1)] )/ 4;

So, I've factored out the repeated for loop and made the logic more declarative, at the expense of introducing some additional indirection (including over which direction the loop iterates through memory!) and the variables to support it.

One obvious direction of future change for this code is increasing from 2 dimensions to 3 (and possible even more). I know which one I'd prefer to do that to, but if the old code is overwhelmingly clearer to people reading it, then the repetition may be worth retaining.

\$\endgroup\$
2
  • \$\begingroup\$ This should also be tagged 'pointer', but not enough rep transfered over from SO to let me create that. \$\endgroup\$ – Phil Miller May 16 '12 at 23:56
  • \$\begingroup\$ So, I went whole-hog with the iterator approach, and it seems to be working out quite well. A lot of other code in the project was simplified using the same structure. \$\endgroup\$ – Phil Miller May 20 '12 at 14:55
1
\$\begingroup\$

I think the first one is more understandable to a reader, while the second is more efficient and or more easily adapted to higher dimensions. Have you thought about something like:

if (dir == LEFT || dir == RIGHT){        
  int x = dir == LEFT ? 1 : block_width-1;
  double *boundary = dir == LEFT ? left_edge : right_edge;
  for(int j = 1; j <= block_height; j += 2)
    left_edge[j/2] = (u[index(x,j)]   + u[index(x+1,j)] +
                      u[index(x,j+1)] + u[index(x+1,j+1)]) / 4;
}
else if (dir == UP || dir == DOWN){
  int y = dir == UP ? 1 : block_height-1
  double *boundary = dir == UP ? top_edge : bottom_edge;
  for(int i = 1; i <= block_width; i += 2)
    boundary[i/2] = (u[index(i,y)]   + u[index(i,y+1)] +
                     u[index(i+1,y)] + u[index(i+1,y+1)]) / 4;
}

Or if the second is more inclined I would recommend just clear comments to show the intention of each variables meaning.

Also in the future adapting to more c++ style arrays (vectors), utilizing a c++ Matrix library, or making your own iterator (boundary iterator) would make for a very nice solution to this problem.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the suggestion of an intermediate point between my two solutions; I hadn't thought of that. \$\endgroup\$ – Phil Miller May 17 '12 at 6:06
  • \$\begingroup\$ I can clearly see how using a real matrix/multi-dimensional array library or an iterator would simplify this code. I'm less clear on what C++ std::vector changes about it, though. The only difference I see is that boundary = foo becomes boundary = &foo[0]. \$\endgroup\$ – Phil Miller May 17 '12 at 6:08

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.