Could this be optimized even better?

[HenriK]

Hello, guys, long time no see.

Here I happen to have a header & source file (both written in C, because of it´s portability, low-level nature & very small output size):

#ifndef PETIT_COLLECTIBLES
#define PETIT_COLLECTIBLES
#ifdef __cplusplus
extern "C"{
#endif /*__cplusplus*/
#ifndef PETITAPI
	#ifdef _WIN32
		#define PETITAPI __declspec(dllexport)
	#else /*_WIN32*/
		#define PETITAPI
	#endif /*else*/
#endif /*PETITAPI*/
#ifndef bool
	#define false 0
	#define true 1
	typedef int bool;
#endif /*bool*/
typedef struct{
	int ext, ext_max;
	int amount, amount_max;
}PetitCollectible;
extern inline PETITAPI void PetitCollectibleChange(PetitCollectible*,bool,int);
extern PETITAPI void PetitCollectibleFix(PetitCollectible*,bool*);
extern inline PETITAPI void PetitCollectibleSet(PetitCollectible*,bool,int);
#ifdef __cplusplus
}
#endif /*__cplusplus*/
#endif /*PETIT_OBJECTS_COLLECTIBLES*/ 

#include "collectibles.h"

void PetitCollectibleChange(PetitCollectible* collectible, bool extension, int value){
	if(extension){
		collectible->ext += value;
	}
	else{
		collectible->amount += value;
	}
	PetitCollectibleFix(collectible,&extension);
}
void PetitCollectibleFix(PetitCollectible* collectible, bool* extension){
	if(extension){
		if(collectible->ext > collectible->ext_max){
			collectible->ext = collectible->ext_max;
		}
		else if(collectible->ext <= 0){
			collectible->ext = 0;
		}
	}
	else{
		if(collectible->amount > collectible->amount_max){
			collectible->amount = collectible->amount_max;
		}
		else if(collectible->amount <= 0){
			collectible->amount = 0;
		}
	}
}
void PetitCollectibleSet(PetitCollectible* collectible, bool extension, int value){
	if(extension){
		collectible->ext = value;
	}
	else{
		collectible->amount = value;
	}
	PetitCollectibleFix(collectible,&extension);
}

Could this code be even faster/smaller or simply better overall? I hope you accept my C attempt, because, after all, isn't C part of C++, a subset?

PS: Please ignore Win32 definitions (__declpec, etc).

[sohguanh]

When you say optimized further are you referring to make the program run faster in shorter time for your above program ?

[HenriK]

To me optimization means faster & smaller (in both code & output) result. Correct me if I'm wrong, but I hope you got I am looking after.

Any suggestions?

[sohguanh]

From your code snippet I see they are pretty ok why are you asking it can be optimized further? From my working experience, very often big performance gains are not in those if-else, pointer referencing, assignment etc (although they matter if objects are huge). You can gain 50% performance gains in your choice of proper data structures and algorithms. These are very important concepts in programming. You choose it wrongly no matter how you tune your statements you are not going to get gains of 50% or even more.

Above is my opinion so please don't flame me :P

[HenriK]

So, what should I do to create better code?

[sohguanh]

So, what should I do to create better code?

You have asked a question for which I have been searching for an answer since I started programming for a living. Until today I do not think I have found the answer yet. It is like hunting for the holy grail :P

I will let other veteran C++ developers to contribute their valuable advice instead.

[1vRz3TCk]

First; run your code through a profiler to see if you have got any performance issues. Then look at the area of the code that is indicated to have issues. i.e. Don't worry about making that code more efficient unless it is shown that you have to.

[HenriK]

OK, fine. I hope I get more advice somewhere else then. Feedback anywhere is welcome!

[1vRz3TCk]

Hmm. I sorry if you feel that the advice given is lacking.

You give a struct and three small functions. It may be worth making six functions without the if(extension), that may save you something... depending on how the decision is made to set extension. Any savings will be small and may only be worth the effort if you are calling the functions a lot.

This would save you a few clock cycles

void PetitCollectibleSet(PetitCollectible* collectible, bool extension, int value){
    if(extension){
        collectible->ext = value;
        PetitCollectibleFixExt(collectible);
    }
    else{
        collectible->amount = value;
        PetitCollectibleFix(collectible);
    }
}
void PetitCollectibleChange(PetitCollectible* collectible, bool extension, int value){
    if(extension){
        collectible->ext += value;
        PetitCollectibleFixExt(collectible);
    }
    else{
        collectible->amount += value;
        PetitCollectibleFix(collectible);
    }
    
}
void PetitCollectibleFixExt(PetitCollectible* collectible){
    if(collectible->ext > collectible->ext_max){
        collectible->ext = collectible->ext_max;
    }
    else if(collectible->ext <= 0){
        collectible->ext = 0;
    }
}
void PetitCollectibleFix(PetitCollectible* collectible){
    if(collectible->amount > collectible->amount_max){
        collectible->amount = collectible->amount_max;
    }
    else if(collectible->amount <= 0){
        collectible->amount = 0;
    }
}

[bartoli]

CodeMonkey's first post was a quite good advice in my opinion.

For other improvement ways, you could see if the use of restrict pointers is valid in your case.

Also, PetitCollectibleFix takes a bool* for extension. And you call it in Set and Change, giving the pointer to a variable allocated on the heap. So extension will always be not null in Fix(). If it is normal code, then you can remove the test for extension in Fix(). Or maybe that's a bug?

[HenriK]

CodeMonkey's post seems OK. However, which one is faster really?

void petitCollectibleChange(PetitCollectible* collectible, bool extension, int value){
    if(extension){
		collectible->ext += value;
    }
    else{
		collectible->amount += value;
    }
	petitCollectibleFix(collectible);
}

void petitCollectibleChange(PetitCollectible* collectible, bool extension, int value){
    if(extension){
        collectible->ext += value;
        petitCollectibleFix(collectible);
    }
    else{
        collectible->amount += value;
        petitCollectibleFix(collectible);
    }
}

[1vRz3TCk]

In the code you posted, you do a comparison in one function then call another and do the same comparison again, mine saves a bit on not 'shipping' the bool to another function and there is no need for the second comparison. It will be a little faster but not much...not noticeable unless you are calling the functions a vast number of times.

[HenriK]

Oops, I made a mistake. Forgot that there were two different Fix -functions.