|
Assuming it really is faster, this method isn't really part of the engine itself. it's not like this function will be run over and over again. Me personally, i wouldn't compromise readability over such a small performance increasement.
But i'm severely starting to doubt myself, a lot of people seem to disagree this is a codehorror :s
|
|
|
|
|
Don't fret about posting. Most of the horror posts don't seem to generate this much discussion. I see that as a good thing.
|
|
|
|
|
Ye i like the activity on this forum 
|
|
|
|
|
have you ever had time to go get a burger during a loading scene...? 
I'd blame it on the Brain farts.. But let's be honest, it really is more like a Methane factory between my ears some days then it is anything else...
-"The conversations he was having with himself were becoming ominous."-.. On the radio...
|
|
|
|
|
.. argh I should have read all the sub threads before posting.. sorry.
I'd blame it on the Brain farts.. But let's be honest, it really is more like a Methane factory between my ears some days then it is anything else...
-"The conversations he was having with himself were becoming ominous."-.. On the radio...
|
|
|
|
|
What you have posted here is exactly what DisplayMode's equals method does, so you could replace the whole lot with:
return mode1.equals(mode2);
You can't get much simpler than that.
But that is not what the original code does. The original code takes BIT_DEPTH_MULTI and REFRESH_RATE_UNKNOWN into account, which your code and the equals method does not.
|
|
|
|
|
But aren't these simply constants? So the value would still have to be the same right? if so, this would mean that you don't need to take them into account. Or am i missing the point :s
|
|
|
|
|
Yes, they are constants, but you do need to take them into account. Consider the following:
if (mode1.getBitDepth() != DisplayMode.BIT_DEPTH_MULTI &&
mode2.getBitDepth() != DisplayMode.BIT_DEPTH_MULTI &&
mode1.getBitDepth() != mode2.getBitDepth())
This is saying:
- if either mode1 or mode2 has BIT_DEPTH_MULTI then I can ignore the bit depth, and I can consider that these two display modes match no matter what bit depth they have.
- if neither mode1 nor mode2 have BIT_DEPTH_MULTI then I need to check that the bit depth matches.
This is different from the equals method, which says that they are only equal if they both have exactly the same bit depth, whether that is BIT_DEPTH_MULTI or not.
|
|
|
|
|
ty for the clarification it appears that these need to be added to the line also. Though this changes nothing to the structural fail of the code
|
|
|
|
|
not necessarily, you have mip-maping, or dynamic scaling, or in game render target changes...
I'd blame it on the Brain farts.. But let's be honest, it really is more like a Methane factory between my ears some days then it is anything else...
-"The conversations he was having with himself were becoming ominous."-.. On the radio...
|
|
|
|
|
you version here is more costly...
You have to remember that when your doing a game engine each comparison burns cycles, the initial example i would argue is faster to execute, if you know more about your environment...
if the first statement is true, then you cut down on the number of lookups...
using your "better code" actually has worse performance... because you force all the lookups! 
I'd blame it on the Brain farts.. But let's be honest, it really is more like a Methane factory between my ears some days then it is anything else...
-"The conversations he was having with himself were becoming ominous."-.. On the radio...
|
|
|
|
|
Check my last reply, 1 statement, no variables, only indenting to preserve readability and my personnal favorite
|
|
|
|
|
I likes it at first glance.... succinct.
These calls in my experience can be a bit black box however and although would work fine in situations where compute time isn't at a premium is always the best way to go, i would (at least in my projects) check to see if it is actually faster and produces less garbage (GC type garbage, not the visual type, which it obviously doesn't...).
it's also been my experience that a lot of these nice API methods are just wrappers for crappy unoptimized code... the XNA. Vector3d.Distance function is a prime example of this... problem with API functions is that they need to be generic, and work in all situations (even when given properties..etc) and as such often make internal scope storage "inside the black box" to accomplish their result...{actually posed this question to a guy who wrote that function on the XNA team.. this was his answer!} so I see it as elegantly coded, but not necessarily the best approach in all situations..
I'd blame it on the Brain farts.. But let's be honest, it really is more like a Methane factory between my ears some days then it is anything else...
-----
"The conversations he was having with himself were becoming ominous."-.. On the radio...
|
|
|
|
|
Your code also checks things it needn't - once the first bool is false we know the displays are not the same so we can short circuit and return - why do the extra compares unless you just want to burn a bit of CPU
|
|
|
|
|
I think the original version is better because I read it once and was reasonably confident I knew what it did, and also what was intended.
The shorter, cleverer version I looked at and said "Eh?".
In my first job I discovered that programs might have to survive being "fixed" by a half-drunk trainee who had been dragged from the pub when something went wrong on Friday night at the start of a holiday weekend and all the sober and competent programmers were gone. I know, I was that trainee.
OK, so this isn't an IBM 360 COBOL accounting program, but still, clarity of intention is desirable.
KISS, in fact.
|
|
|
|
|
This format definitely has an advantage in that if you suspect displayModesMatch of doing something wrong, or otherwise just want to know what it is doing, you only need to set one breakpoint.
I guess both approaches have their merits.
But before we just go ahead and assume that this way is better than that way, let's see how C++ would have done this task. I found the results quite surprising.
To simplify, I used the following code snippet:
bool test1(int dim1, int dim2, int depth1, int depth2, int refresh1, int refresh2)
{
bool isEqDim = (dim1 == dim2);
bool isEqDepth = (depth1 == depth2);
bool isEqRefresh = (refresh1 == refresh2);
return isEqDim && isEqDepth && isEqRefresh;
}
bool test2(int dim1, int dim2, int depth1, int depth2, int refresh1, int refresh2)
{
if (dim1 != dim2)
return false;
if (depth1 != depth2)
return false;
if (refresh1 != refresh2)
return false;
return true;
}
void trialrig()
{
int dim1, dim2, depth1, depth2, refresh1, refresh2;
scanf("%i", &dim1);
scanf("%i", &dim2);
scanf("%i", &depth1);
scanf("%i", &depth2);
scanf("%i", &refresh1);
scanf("%i", &refresh2);
bool b1_1 = test1(dim1, dim2, depth1, depth2, refresh1, refresh2);
bool b2_1 = test2(dim1, dim2, depth1, depth2, refresh1, refresh2);
if (b1_1)
printf("b1 is true\n");
if (b2_1)
printf("b2 is true\n");
}
The scanf's were important, because if they're not there, my compiler just optimized both the test functions out of existence.
During compiling, some of these were inlined, so I have to compensate somewhat. So for simplicity, assume that both code blocks below are prefixed with these instructions:
mov eax, DWORD PTR _dim1$[esp+88]
mov ecx, DWORD PTR _dim2$[esp+88]
mov esi, DWORD PTR _depth1$[esp+88]
mov edi, DWORD PTR _depth2$[esp+88]
mov ebx, DWORD PTR _refresh1$[esp+88]
mov ebp, DWORD PTR _refresh2$[esp+88]
In release build, it produced the following for one of the two functions (excluding the preamble above):
cmp eax, ecx
jne SHORT $LN7@test12
cmp esi, edi
jne SHORT $LN7@test12
cmp ebx, ebp
jne SHORT $LN7@test12
mov dl, 1
jmp SHORT $LN8@test12
$LN7@test12:
xor dl, dl
$LN8@test12:
And the following code for the other function:
cmp eax, ecx
je SHORT $LN11@test12
xor bl, bl
jmp SHORT $LN9@test12
$LN11@test12:
cmp esi, edi
je SHORT $LN10@test12
xor bl, bl
jmp SHORT $LN9@test12
$LN10@test12:
cmp ebx, ebp
sete dl
$LN9@test12:
Now just to drive the point home, I'm not going to tell you which of those two code blocks were produced by which function, because as you can see, they're quite similar.
Now, for the first snippet, starting at the cmp, the longest path of execution is 8 instructions, and the shortest path of execution is 3 instructions. For the second snippet, the longest path of execution is 6 instructions, and the shortest path is 4 instructions.
So to summarize, one approach takes 3-8 instructions, whereas the other takes 4-6 instructions.
So if this code was in a real tight loop somewhere, which approach is actually faster depends really on what you expect the outcome to be.
Never assume that you know how the compiler/bytecode interpreter is going to optimize things, and never optimize things without profiling your code to see where the real bottle necks lie.
|
|
|
|
|
Wow this information is interesting. Thx for the effort. I could be wrong but i can't see the solution with 1 statement anywhere among your tests functions. I presume that using that solution would be even faster.
|
|
|
|
|
It's all in the first block of code. test1() vs. test2().
The examples are a bit contrived, and might not look like the original question anymore. For one thing, I just assume that you magically know the result of "getWidth()", etc. Basically, I try to pit the concept of declaring boolean variables versus returning false early against each other.
|
|
|
|
|
i could be saying dumb sh*t here but would it be faster to not use the variables and simply return the whole thing?
|
|
|
|
|
It's hard for me to tell what the answer would be inside a Java interpreter. I suspect that the difference will be small.
I doubt that declaring local variables vs. not declaring will actually make a difference in C/C++. Local variables get assigned machine registers, and they may actually not require stack locations at all.
In fact, with a decent optimizing compiler, the following snippets will compile to the same code:
void fx(int x)
{
doSomethingWithX(x);
}
and
void fx(int x)
{
int descriptionForX = x;
doSomethingWithX(descriptionForX);
}
The reason is that the 'descriptionForX' variable will be coalesced so that it uses the same register as x, and will subsequently be removed by the compiler.
|
|
|
|
|
That sounds reasonable to me. Your knowledge seems impressive. Nevertheless I think it is strange to return a boolean from the result of any if structure. It just seems strange to me.
Just one remark on the variable/nonvariable matter. I don't know about C/C++ but in java when you combine boolean expressions using the && operator, automatically when 1 value is false the other expression are skipped. That's why i would assume that it has the same advantage of being able to skip part of the calculations. I would think when using the variables, that advantage would be partially lost because you'd split up the calculations, this enlarging the calculations i'd think.
This is just theory i'm not sure of this
sorry if i'm not clear i'm having trouble expressing myself in english
|
|
|
|
|
All-in-all, I agree with you. I like tidying up the code as well...multiple return statements and complicated ifs do make code less clear. IMHO, however, I'd probably move these into their own methods within a class (let's name it DisplayModeAnalyzer). I would especially do this if these same conditions are used elsewhere in the code...then it could read something like:
public boolean displayModesMatch( DisplayMode mode1,
DisplayMode mode2 )
{
DisplayModeAnalyzer analyzer = new DisplayModeAnalyzer( mode1, mode2 );
boolean result = true;
if( analyzer.dimensionsNotEqual() )
result = false;
else if( analyzer.depthNotEqual() )
result = false;
else if( analyzer.refreshRateNotEqual() )
result = false;
return result;
}
Breaking it into methods like this increases the amount of code (unless you use these conditions elsewhere) but makes the intent a bit clearer when you scan the code (similar to what you were doing). The DisplayModeAnalyzer class, coded for the above, would also be highly unit-testable.
Note, as I think others said...those tests against the constants are very important. I actually like the clarity of your code better than the original, but not the logic...it breaks a fundamental rule of refactoring...NEVER change the logic on existing code. The reason for those constant tests is so that if it's Multi-depth, then even if the depths are equal, that condition would return false. Same for the refresh rate.
Kevin
|
|
|
|
|
if( analyzer.dimensionsNotEqual() )
result = false;
look at what ur writing here...
i'm not trying to insult you, but that just seems illogical to me.
<br />
analyzer.dimensionsNotEqual()<br />
this method returns a boolean value, this enabling you to simply write:
<br />
result = analyzer.dimensionsNotEqual();<br />
this was what bothered me the most when i posted the code. The fact that someone tests for a boolean value within a if structure, and then depending on the outcome returns true or false, that seems so strange to me.
if the above explanation isn't clear enough i'll explain a bit more below why this is so strange to me.
the contents00 if(boolean){contents00}else{contents01} are executed when the boolean value is true.
when the boolean is false the contents01 are executed.
When the only command within the contents00 or contents01 is a command to put a certain value within a boolean variable, you really are testing for no reason.
simply take the boolean value within the if structure, remove the if structure and directly put that value within that variable.
I believe when you write stuf like if(boolean00){boolean immaboolean = boolean00} it is called 'False selection' not sure how people call it in english though.
Hope i'm making my point clear here 
|
|
|
|
|
Ah, I see, so your main point was simply that using boolean conditions in ifs and then returning boolean is nonsensical. That's fair enough, so long as the operations aren't expensive and/or aren't frequent. Sometimes, as others pointed out, you do need to exit early for performance (i.e. if checking refresh rate is expensive and called frequently, then you would definitely not want to do it your way). Personally, though, I was more focused on the advantages of breaking things into methods than on the ifs.
So, more on my point of breaking it into methods, but also taking into consideration your main point...methods can meet your criteria...but additionally, meet the performance criteria of many of the other posters, e.g.
public boolean displayModesMatch( DisplayMode mode1,
DisplayMode mode2 )
{
DisplayModeAnalyzer analyzer = new DisplayModeAnalyzer( mode1, mode2 );
return analyzer.dimensionsEqual() && analyzer.depthEqual()
&& analyzer.refreshRateEqual();
}
By breaking it into methods and doing it this way it's still very readable, it meets your conditions of not having ifs to surround boolean returns, and it also satisfies the point that others had about checking all the conditions at the beginning of the method can reduce performance. If the dimensions are not equal, it will return false immediately without checking depth or refresh rate...thus slightly less expensive than checking all the conditions at the beginning of the method.
Thus, this has the same fail-early/"good performance" of the original "coding horror" you posted, while at the same time having the simplicity of your solution.
Kevin
|
|
|
|
|
the code you wrote evaluates every possible scenario and then returns a value.. The original code disqualifies the rest of the code and returns rather than proceeding. What if isEqualDepth takes 3 seconds to run.. every method call would take 3 seconds even when they obviously didnt match after the first if
Running every if and returning a combined boolean makes sense for validation but the logic doesnt translate to this situation at all..
DrewG MCSD .Net
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
