False selection...
-
I believe BIT_DEPTH_MULTI and REFRESH_RATE_UNKNOWN don't need to be there. they are constants. even if they should be there, it still would be a lot cleaner. I'm not saying this IS the best approach i'm just saying how i think is the cleanest solution. Even with 50 things to check it's much more readable using variables then doing some if structures :p
Both may be constants, but they do play an important role here. Your version would wrongly return false in some cases when those two constants appear. Besides that, your approach works and is neat enough. Unfortunately things are not always as simple as they are here. This can very quickly turn into a bowl of spaghetti and some compromises may have to be made. Unfortunately. Anyway, the original version still is not so bad that it's a horror.
A while ago he asked me what he should have printed on my business cards. I said 'Wizard'. I read books which nobody else understand. Then I do something which nobody understands. After that the computer does something which nobody understands. When asked, I say things about the results which nobody understand. But everybody expects miracles from me on a regular basis. Looks to me like the classical definition of a wizard.
-
I believe BIT_DEPTH_MULTI and REFRESH_RATE_UNKNOWN don't need to be there. they are constants. even if they should be there, it still would be a lot cleaner. I'm not saying this IS the best approach i'm just saying how i think is the cleanest solution. Even with 50 things to check it's much more readable using variables then doing some if structures :p
lordofawesome wrote:
I believe BIT_DEPTH_MULTI and REFRESH_RATE_UNKNOWN don't need to be there.
According to the original function comments, that is incorrect. You can't just change the premise of the code on a whim. You can make it cleaner in any way you feel you want (even though it worked), but you can't change the unit requirements after it has already been in use like that.
lordofawesome wrote:
even if they should be there, it still would be a lot cleaner.
Fine, but you must include those constants if they were already there. incorporate them into your code.
lordofawesome wrote:
Even with 50 things to check it's much more readable using variables then doing some if structures
Some would agree with you, some would disagree. Let's put it this way: if someone wrote code that works, leave it alone. Why reinvent the wheel?
Josh Davis
This is what plays in my head when I finish projects. -
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
-
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 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.
-
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.
-
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
-
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 :D
-
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 was always told to only have one return statement in a procedure, so I can see this being a horror.
-
I was always told to only have one return statement in a procedure, so I can see this being a horror.
Rules are made to be broken! Especially when you know why you are breaking them :-D (That's why C# still has a "goto" but students are told not to use it.)
Real men don't use instructions. They are only the manufacturers opinion on how to put the thing together.
-
It sure does its job, wich itself is not really complicated. So why don't you just post your more fitting version?
A while ago he asked me what he should have printed on my business cards. I said 'Wizard'. I read books which nobody else understand. Then I do something which nobody understands. After that the computer does something which nobody understands. When asked, I say things about the results which nobody understand. But everybody expects miracles from me on a regular basis. Looks to me like the classical definition of a wizard.
CDP1802 wrote:
It sure does its job, wich itself is not really complicated. So why don't you just post your more fitting version?
How about creating a function to check if any two of the three parameters are equal? Calling that function with a parameter from each display mode along with the "matches anything" constant would perform three 'equals' tests.
-
Rules are made to be broken! Especially when you know why you are breaking them :-D (That's why C# still has a "goto" but students are told not to use it.)
Real men don't use instructions. They are only the manufacturers opinion on how to put the thing together.
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.
Ideally, yes, but there are exceptions... this isn't one of them.
-
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.
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! :doh:
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...
-
Hi, One reason for the original approach is it reduces processing time, which is of primary importance for a game engine. Your proposal will require the function to process each variable before returning while the original will check the most likely areas of failure first, then return -- eliminating the need to process the further checks.
.. 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...
-
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
have you ever had time to go get a burger during a loading scene...? :laugh:
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...
-
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
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...
-
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.
-
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.
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.
-
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! :doh:
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
