CODEWITHSUNDEEP
ccode-readability
Swanand
Swanand

Reputation: 4115

Suggestions for improving the readability of this code

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)

Upvotes: 1

Views: 125

Answers (5)

Austin Mullins
Austin Mullins

Reputation: 7437

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.

Upvotes: 2

chqrlie
chqrlie

Reputation: 144951

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);
    }
}

Upvotes: 0

Ulfalizer
Ulfalizer

Reputation: 4762

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);
}

Upvotes: 1

EvilTeach
EvilTeach

Reputation: 28872

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

Upvotes: 0

&#212;rel
&#212;rel

Reputation: 7632

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);   
    }
}

Upvotes: 2

Related Questions