False selection...
-
I'd probably go for this approach
/**
Determines if two display modes "match". Two display
modes match if they have the same resolution, bit depth,
and refresh rate. The bit depth is ignored if one of the
modes has a bit depth of DisplayMode.BIT_DEPTH_MULTI.
Likewise, the refresh rate is ignored if one of the
modes has a refresh rate of
DisplayMode.REFRESH_RATE_UNKNOWN.
*/
public boolean displayModesMatch(DisplayMode mode1,
DisplayMode mode2)
{
boolean isEqualDimention = (mode1.getWidth() == mode2.getWidth() && mode1.getHeight() == mode2.getHeight());
boolean isEqualDepth = (mode1.getBitDepth() == mode2.getBitDepth());
boolean isEqualRefreshRate = (mode1.getRefreshRate() == mode2.getRefreshRate());return (isEqualDimention && isEqualDepth && isEqualRefreshRate); }I think this is much more readable and much simpler.
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 -
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 jeWow 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.
-
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.
-
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 shit here but would it be faster to not use the variables and simply return the whole thing?
-
i could be saying dumb shit 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.
-
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
-
OriginalGriff wrote:
That's why C# still has a "goto" but students are told not to use it.
I always thought it was because the developer who stole modified the C++ compiler forgot to take that part out.
-
I think this is a code horror because all this could be done with 1 statement, this is the most performant way to do this AND with proper indenting the readability isn't compromised.
lordofawesome wrote:
I think this is a code horror because all this could be done with 1 statement, this is the most performant way to do this AND with proper indenting the readability isn't compromised.
I would like to see that one liner, that could be as quickly and easily understood as the original. Assuming that DisplayMode has only the elements that are being tested in this routine (height, width, depth, etc.), I think would be a mistake, and error prone should DisplayMode not now or in the future, contain only those elements. Flip the And/Or logic, swap != and == and remove the If's and you can do it in one return statement, but it wouldn't be as simple or quick to grasp.
Gary
-
I think this is a code horror because all this could be done with 1 statement, this is the most performant way to do this AND with proper indenting the readability isn't compromised.
/**
Determines if two display modes "match". Two display
modes match if they have the same resolution, bit depth,
and refresh rate. The bit depth is ignored if one of the
modes has a bit depth of DisplayMode.BIT_DEPTH_MULTI.
Likewise, the refresh rate is ignored if one of the
modes has a refresh rate of
DisplayMode.REFRESH_RATE_UNKNOWN.
*/
public boolean displayModesMatch(DisplayMode mode1,
DisplayMode mode2)
{
return (mode1.getWidth() == mode2.getWidth() && // Same resolution?
mode1.getHeight() == mode2.getHeight()) &&
(mode1.getBitDepth() == DisplayMode.BIT_DEPTH_MULTI || // & Same Bit Depth?
mode2.getBitDepth() == DisplayMode.BIT_DEPTH_MULTI ||
mode1.getBitDepth() == mode2.getBitDepth()) &&
(mode1.getRefreshRate() == // & Same Refresh Rate?
DisplayMode.REFRESH_RATE_UNKNOWN ||
mode2.getRefreshRate() ==
DisplayMode.REFRESH_RATE_UNKNOWN ||
mode1.getRefreshRate() == mode2.getRefreshRate());
}Better? I don't think so. Faster? Maybe.
Gary
-
I'd probably go for this approach
/**
Determines if two display modes "match". Two display
modes match if they have the same resolution, bit depth,
and refresh rate. The bit depth is ignored if one of the
modes has a bit depth of DisplayMode.BIT_DEPTH_MULTI.
Likewise, the refresh rate is ignored if one of the
modes has a refresh rate of
DisplayMode.REFRESH_RATE_UNKNOWN.
*/
public boolean displayModesMatch(DisplayMode mode1,
DisplayMode mode2)
{
boolean isEqualDimention = (mode1.getWidth() == mode2.getWidth() && mode1.getHeight() == mode2.getHeight());
boolean isEqualDepth = (mode1.getBitDepth() == mode2.getBitDepth());
boolean isEqualRefreshRate = (mode1.getRefreshRate() == mode2.getRefreshRate());return (isEqualDimention && isEqualDepth && isEqualRefreshRate); }I think this is much more readable and much simpler.
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
-
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.
analyzer.dimensionsNotEqual()this method returns a boolean value, this enabling you to simply write:result = analyzer.dimensionsNotEqual();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 contents00if(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 likeif(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 ;) -
I'd probably go for this approach
/**
Determines if two display modes "match". Two display
modes match if they have the same resolution, bit depth,
and refresh rate. The bit depth is ignored if one of the
modes has a bit depth of DisplayMode.BIT_DEPTH_MULTI.
Likewise, the refresh rate is ignored if one of the
modes has a refresh rate of
DisplayMode.REFRESH_RATE_UNKNOWN.
*/
public boolean displayModesMatch(DisplayMode mode1,
DisplayMode mode2)
{
boolean isEqualDimention = (mode1.getWidth() == mode2.getWidth() && mode1.getHeight() == mode2.getHeight());
boolean isEqualDepth = (mode1.getBitDepth() == mode2.getBitDepth());
boolean isEqualRefreshRate = (mode1.getRefreshRate() == mode2.getRefreshRate());return (isEqualDimention && isEqualDepth && isEqualRefreshRate); }I think this is much more readable and much simpler.
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
-
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
Are you talking about the variables code or the 1 statement code? When you are talking about the variables code you are right (but like 9999999... people already stated that, and that solution was but a compromise for greater readability) If you are talking about the 1 statement code you are wrong.
-
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.
analyzer.dimensionsNotEqual()this method returns a boolean value, this enabling you to simply write:result = analyzer.dimensionsNotEqual();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 contents00if(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 likeif(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
-
I'd probably go for this approach
/**
Determines if two display modes "match". Two display
modes match if they have the same resolution, bit depth,
and refresh rate. The bit depth is ignored if one of the
modes has a bit depth of DisplayMode.BIT_DEPTH_MULTI.
Likewise, the refresh rate is ignored if one of the
modes has a refresh rate of
DisplayMode.REFRESH_RATE_UNKNOWN.
*/
public boolean displayModesMatch(DisplayMode mode1,
DisplayMode mode2)
{
boolean isEqualDimention = (mode1.getWidth() == mode2.getWidth() && mode1.getHeight() == mode2.getHeight());
boolean isEqualDepth = (mode1.getBitDepth() == mode2.getBitDepth());
boolean isEqualRefreshRate = (mode1.getRefreshRate() == mode2.getRefreshRate());return (isEqualDimention && isEqualDepth && isEqualRefreshRate); }I think this is much more readable and much simpler.
lordofawesome wrote:
boolean isEqualDimention = (mode1.getWidth() == mode2.getWidth() && mode1.getHeight() == mode2.getHeight()); boolean isEqualDepth = (mode1.getBitDepth() == mode2.getBitDepth()); boolean isEqualRefreshRate = (mode1.getRefreshRate() == mode2.getRefreshRate()); return (isEqualDimention && isEqualDepth && isEqualRefreshRate);
Actually (as a firmware developer), I found the original code quite reasonable. The function was exited as quickly as possible, with as few calculations as possible. The quoted code above performs ALL the calculations before exiting the function, doing a lot of (possibly) unnecessary work.
-
lordofawesome wrote:
boolean isEqualDimention = (mode1.getWidth() == mode2.getWidth() && mode1.getHeight() == mode2.getHeight()); boolean isEqualDepth = (mode1.getBitDepth() == mode2.getBitDepth()); boolean isEqualRefreshRate = (mode1.getRefreshRate() == mode2.getRefreshRate()); return (isEqualDimention && isEqualDepth && isEqualRefreshRate);
Actually (as a firmware developer), I found the original code quite reasonable. The function was exited as quickly as possible, with as few calculations as possible. The quoted code above performs ALL the calculations before exiting the function, doing a lot of (possibly) unnecessary work.
Pls read other posts, i'm getting tired of getting the same remark over and over again :p
-
/**
Determines if two display modes "match". Two display
modes match if they have the same resolution, bit depth,
and refresh rate. The bit depth is ignored if one of the
modes has a bit depth of DisplayMode.BIT_DEPTH_MULTI.
Likewise, the refresh rate is ignored if one of the
modes has a refresh rate of
DisplayMode.REFRESH_RATE_UNKNOWN.
*/
public boolean displayModesMatch(DisplayMode mode1,
DisplayMode mode2)
{
return (mode1.getWidth() == mode2.getWidth() && // Same resolution?
mode1.getHeight() == mode2.getHeight()) &&
(mode1.getBitDepth() == DisplayMode.BIT_DEPTH_MULTI || // & Same Bit Depth?
mode2.getBitDepth() == DisplayMode.BIT_DEPTH_MULTI ||
mode1.getBitDepth() == mode2.getBitDepth()) &&
(mode1.getRefreshRate() == // & Same Refresh Rate?
DisplayMode.REFRESH_RATE_UNKNOWN ||
mode2.getRefreshRate() ==
DisplayMode.REFRESH_RATE_UNKNOWN ||
mode1.getRefreshRate() == mode2.getRefreshRate());
}Better? I don't think so. Faster? Maybe.
Gary
How about this one?
return
mode1.getBitDepth() != DisplayMode.BIT_DEPTH_MULTI && // False if BIT_DEPTH_MULTI
mode2.getBitDepth() != DisplayMode.BIT_DEPTH_MULTI &&
mode1.getRefreshRate() != DisplayMode.REFRESH_RATE_UNKNOWN && // False if REFRESH_RATE_UNKNOWN
mode2.getRefreshRate() != DisplayMode.REFRESH_RATE_UNKNOWN &&
mode1.getBitDepth() == mode2.getBitDepth() &&
mode1.getRefreshRate() == mode2.getRefreshRate();I think this should be better and faster. See you
-
This is a piece of code i took from a book i read about java game development.
/**
Determines if two display modes "match". Two display
modes match if they have the same resolution, bit depth,
and refresh rate. The bit depth is ignored if one of the
modes has a bit depth of DisplayMode.BIT_DEPTH_MULTI.
Likewise, the refresh rate is ignored if one of the
modes has a refresh rate of
DisplayMode.REFRESH_RATE_UNKNOWN.
*/
public boolean displayModesMatch(DisplayMode mode1,
DisplayMode mode2)
{
if (mode1.getWidth() != mode2.getWidth() ||
mode1.getHeight() != mode2.getHeight())
{
return false;
}if (mode1.getBitDepth() != DisplayMode.BIT\_DEPTH\_MULTI && mode2.getBitDepth() != DisplayMode.BIT\_DEPTH\_MULTI && mode1.getBitDepth() != mode2.getBitDepth()) { return false; } if (mode1.getRefreshRate() != DisplayMode.REFRESH\_RATE\_UNKNOWN && mode2.getRefreshRate() != DisplayMode.REFRESH\_RATE\_UNKNOWN && mode1.getRefreshRate() != mode2.getRefreshRate()) { return false; } return true; }I agree with you. There are cleaner ways to do this. However, none will make your book have more pages!
public boolean displayModesMatch(DisplayMode mode1, DisplayMode mode2)
{
return
mode1.getWidth() == mode2.getWidth() &&
mode1.getHeight() == mode2.getHeight() &&mode1.getBitDepth() == mode2.getBitDepth() && mode1.getBitDepth() != DisplayMode.BIT\_DEPTH\_MULTI && mode1.getRefreshRate() == mode2.getRefreshRate() && mode1.getRefreshRate() != DisplayMode.REFRESH\_RATE\_UNKNOWN;}
-
I was always told to only have one return statement in a procedure, so I can see this being a horror.
aspdotnetdev wrote:
I was always told to only have one return statement in a procedure, so I can see this being a horror.
I spent time last century programming in Pascal (amongst many, many other languages) and one of the "rules" which I stubbornly held onto for many years afterwards was to only have a single return from a method... BUT I finally woke up to myself and (Thank You Martin Fowler!) realised that exiting early (using what I believe are called "guard clauses" by people who know) can really improve readability and performance. Although I do wish that (currently) Visual Studio would highlight every "return" because I still sometimes only notice the last one in a method on first read. In my OPINION (and without knowing much about the actual subject domain) I prefer the first posted "horror" example to anything I've seen posted since. That is just subjective opinion, but it is based on coding for a long time and working with many other developers with different levels of experience. In fact it's probably mostly based on being embarrassed when returning to my own code years later and not having a clue what I was trying to do... Thanks to LordOfAwesome for starting this thread and prompting me to post here for the first time (I think... My memory really is getting worse with age). I just also wanted to reply to one of your later comments:
lordofawesome wrote:
Nevertheless I think it is strange to return a boolean from the result of any if structure. It just seems strange to me.
I agree with you that if there was a single "if" statement which returned a boolean, that would be silly, but in this case each of the "if" blocks might either return false AND exit the method, or fall through to the next statement. And it is that "falling through" that is very difficult to show clearly using in-line boolean operations, even with (or perhaps especially because of) the use of short-circuit boolean operators (like &&, etc.) So in the case we're discussing, my preference would be to write more code to be absolutely clear about what I was trying to do (and let's remember that the example was from a text book, so clarity is especially important), exit early to only execute the minimum required and if the method did end up being a performance bottleneck, go back and optimise it later, which would be much easier to do based on clear existing logic.
-
How about this one?
return
mode1.getBitDepth() != DisplayMode.BIT_DEPTH_MULTI && // False if BIT_DEPTH_MULTI
mode2.getBitDepth() != DisplayMode.BIT_DEPTH_MULTI &&
mode1.getRefreshRate() != DisplayMode.REFRESH_RATE_UNKNOWN && // False if REFRESH_RATE_UNKNOWN
mode2.getRefreshRate() != DisplayMode.REFRESH_RATE_UNKNOWN &&
mode1.getBitDepth() == mode2.getBitDepth() &&
mode1.getRefreshRate() == mode2.getRefreshRate();I think this should be better and faster. See you
_Erik_ wrote:
How about this one?
return
mode1.getBitDepth() != DisplayMode.BIT_DEPTH_MULTI && // False if BIT_DEPTH_MULTI
mode2.getBitDepth() != DisplayMode.BIT_DEPTH_MULTI &&
mode1.getRefreshRate() != DisplayMode.REFRESH_RATE_UNKNOWN && // False if REFRESH_RATE_UNKNOWN
mode2.getRefreshRate() != DisplayMode.REFRESH_RATE_UNKNOWN &&
mode1.getBitDepth() == mode2.getBitDepth() &&
mode1.getRefreshRate() == mode2.getRefreshRate();I think this should be better and faster.
Faster. Absolutely NOT BETTER. Only faster because you skip half the elements to test X| . You need to check all the conditions in the original code, including height and width :-O . Also your test is just plain wrong. The original...
if (mode1.getBitDepth() != DisplayMode.BIT\_DEPTH\_MULTI && mode2.getBitDepth() != DisplayMode.BIT\_DEPTH\_MULTI && mode1.getBitDepth() != mode2.getBitDepth()) { return false; }Assume the case of mode1.getBitDepth() equals DisplayMode.BIT_DEPTH_MULTI. The original code *might* return TRUE in this condition, depending on subsequent testing of RefreshRate. Your code always returns FALSE under this condition. :rose:
Gary