|
I would one step further since the ternary test is not only silly, it might throw an exception all on it's own due to text being null. If you KNOW something is "0", why parse it to 0? Why is an empty string valid? Is a null string valid?
The code has other problems. Why create the file before you know whether there are any errors?
Why set totalDelays and value back to zero?
"its" is spelled "it's" in this context, but it should probably read "it was".
|
|
|
|
|
That ternary cannot throw. You're thinking of Java and its .equals nonsense. == won't throw for a null.
|
|
|
|
|
|
Joe Woodbury wrote: I would one step further since the ternary test is not only silly, it might
throw an exception all on it's own due to text being null. If you KNOW something
is "0", why parse it to 0? Why is an empty string valid? Is a null string
valid?
I dont know its going to be zero. It could be any number and it will never be null.
An empty string is valid because a previous version of this code used them. This version doesnt.
Joe Woodbury wrote: The code has other problems. Why create the file before you know whether there are any errors?
Because there are terminals that watch for changes in this file. If a parse fails then I have a partially written data file. This will cause several other terminals to report errors.
Joe Woodbury wrote: Why set totalDelays and value back to zero?
Because totalDelays is for each individual class in dataList. If it's not set back to zero then it will add up across all of the classes in the list. You're making assumptions here. Setting delays to zero is unnecessary. That was left over and can be removed.
Joe Woodbury wrote: "its" is spelled "it's" in this context, but it should probably read "it was".
Thanks for pointing that out. That grammatical error could cause the entire app to crash and burn. Good catch.
|
|
|
|
|
DanielSheets wrote: I dont know its going to be zero. It could be any number and it will never be null.
An empty string is valid because a previous version of this code used them. This version doesnt.
int.TryParse((dgc.MATL.Equals("") ? "0" : dgc.MATL), out value))
In the case of empty string, you know the result will be zero. If this isn't valid for this version, refactor the code and remove the check.
DanielSheets wrote: Because there are terminals that watch for changes in this file. If a parse fails then I have a partially written data file. This will cause several other terminals to report errors.
You miss my point. You don't need to create the file until you have added up the total count. You can still keep the rename in for the purposes you stated.
DanielSheets wrote: Because totalDelays is for each individual class in dataList. If it's not set back to zero then it will add up across all of the classes in the list. You're making assumptions here.
Huh? Oh yeah, I was right:
DanielSheets wrote: Setting delays to zero is unnecessary. That was left over and can be removed.
Fact is, you don't need the goto and were showed why. Instead of taking that and learning (as I did with the ternary and null) you became defensive. Turns out your code needs to be refactored anyway, so why not fix it right?
|
|
|
|
|
Joe Woodbury wrote: Huh? Oh yeah, I was right:
You were half right.
Joe Woodbury wrote: Fact is, you don't need the goto and were showed why. Instead of taking that and
learning (as I did with the ternary and null) you became defensive. Turns out
your code needs to be refactored anyway, so why not fix it right?
I've not gotten defensive at all? If you think I did, then where?
Personally, I like the ternary operator. I like the way it looks and I like the way it works.
I'm not a "n00b" programmer by any means. I am, however, completely self taught. I'm also in the process of taking Microsoft certified courses in C# (through the company I work for). Perhaps then I can learn to code to the standards set forth by the majority here.
And for the record... Of all of the projects that I've completed here, the code snippet I put here contained the very first use of goto that I've used in years. After thinking about it, its simple and concise.
You remind me of a gear snob. In case you're not a musician let me explain that for you.
A gear snob is, for example, a guitarist who plays an expensive Fender Strat that looks down on another guitarist that plays a Squier or an Epiphone.
You are convinced that goto's and ternary operators are examples of bad programming. So you're condescending to me because I use them.
But in the end... I don't care what you think. Your opinion means about as much to me as anybody else's here. And thats exactly what it is. An opinion.
How's that for defensive?
|
|
|
|
|
DanielSheets wrote:
You are convinced that goto's and ternary operators are examples of bad programming.
Good grief. I didn't say ternary operators were bad; I said that since they are no longer needed in your specific code, they can be removed. What I learned was that the code
text == "" ? "0" : ... does not throw an exception in C# if text is null.
My view that gotos are bad is based on years of experience of finding bugs caused by their use (even in assembly where JMPs are a given and cause all sorts of problems when you aren't careful.)
Condescension is in the eye of the beholder. In this case, you had a code review, extremely valid suggestions were made on how to improve it. You concede that much of what you are doing isn't needed, but insist on keeping it that way. That's fine, but don't go around blasting everyone for being condescending, a snob or not seeing the full picture.
DanielSheets wrote: I don't care what you think
Apparently you do else you wouldn't reply. 
|
|
|
|
|
I did have valid suggestions. And they were good suggestions.
Was this part of your code review?
Joe Woodbury wrote: "its" is spelled "it's" in this context, but it should probably read "it was".
|
|
|
|
|
Quite simple, break the foreach loop into a separate method, returning a boolean. Replace goto's with return.
Improved structure, documents itself (call it ParseDataGridList(List<DataGrid> dataList) )
Caller simply uses "if" on return value - if true { copies file } else { show message }
Fixed.
|
|
|
|
|
Eddy Vluggen wrote: Do you have an example? After years in the trade, I have still to see the first instance where it actually improves readability/maintainability.
Hi,
In C/C++ for e.g. in a switch statement where you must share some portion of code.
Although one can put that shared part inside another function, for visibility & tracking purposes, some times is better to have the code in the same screen avoiding scrolling up and down to follow the code. Believe it, is real but I cannot show company code.
I don't remember which old Pascal guru told that but sometimes a goto is preferable instead of doing a lot of work to avoid its usage and make the code difficult to read.
For the other side, as an assembly programmer... I see JMPs everywhere hehehe.
Regards,
Mauro.
|
|
|
|
|
Mauro Leggieri wrote: For the other side, as an assembly programmer... I see JMPs everywhere hehehe.
..simply because there's little alternative in assembly. Assumed we weren't talking about assembly, but higher-level languages - there are few people still working professionally with assembly.
Bastard Programmer from Hell 
If you can't read my code, try converting it here[^]
|
|
|
|
|
Days ago started to program a driver and remembered me that, altough in some places you can see try/leave/except blocks is modern code, because is programmed in plain C, "goto" is still used a lot mainly for cleanup purposes.
|
|
|
|
|
Its all relative I suppose.
|
|
|
|
|
Not without being forced to. Futzing around with an old 8-bit BASIC - ATARI. It was a shockingly poor implementation of the language.
Otherwise I can't recall using goto in any C-like language at all. ever!
Q. Hey man! have you sorted out the finite soup machine?
A. Why yes, it's celery or tomato.
|
|
|
|
|
I don't use it in high level languages where there is a structured alternative. The only time I'm tempted is the 'double break', i.e. wanting to escape from two levels of nested looping; to do this you have to create a method to do the double loop and use return, which often feels like overkill.
|
|
|
|
|
I have used it for some lazy error handling.
---------------------------------
I will never again mention that I was the poster of the One Millionth Lounge Post, nor that it was complete drivel. Dalek Dave
CCC Link[ ^]
|
|
|
|
|
Use it in .bat files all the time.
|
|
|
|
|
wizardzz wrote: in .bat files all the time.
I second that.
Common sense is admitting there is cause and effect and that you can exert some control over what you understand.
|
|
|
|
|
I've using one while I test some timeout code on a function, but I'll refactor it out before I commit it. Just nice to see the whole process at once for now (and I can make sure it works before I spend time figuring out exactly how to best cleanup before cancelling it).
|
|
|
|
|
I haven't used a "goto" since.....let's see now.....1995, in some assembly language I was writing for a graphics library. This s, if you call any flavor of JMP a "goto".
Come to think of it, that's probably around the last time I've ever used the keyword "goto" in any higher languages.
|
|
|
|
|
depends on the language. sometimes, there is no way to avoid it.
|
|
|
|
|
I have always thought goto was the equlivant of the assembly language jmp (blocking?), if it is I used one this morning. Goto is also acceptable in RTOS situations (which Windows isn't) if you want the valve to close now and not later.
Glenn
|
|
|
|
|
Real programmers (ones who code in languages like assembly, C and FORTRAN) use it all the time. Rest of us - rarely if ever.
|
|
|
|
|
Nemanja Trifunovic wrote: assembly, -C- and FORTRAN
FTFY - rarely used in C either.
(Edit - C with strikethrough looked too much like a Euro symbol)
|
|
|
|
|
Rob Grainger wrote: rarely used in C either.
Take a look at any non-trivial C code base (Linux kernel, for instance) and you'll see tons of goto statements.
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
