|
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
|
|
|
|
|
That is some really scary code.
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
The third one is especially sublime -- just return null no matter what.
I think the compiler should detect the proportion of your methods that are static and if it is higher than say 2%, should issue an error "You are not fit to be programming. Please step away from the computer."
|
|
|
|
|
Oh, that was great days...
Private Sub Command16_Click()
Open asc & "Read" & File2.FileName For Input As #1
Line Input #1, dat
Line Input #1, doo
Line Input #1, od
Line Input #1, temat
Close
Form2.Text1 = "Answer - " & od
Form2.Text2 = doo
Form2.Text3 = "Answer - " & temat
Form2.Text4 = "This is an answer to a message received in " & dat & "."
Form2.Show
End Sub
Private Sub Command6_Click()
Form4.Show
End Sub
Private Sub Command7_Click()
Frame1.Visible = True
Frame2.Visible = False
Frame3.Visible = False
Frame4.Visible = False
File1.Refresh
End Sub
Private Sub Command8_Click()
Frame1.Visible = False
Frame2.Visible = True
Frame3.Visible = False
Frame4.Visible = False
File2.Refresh
End Sub
Private Sub Command9_Click()
Frame1.Visible = False
Frame2.Visible = False
Frame3.Visible = True
Frame4.Visible = False
File3.Refresh
End Sub
Private Sub File1_DblClick()
If File1.ListIndex = -1 Then Exit Sub
sws = File1
Form3.Show
End Sub
Private Sub Form_Load()
On Error GoTo 1
Label5 = "Welcome, it's " & Format$(Now, "long date") & ", time" & Time$
ChDir App.Path
asc = App.Path & "\"
Label4 = asc & "user.ini"
Command7.Picture = LoadPicture(asc & "unr.ico")
Command8.Picture = LoadPicture(asc & "read.ico")
Command9.Picture = LoadPicture(asc & "deleted.ico")
Command10.Picture = LoadPicture(asc & "user.ico")
If Len(App.Path) = 3 Then asc = App.Path
Close
On Error GoTo 6
7:
Load Form8
Open asc & "InLookcon.ini" For Input As #1
Line Input #1, d1
Line Input #1, d2
Line Input #1, d3
Close #1
File1.Path = d1
If d2 = "ask" Then
If File1.ListCount = 0 Then
X = MsgBox("There are no new messages. Do you want to start InLook?", 32 + 4)
If Not X = 6 Then End
End If
ElseIf d2 = "norun" Then
End
End If
Label1 = d1
zapis = d3
On Error Resume Next
File2.Path = asc & "Read"
If Err > 0 Then
MkDir asc & "Read"
Err = 0
End If
File2.Path = asc & "Read"
Label2 = File2.Path
File3.Path = asc & "Deleted"
If Err > 0 Then
MkDir asc & "Deleted"
Err = 0
End If
File3.Path = asc & "Deleted"
Label3 = File3.Path
On Error GoTo 2
4:
Open asc & "user.ini" For Input As #1
3:
If Not EOF(1) Then
Line Input #1, a
List1.AddItem a
GoTo 3
End If
Close
Open asc & "usop.ini" For Input As #1
Line Input #1, opu
opus = opu
Close
Exit Sub
1:
MsgBox "Error. Required files are missing I suppose. Run ''Repair.exe''!", 0 + 16
End
Resume
2:
Close
Open asc & "user.ini" For Output As #1
Print #1, "Error occured: users was reset."
Close
GoTo 6
Resume
6:
Open asc & "opwp.ini" For Output As #1
Print #1, "1"
Close #1
GoTo 7
End Sub
...
Greetings - Jacek
|
|
|
|
|
I just love the error message "Required files are missing I suppose."
It let's the user know just what you think about error checking!
I take it this was a very early example of your craft?
I hope like hell this was a very early example of your craft?
Real men don't use instructions. They are only the manufacturers opinion on how to put the thing together.
|
|
|
|
|
OriginalGriff wrote: I hope like hell this was a very early example of your craft?
Yes it is a very early code. There have been a lot of OOP education later (that time I didn't even know classses and methods -- that's why everything was put in event handlers which were generated by "double clicking" in designer).
Greetings - Jacek
|
|
|
|
|
And we hope you have adopted more meaningful method naming habits. Not that "Command1_click" isn't a lovely name...
|
|
|
|
|
That time I didn't know that method/control names can be changed.
Greetings - Jacek
|
|
|
|
|
In that case, you must have been thinking "What a stupid system where everything has to be named Command[digit]_click "
|
|
|
|
|
|
looooooooooooooooooooooooooooool
Oh my god!!! Two questions come to my mind: How many times did you come and go from designer view to code view and vice versa? How many mouses did you wreck?
|
|
|
|
|
Check to see if the day defined by "year", "mon" and "day" is between dtStart and dtEnd:
if (year >= dtStart.Year && year <= dtEnd.Year &&
mon >= dtStart.Month && mon <= dtEnd.Month &&
day >= dtStart.Day && day <= dtEnd.Day)
{
ok = true;
}
else
{
ok = false;
}
He said, "Boy I'm just old and lonely,
But thank you for your concern,
Here's wishing you a Happy New Year."
I wished him one back in return.
|
|
|
|
|
Ah, a false false generator in disguise.
|
|
|
|
|
On a previous job, we had a small bug in a date validation routine... it was supposed to ensure that the date (year and month) was in the future. It checked the year, if it was greater than this year it returned true, if it was the same year it checked the month*. We encountered some data with a future year and a month of zero...
* Further details omitted for brevity.
|
|
|
|