troublesh00ter wrote:When you say 'wrong' I guess you don't always mean that the result is incorrect - but also that it can be done better/faster?
#1:
- Code: Select all
// Dividing (especially floats) is slow
CUtils::CallFunction( fX * 0.0625f, fY * 0.0625f );
This is a 90% answer, but the concept behind the question has been solved.
The 100% answer is:
- Code: Select all
CUtils::CallFunction( (1.0f / 16.0f) * fX, (1.0f / 16.0f) * fY );
Rule #1: Put the constants to the left of the variable. This guarantees that the compiler will perform the division at compile-time even if you forget the parentheses.
Rule #2: Use parentheses anyway. It makes it clear to viewers that that constant will be evaluated by itself, and ensures no complications with other multiplications or divisions.
Rule #3: Use the long form of 1.0f / 16.0f instead of just using the constant 0.0625f. This allows everyone to easily determine how the constant was derived and allows them to easily reconsider the problem as a division rather than a multiplication of an arbitrary number.
troublesh00ter wrote:#2:
- Code: Select all
vVec.x = ::cos( fA );
vVec.y = -::sin( fA );
vVec.z = -vVec.y; // No need to call sin() twice
Is there something more you can do to this?
troublesh00ter wrote:#4:
- Code: Select all
// No elements in the vector are destroyed/created.. so the size is constant
// leaving .size() in the for loop will mean it gets called on every iteration
unsigned long ulVecSize = vVector.size();
for ( size_t I = 0UL; I < ulVecSize; ++I ) {
ulMax = std::max( ulMax, vVector[I] );
}
That is one good idea, but is there a better way to go about this?
troublesh00ter wrote:#5:
- Code: Select all
[/quote]
ulSwitch = 0; // Don't really see what's wrong with this one...
// Guess it had something to do with left-to-right and right-to-left.. but I thought that only mattered if you
// combine expressions like: y = x / ++x;
// Ran the following snippet for a while and it never broke the loop.
// Or is it in fact a left-to-right problem and am I just lucky that my compiler does it right?
while( true )
{
++ulSwitch;
ulSwitch %= 2UL;
if( ulSwitch != ! ulLastResult )
break;
ulLastResult = ulSwitch;
}
printf( "ulSwitch %u\n", ulSwitch );
Your loop will never break because the code is doing the same thing as
ulSwitch = !ulSwitch.
Is there a better way to write this?
troublesh00ter wrote:#6:
- Code: Select all
ulY = ulIndex / 16UL; // % is unnecessary if we also need the division
ulX = ulIndex - ulY * 16UL;
I am afraid this is incorrect.
CoMPMStR wrote:#3:
- Code: Select all
int iSize = static_cast<int>(vVector.size() - 1UL);
for ( int I = iSize; I >= 0; --I ) {
// Do whatever.
}
or
- Code: Select all
int iSize = vVector.size() - 1;
for ( int I = iSize; I >= 0; --I ) {
// Do whatever.
}
I am afraid that each of these is at least as problematic as the original snippets.
cobr_h wrote:if it is for performance, I think:
for #4 I would
- Code: Select all
unsigned long ulMax = 0UL;
for ( size_t I = (sizeof(vVector)/sizeof(unsigned long))-1; I > 0; --I ) {
ulMax = std::max( ulMax, vVector[I] );
}
no more memory used and not a lot of calculations done. in the end it's the same solution proposed by troubleshooter.
And I have considered that, if you wanna call vVector with brackets, it is not supposed to have any method... or else it would be an vector of objects. Did I take in consideration subtle detail?
The original snippet used a vector class, which would of course give you some troubles if you were to apply the sizeof() operator in this manner; however, as stated, this solution only uses the vector class as an example, and you are indeed free to change it to a regular array in your answer. This is, however, not part of the answer.
On the brighter side, you did get a 50% score for another reason. Can you finish it off for the final 100%?
cobr_h wrote:- Code: Select all
ulSwitch=!ulSwitch;
As I really have seen in a code around (tutorials or so).
This is very superior over the code in the original snippet.
Is there a better way to write this?
L. Spiro
Our songs remind you of songs you’ve never heard.