|
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.
|
|
|
|
|
I must be missing something obvious. What's your point?
|
|
|
|
|
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.
|
|
|
|
|
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
|
|
|
|
|
public boolean displayModesMatch(DisplayMode mode1,
DisplayMode mode2)
{
return (mode1.getWidth() == mode2.getWidth() &&
mode1.getHeight() == mode2.getHeight()) &&
(mode1.getBitDepth() == DisplayMode.BIT_DEPTH_MULTI ||
mode2.getBitDepth() == DisplayMode.BIT_DEPTH_MULTI ||
mode1.getBitDepth() == mode2.getBitDepth()) &&
(mode1.getRefreshRate() ==
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 &&
mode2.getBitDepth() != DisplayMode.BIT_DEPTH_MULTI &&
mode1.getRefreshRate() != DisplayMode.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 &&
mode2.getBitDepth() != DisplayMode.BIT_DEPTH_MULTI &&
mode1.getRefreshRate() != DisplayMode.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  .
You need to check all the conditions in the original code, including height and width  .
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 | [Rose]](https://codeproject.global.ssl.fastly.net/script/Forums/Images/rose.gif)
Gary
|
|
|
|
|
Yes, you're right. I think I misunderstood the initial post. I thought it should return false if one of the modes was BIT_DEPTH_MULT or REFRESH_RATE_UNKNOWN. Instead, what I understand now is that bit depth matches if both are equal or one of them is BIT_DEPTH_MULTI, and refresh rate matches if both are equal or one of them is REFRESH_RATE_UNKNOWN. Am I right? I am sure you agree that looking at the original code does not help too much to figure out what it has to do, and that is exactly what we all are talking about here:
return mode1.getWidth() == mode2.getWidth() && mode1.getHeight() == mode2.getHeight() &&
(mode1.getBitDepth() == mode2.getBitDepth() ||
mode1.getBitDepth() == DisplayMode.BIT_DEPTH_MULTI ||
mode2.getBitDepth() == DisplayMode.BIT_DEPTH_MULTI)
&&
(mode1.getRefreshRate() == mode2.getRefreshRate() ||
mode1.getRefreshRate() == DisplayMode.REFRESH_RATE_UNKNOWN ||
mode2.getRefreshRate() == DisplayMode.REFRESH_RATE_UNKNOWN);
I think this would be much more readable, even without any comment explaining what it has to do.
Edit
That said, I think it would be still better to split this into three methods (sizeMatch, bitDepthMatch, refreshRateMatch), so displayModesMatch would still become much easier to read.
return sizeMatch(mode1, mode2) && bitDepthMatch(mode1, mode2) && refreshRateMatch(mode1, mode2);
You see, there are many ways to get the same result, and I think the given example here in the first post is a coding horror becouse it is one of the worst approaches.
See you.
modified on Monday, October 18, 2010 9:41 AM
|
|
|
|
|
Erik. A 2 vote? REALLY? Thanks buddy. I see you don't handle criticism very well.
The original code was very straight forward. Some developers don't like multiple returns. That does not make it a "horror".
Likely you did not read the comments in the code or analyze the code properly. I made this mistake upon first inspection, assuming I knew what it was doing, but did not.
Now, your latest example: Perform a routine, which performs 3 routines - that has to be the slowest rewrite that was presented in this thread. Loading and clearing the register stack 4 times. Wow! Too much overhead. Yes, clear, but doesn't belong in time-sensitive logic (if this is time-sensitive).
But I have to wonder how the bitdepth and refreshrate code would be written. You haven't gotten rid of the nuance, just move it and added some overhead. You'd have better comments?
In short, your final logic does the same as the original but is slower.
Gary
|
|
|
|
|
The 2 vote was another mistake of mine, sorry for that. I wanted to go to page 2 to see the original post, clicked the vote 2 instead, and forgot to make the correct vote to your post. I've just corrected it with the vote it really deserves, a 5.
I know my final logic does the same as the original but is slower. It is obvious. And it is much clearer, and this is also obvious. The sample I give just before does the same also, and is faster. Both of them improve the original in readability, and the first one also improves it in speed. If you had to manage a team of developers, some of them newcomers with very little experience, tell me, having the original code and the two samples I am giving, which one would you choose?
I would choose the last one, though I know it is slower, but I also know that the overhead is just negligible in this case, becouse this operation will not be used over and over again within de context of a long and complex batch process.
I don't mind to have several "return" in the same method... if it improves readability and/or performance. In the case presented here, it does not improve readablity neither performance, so that is the reason why I consider it a horror. Nobody is obligated to agree, I am just sharing my point of view.
Edit
Sorry again, I was forgetting the las part. The implementation for bitdepth and refreshrate is given just before, but like you wonder how they should be, I will give some extra clues:
return mode1.getWidth() == mode2.getWidth() && mode1.getHeight() == mode2.getHeight() &&
(mode1.getBitDepth() == mode2.getBitDepth() ||
mode1.getBitDepth() == DisplayMode.BIT_DEPTH_MULTI ||
mode2.getBitDepth() == DisplayMode.BIT_DEPTH_MULTI)
&&
(mode1.getRefreshRate() == mode2.getRefreshRate() ||
mode1.getRefreshRate() == DisplayMode.REFRESH_RATE_UNKNOWN ||
mode2.getRefreshRate() == DisplayMode.REFRESH_RATE_UNKNOWN);
Thank you
modified on Tuesday, October 19, 2010 9:28 AM
|
|
|
|
|
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 
(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.
|
|
|
|
|
Naw, it's because the if/then/else indenting looks pretty ugly after about 15 else if's.
Do a GoTo after every 7 else if's and things look nice and elegant.
Gary
|
|
|
|
|
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.
Thanks
AB
|
|
|
|
|
FSANB wrote: 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.)
Um, use ELSE to replace the fall-through. Would that make it more acceptable?
I think nesting if's with the else's would be more unreadable, but that is me.
Funny, we could all use the same coding "standards" but write this little routine so differently. That's what makes us arteests. 
Gary
|
|
|
|
|
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 found this code recently:
Public Sub Round100(ByRef amount As Long)
If (Math.IEEERemainder(CDbl(amount), CDbl(100)) <> 0) Then
amount = CLng(System.Math.Floor(amount / 100))
amount += 1
amount *= 100
End If
End Sub
0) Misleading name (it doesn't round)
1) Ought to be a function
2) Needless conversions (three of them)
3) Performs the divide twice
4) Should use Ceiling... or the \ operator ( )
"Well, it's VB; of course it's a horror!", I here you say, but according to the comments it was ported from C++:
void Round100(long &amount)
{
if (fmod((double)amount, (double)100) != 0)
{
amount/=100;
amount++;
amount*=100;
}
}
|
|
|
|
|
|
The C++ version is easier to understand at the first view :P
But yes, both are the same horror
Horror code is one of the most amusing things in the world if you see it, but the biggest hell if you have to work with it.
|
|
|
|
|
Are those pesky nulls bothering you? No problem - here's the Clever solution:
public class CleverConvert
{
public static object IsNull(object value, object default_value)
{
if (value == null) {
return default_value;
}
return value;
}
public static object IsDbNullOrNull(object value, object default_value)
{
if (value == null | object.ReferenceEquals(value, DBNull.Value)) {
return default_value;
}
return value;
}
public static object IsDBNull(object value, object default_value)
{
return null;
}
public static object IsDBNull(object value, object default_value, object else_value)
{
if (object.ReferenceEquals(value, DBNull.Value)) {
return default_value;
}
return else_value;
}
public static string FormatDbNullDate(object value, string format)
{
string result = "";
if ((!object.ReferenceEquals(value, DBNull.Value)) & (value != null)) {
result = ((DateTime)value).ToString(format);
}
return result;
}
public static object IsEmptyString(string value, object default_value)
{
return IsEmptyString(value, default_value, value);
}
public static object IsEmptyString(string value, object default_value, object else_value)
{
if (value == "") {
return default_value;
}
return else_value;
}
}
The class name and comments (actually a commented line) are the way I found them - I haven't changed anything.
|
|
|
|
|
Is this supposed to be c#?
I suppose
var valueOrDefault = value ?? defaultValue; was just too complicated?
|
|
|
|
|
Oh yes, that's C# I forgot to mention it
|
|
|
|
|
Coming from a C++ background, I constantly have to remind myself about the ?? operator. One of these days, I will type it before ReSharper reminds me that my ?: expression can be converted into an equivalent ?? expression. 
--
Kein Mitleid Für Die Mehrheit
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
