False selection...
-
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 remember seeing very much the same thing in some DirectX sample. It certainly is not the greatest code ever written, but where do you see the 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.
-
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; }So what's wrong with that? I think it's the tidiest piece of code I have ever seen in
Coding Horrors.Phil
The opinions expressed in this post are not necessarily those of the author, especially if you find them impolite, inaccurate or inflammatory.
-
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 must be missing something obvious. What's your point?
-
I remember seeing very much the same thing in some DirectX sample. It certainly is not the greatest code ever written, but where do you see the 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.
The code will work of course, but it should be obvious that the if structures are redundant... simply return the combined value of all the booleans
-
The code will work of course, but it should be obvious that the if structures are redundant... simply return the combined value of all the booleans
Really? It would become quite an ugly expression and it would be much harder to read. I remember writing similar code when playing with DirectX, but I wanted to filter display modes out of a list and any parameter could also have a 'don't care' value. This would have gotten even messier, so I also went for the more readable variant.
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.
-
Really? It would become quite an ugly expression and it would be much harder to read. I remember writing similar code when playing with DirectX, but I wanted to filter display modes out of a list and any parameter could also have a 'don't care' value. This would have gotten even messier, so I also went for the more readable variant.
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.
uhum that makes sence, but i'd probably try to simplify the expression using some describing variable names.
-
uhum that makes sence, but i'd probably try to simplify the expression using some describing variable names.
When putting together a graphics engine, you first have to set up a rendering context of some kind. To do that you will have to sort out what kind of graphics capabilities the host computer has and which not. In short: You will be writing lots of such code and there will be a few errors. Then it's really nice if you don't have to pull apart more complex logical terms and just can make your correction at the right place. It's just more easy to read and to maintain. And also the day will come when you have to look at the code and don't quite remember why you once decided to do things just the way they are, much less a stranger who works with your code for the first time.
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.
-
When putting together a graphics engine, you first have to set up a rendering context of some kind. To do that you will have to sort out what kind of graphics capabilities the host computer has and which not. In short: You will be writing lots of such code and there will be a few errors. Then it's really nice if you don't have to pull apart more complex logical terms and just can make your correction at the right place. It's just more easy to read and to maintain. And also the day will come when you have to look at the code and don't quite remember why you once decided to do things just the way they are, much less a stranger who works with your code for the first time.
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 i understand what you are saying, but i don't think the above solution is a good solution to that problem.
-
I believe i understand what you are saying, but i don't think the above solution is a good solution to that problem.
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.
-
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.
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.
Nice and well, but what happened to the BIT_DEPTH_MULTI and REFRESH_RATE_UNKNOWN business? Including those two will take away some of the neatness again. And what if we don't just want to check three things? What if there are 20? Then you will have 20 variables which are used in one fat term at the end.
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'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.
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.
-
Nice and well, but what happened to the BIT_DEPTH_MULTI and REFRESH_RATE_UNKNOWN business? Including those two will take away some of the neatness again. And what if we don't just want to check three things? What if there are 20? Then you will have 20 variables which are used in one fat term at the end.
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
-
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.
Runtime behavior would be worth a look for any code executed inside the rendering loop. Display modes usually are checked beforehand, so yes, technically this would be faster, but the effect would be unnoticable.
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.
-
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.
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
-
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.
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.
-
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.