# Thread: Sample Function Optimisations For a Novice

1. ## Sample Function Optimisations For a Novice

Hi,

New user here, expert JavaScript and ex Perl but C is all new and I'm keen. I'm hoping you can give some tips on the following (mark my work!).

What it does is it calculates a duty to hand to a pwm, which lights an LED. This duty is multiplied by the value of an 8 bit timer so that the LED fades up and down from off - on - off.

The values fed into the function are <=52 and 40 respectively. (the "52" allows for different voltage level led colours if your wondering).

adc.voltages[ADC_POS_BOOST] can have a value of 0 to 1023.

Bonus point if anyone knows how to make the LED fade in/out in a logarithmic way better suited to eye sensitivity.

Code:
```uint16_t __getLedDuty(uint8_t multi, uint8_t minus) {

uint16_t duty = (uint16_t) ((uint16_t)(1023U*multi) / (adc.voltages[ADC_POS_BOOST])) - minus;
uint8_t halfPeriod = LED_TIMER_FLASH_PERIOD/2U;
uint8_t myValue = LED_TIMER_FLASH_READ? (LED_TIMER_FLASH_READ > halfPeriod? LED_TIMER_FLASH_PERIOD - LED_TIMER_FLASH_READ : LED_TIMER_FLASH_READ) : 0U;
uint16_t myDuty = myValue? (uint16_t) (myValue*100U) / halfPeriod : 0U;
return myDuty? (uint16_t) ((duty*myDuty) / 100U) : 0U;
}```
Many thanks,

Andrew

2. My first tip would be to rewrite it so you don't have any ?: operators in the code.
The code as is is basically unreadable.

For example,
Code:
`uint16_t myDuty = myValue? (uint16_t) (myValue*100U) / halfPeriod : 0U;`
Becomes
Code:
```uint16_t myDuty;
if ( myValue ) {
myDuty = (uint16_t) (myValue*100U) / halfPeriod;
} else {
myDuty = 0;
}```
> uint16_t __getLedDuty
To quote the C99 standard.
7.1.3 Reserved identifiers
All identifiers that begin with an underscore and either an uppercase letter or another
underscore are always reserved for any use.

3. That's a style over optimisation though, I prefer the ?:; shorthand.

Is there a means in C to say a function should only be available for it's .c file? (aka private)?

4. Originally Posted by snoop33
That's a style over optimisation though, I prefer the ?:; shorthand.
It is true that it is a matter of style, but nesting ternary operators is pretty much always poor style, so at the very least you should get rid of that.

Originally Posted by snoopy33
Is there a means in C to say a function should only be available for it's .c file? (aka private)?
Declare it static. Of course, this means you should not declare it in a header.

5. > That's a style over optimisation though
Optimisation has very little to do with the number of characters in your source code.

Code:
```#include <stdio.h>

typedef unsigned short uint16_t;

uint16_t foo ( uint16_t myValue, uint16_t halfPeriod ) {
uint16_t myDuty = myValue? (uint16_t) (myValue*100U) / halfPeriod : 0U;
return myDuty;
}

uint16_t bar ( uint16_t myValue, uint16_t halfPeriod ) {
uint16_t myDuty;
if ( myValue ) {
myDuty = (uint16_t) (myValue*100U) / halfPeriod;
} else {
myDuty = 0;
}
return myDuty;
}```
The respective assembler codes with no optimisation at all.
Code:
```foo:
pushq	%rbp
movq	%rsp, %rbp
movl	%edi, %edx
movl	%esi, %eax
movw	%dx, -20(%rbp)
movw	%ax, -24(%rbp)
cmpw	\$0, -20(%rbp)
je	.L2
movzwl	-20(%rbp), %edx
movl	%edx, %eax
sall	\$2, %eax
leal	0(,%rax,4), %edx
sall	\$2, %eax
movl	\$0, %edx
divw	-24(%rbp)
jmp	.L3
.L2:
movl	\$0, %eax
.L3:
movw	%ax, -2(%rbp)
movzwl	-2(%rbp), %eax
popq	%rbp
ret

bar:
pushq	%rbp
movq	%rsp, %rbp
movl	%edi, %edx
movl	%esi, %eax
movw	%dx, -20(%rbp)
movw	%ax, -24(%rbp)
cmpw	\$0, -20(%rbp)
je	.L6
movzwl	-20(%rbp), %edx
movl	%edx, %eax
sall	\$2, %eax
leal	0(,%rax,4), %edx
sall	\$2, %eax
movl	\$0, %edx
divw	-24(%rbp)
movw	%ax, -2(%rbp)
jmp	.L7
.L6:
movw	\$0, -2(%rbp)
.L7:
movzwl	-2(%rbp), %eax
popq	%rbp
ret```
Compile with optimisation, and you get less instructions. But still the SAME set of instructions.

The rule nowadays is go for readability every time unless you have a damn good reason and lots of evidence (not 30 year old heresay) that cryptic compressed code is actually better. Mostly it won't be, because code that confuses human readers also confuses code optimisers.

6. Speaking of that, I note that if myValue == 0, the computation would still result in myDuty == 0. Considering the computation is just a multiplication, a cast, and a division, it looks unnecessary to treat it as a special case unless you're saying that a non-zero myValue is expected to be rare. If not, this is premature optimisation (and might even be a "pessimisation" if it turns out that myValue == 0 is rare).

Popular pages Recent additions