Suggestions for improving the readability of this code

Question

I am not sure whether I should ask this here or to some other StackExchange site, but I will go on... Please migrate if it is not suitable here)

I am reviewing a code. Requirement is to call a function nnumber of times with argument ranging from 0 to n. But if nis greater than 7, call the function only 7 times.

My colleague implemented it as follows:

void ExecuteFunctions(U8 count)
{
    if(count > 0) oprA(0);
    if(count > 1) oprA(1);
    if(count > 2) oprA(2);
    if(count > 3) oprA(3);
    if(count > 4) oprA(4);
    if(count > 5) oprA(5);
    if(count > 6) oprA(6);
    if(count > 7) oprA(7);
}

I modified it to:

void ExecuteFunctions(U8 count)
{
    for(U8 loopcnt = 0; loopcnt < count; loopcnt++)
    {
        oprA(loopcnt);

        if(loopcnt == 7)
        {
            // We don't want to execute this function more number of times if it is already executed 7 times
            break;
        }
    }
}

But I still feel that there could be a better way and need your inputs. (Also please migrate if it is off topic here)

Let the compiler optimize it. It will probably unroll the loop for you. — Austin Mullins, Mar 03 '15 at 15:59
@AustinMullins Nice idea but Optimization is turned off. And you can say, it became like a Programming riddle for me now! — Swanand, Mar 03 '15 at 16:02
You can move the `loopcnt < 8` condition to the loop condition. It won't optimize it, but make it a bit more compact. — Eugene Sh., Mar 03 '15 at 16:03
The question is unclear: `0` to `7` is already `8` times. is `n` included in the range or not? Both your code and your colleague's don't seem to implement the spec. — chqrlie, Apr 09 '15 at 00:55

score 2 · Accepted Answer · answered Mar 03 '15 at 16:07

2

MIN is a macro useful and often defined

#define MIN(a,b)     (((a) > (b)) ? (b) : (a))
#define MAX_STEP 7

void ExecuteFunctions(U8 count)
{
    int loopcnt = 0;
    count = MIN(count, MAX_STEP);

    while(loopcnt++ < count) {
        oprA(loopcnt);   
    }
}

answered Mar 03 '15 at 16:07

Ôrel

7,044
3
27
46

You should also mention about pitfalls `MIN` macro has. [This link](http://stackoverflow.com/a/3437484/3866447) will be also useful here. – Sam Protsenko Mar 03 '15 at 16:20
`while(loopcnt++ < count)` should be changed to `for (loopcnt = 0; loopcnt < count; loopcnt++)` otherwise `oprA()` is called with incremented values. – chqrlie Apr 09 '15 at 00:39

Austin Mullins · Answer 2 · 2015-04-09T02:01:59.147

2

Let the compiler optimize it. I feel like the following is slightly more readable:

void ExecuteFunctions(U8 count)
{
    for(U8 loopcnt = 0; loopcnt < count && loopcnt < 8; loopcnt++)
    {
        oprA(loopcnt);
    }
}

Of course, you'll need to run a profiler to actually evaluate the performance of your program.

edited Apr 09 '15 at 02:01

answered Mar 03 '15 at 16:12

Austin Mullins

7,307
2
33
48

1

Nice `for` loop. Note: `swtich()` calls `oprA()` in the reverse order. – chux - Reinstate Monica Mar 03 '15 at 18:41
Right, that's why I said "if the order doesn't matter". – Austin Mullins Mar 03 '15 at 18:43
Why yes you did - another example of quick reading and code speaking louder than documentation. – chux - Reinstate Monica Mar 03 '15 at 18:45
This code calls `oprA()` one extra time for `count < 8`, but if neither order nor accuracy matter... – chqrlie Apr 09 '15 at 00:45
I wonder why no one caught that earlier. I'll cut out the switch statement part. – Austin Mullins Apr 09 '15 at 01:59

Ulfalizer · Answer 3 · 2015-03-03T21:47:49.250

I would write it as

void ExecuteFunctions(unsigned count) {
    // Could use min() too if available.
    unsigned iters = count < 7 ? count : 7;

    for (unsigned i = 0; i < iters; ++i)
        oprA(i);
}

The size of the code generated for this function is ~36 bytes on X86_64 with GCC 4.9 and -O3. The size of the original version is 141 bytes (GCC doesn't seem to be very smart with it).

Note that passing plain ints, unsigned ints, size_ts, etc., is usually better than narrowing a parameter just because you know the value will be small. The compiler often has an easier time generating good code for variables that have a natural size for the architecture. An exception is when you need to store large amounts of data.

Update:

The following version (modified a tiny bit from Austin's answer -- sorry for stealing it :)) gives 32 bytes by the way. I think I prefer it.

void ExecuteFunctions(unsigned count) {
    for (unsigned i = 0; i < count && i < 8; ++i)
        oprA(i);
}

There's no such thing as stealing on SO. The best answer will eventually emerge. No need to mention me in yours. — Austin Mullins, Mar 03 '15 at 18:42

score 0 · Answer 4 · answered Mar 03 '15 at 16:30

0

void ExecuteFunctions(U8 count)
{
    for(U8 loopcnt = 0; loopcnt < (count & 7); loopcnt++)
    {
        oprA(loopcnt);
    }
}

answered Mar 03 '15 at 16:30

EvilTeach

28,120
21
85
141

That will call it zero times for `count == 8`. Might be some similar trick though... – Ulfalizer Mar 03 '15 at 16:36

score 0 · Answer 5 · answered Apr 09 '15 at 00:51

If you really like switch statements, try this and see if it survives code review:

void ExecuteFunctions(U8 count) {
    int i = 0;
    switch (count) {
      default: oprA(i++);
      case 6:  oprA(i++);
      case 5:  oprA(i++);
      case 4:  oprA(i++);
      case 3:  oprA(i++);
      case 2:  oprA(i++);
      case 1:  oprA(i++);
      case 0:  oprA(i);
    }
}

Suggestions for improving the readability of this code

5 Answers5