How can i optimize this code

[ArunS]

Hello,

I just want to know how can i optimize this code.
my requirement: i will receive frame of data via interrupt as: Length, Inverted Length, Data[Length], BCC.
I have done the code and it is working.
how can i optimize this?

         if(Count == 1)
         {
               length = ValueFromInterrupt;
               Count++;
         }
         else if(Count == 2)
         {
               InvertedLength = ValueFromInterrupt;
               Count++;
         }
         else if(Count < (length+3))
         {
               buffer[buffer_Count] = ValueFromInterrupt;
               Checksum_BCC ^= ValueFromInterrupt;
               Count++;
               buffer_Count++;
         }
         else if(Count == (length+3))
         {
               if(ValueFromInterrupt == Checksum_BCC)
               {
                  Count=0;
                  Checksum_BCC = 0;
               }
               else
               {
                  /*Delete the buffer data*/
                  buffer_Count=0;
                  Count=0;
               }
          }

Thank you in advance

[rogster001]

What makes you think it isnt optimised anyway?

[ArunS]

How can i change it into switch case?

[laserlight]

How can i change it into switch case?

It does not look like a good candidate for that. I don't see anything obviously inefficient about the code snippet, though if you know which condition is most likely to be true, you could do a minor optimisation by testing for that condition first, but if this is not part of a performance bottle neck in your code, it really would not matter in the slightest.

[itCbitC]

I just want to know how can i optimize this code.

By leaving it alone

[AndrewHunter]

As laser pointed out, unless you are actually seeing a performance bottle neck it isn't worth worrying about. The concept of code optimization, with the exception of some embedded system design and certain high performance applications, past writing clean manageable code is largely outdated and a throwback to the dark ages when compilers were clunky beasts and processors were stupid.

In this day and age, most compilers are exceptionally good at what they do. In fact trying to out think the compiler often results is less optimal code because the compiler cannot figure out what you want to do and thus leaves the code as is; which in case you were wondering is not as efficient as it could be. Additionally, processors have evolved and are no longer the single cell amoebas they use to be; current branch prediction success is usually in the neighborhood of > 90%.

[KCfromNC]

Minor optimization which will also make the code more readable. Readability's the only reason to do it since any non-broken optimizing compiler should figure it out for you any way.

        {
               Count=0;
               if(ValueFromInterrupt == Checksum_BCC)
               {
                     Checksum_BCC = 0;
               }
               else
               {
                  /*Delete the buffer data*/
                  buffer_Count=0;
               }
        }

You might also try something more error-resistant than a simple XOR checksum, assuming you have access to both the sending and receiving code. CRC16 or 32 wouldn't be too much more work and way better.

[quzah]

if( Count == length+3 )
{
    Count = 0;
    if( ValueFromInterrupt == Checksum_BCC )
        Checksum_BCC = 0;
    else
        buffer_Count = 0;
}
else
{
    if(Count == 1)
        length = ValueFromInterrupt;
    else if(Count == 2)
        InvertedLength = ValueFromInterrupt;
    else
    {
        buffer[buffer_Count] = ValueFromInterrupt;
        Checksum_BCC ^= ValueFromInterrupt;
        buffer_Count++;
    }
    Count++;

}

It's not any more efficient, but there are less repeated lines.


Quzah.

[grumpy]

It's not any more efficient, but there are less repeated lines.

There is a minor concern with a missing curly brace, but that's just a typo.

The only other change that might be relevant is changing post-increment to pre-increment in standalone statements. It doesn't make much difference with most modern compilers, and doesn't change the end result of the code, but is more semantically correct (a post-increment, semantically, involves a temporary that is not needed in this case).

[iMalc]

It should also be noted that quzah's version increments Count before checking if it is 1 or 2, thus is incorrect in that manner also. I'm sure he'll fix that now.
It also does stuff when Count > length+3, although we don't actually know if this case can actually occur.

Case in point, if it aint broke - don't fix it!

[quzah]

It should also be noted that quzah's version increments Count before checking if it is 1 or 2, thus is incorrect in that manner also. I'm sure he'll fix that now.

No. I won't fix it, because it doesn't need fixing. All three of those cases increment Count, and don't do anything else with it. It's fine how I have it.


Quzah.

[grumpy]

No. I won't fix it, because it doesn't need fixing. All three of those cases increment Count, and don't do anything else with it. It's fine how I have it.

Sorry; iMalc is correct.

The code is incrementing Count, and then testing the value of Count. The values being tested against need to be incremented before testing, or do the incrementing of Count AFTER the other stuff.

One of the "joys" of optimising code is that it is very easy to subtly change its meaning.

[quzah]

Sorry; iMalc is correct.

I see. I also had it typoed in the first if check.


Quzah.

[AndrewHunter]

One of the "joys" of optimising code is that it is very easy to subtly change its meaning.

"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it."

-Brian Kernighan

[KCfromNC]

if( Count == length+3 )
{
    Count = 0;
    if( ValueFromInterrupt == Checksum_BCC )
        Checksum_BCC = 0;
    else
        buffer_Count = 0;
}
else
{
    if(Count == 1)
        length = ValueFromInterrupt;
    else if(Count == 2)
        InvertedLength = ValueFromInterrupt;
    else
    {
        buffer[buffer_Count] = ValueFromInterrupt;
        Checksum_BCC ^= ValueFromInterrupt;
        buffer_Count++;
    }
    Count++;

}

It's not any more efficient, but there are less repeated lines.


Quzah.

This may be slightly slower if the common case is the last nested else since you're doing an extra if before you get there. Not that I'd bet it would make a measurable difference, though.