|
gkushner wrote:
var strChar;
I hate weak-typing. 
Greetings - Jacek
|
|
|
|
|
Not the worst code I've seen, but it's bad. This is typical code worked on by multiple developers over time. The
class this is in has over 5000 lines of code. All of it looks like this.
DataTable dtShopOrder;
if(sSNInPK.ToLower()=="yes;")
dtShopOrder = new ERP().GetShopOrderDetails(txtSerialNum.Text);
else
dtShopOrder = new Operation().GetShopOrderDetails(txtSerialNum.Text);
if(dtShopOrder == null || dtShopOrder.Rows.Count <=0)
{
this.showMessage("Cannot find the serial number. Please contact your System Administrator.", MessageType.Warning);
return;
}
txtShopOrder.Text = dtShopOrder.Rows[0]["ShopOrder"].ToString();
txtAssembly.Text = dtShopOrder.Rows[0]["Assembly"].ToString();
txtRev.Text = dtShopOrder.Rows[0]["Revision"].ToString();
txtCUCode.Text = dtShopOrder.Rows[0]["CUCODE"].ToString();
txtCustomer.Text = dtShopOrder.Rows[0]["CUNAME"].ToString();
txtQty.Text = dtShopOrder.Rows[0]["Qty"].ToString();
strSubAsmWC = "";
try
{
if(dtShopOrder.Rows[0]["SubAsmWC"] != null)
{
strSubAsmWC = dtShopOrder.Rows[0]["SubAsmWC"].ToString();
}
}
catch
{
strSubAsmWC = "";
}
iCustOpNum = -1;
iMACOpNum = -1;
iMACCount = -1;
strCustLabel = "";
strMACAddress = "";
if(dtShopOrder.Rows[0]["iCustLabelOpNum"] != null)
{
try
{
iCustOpNum = int.Parse(dtShopOrder.Rows[0]["iCustLabelOpNum"].ToString());
}
catch
{
}
}
if(dtShopOrder.Rows[0]["iMacAddressOpNum"] != null)
{
try
{
iMACOpNum = int.Parse(dtShopOrder.Rows[0]["iMacAddressOpNum"].ToString());
}
catch
{
}
}
if(dtShopOrder.Rows[0]["iMacAddressCount"] != null)
{
try
{
iMACCount = int.Parse(dtShopOrder.Rows[0]["iMacAddressCount"].ToString());
}
catch
{
}
}
if(dtShopOrder.Rows[0]["CustLabel"] != null)
strCustLabel = dtShopOrder.Rows[0]["CustLabel"].ToString();
if(dtShopOrder.Rows[0]["MacAddress"] != null)
strMACAddress = dtShopOrder.Rows[0]["MacAddress"].ToString();
Found this in there also
if(slAppConfig[AppConfig.Keys.address_defect_wc.ToString()].ToString().IndexOf(lblWCKey.Text + ";",0)>=0
|| lblWCKey.Text == this.GetCutOffWC() || lblWCKey.Text == "MRBW")
{
this.EnableDefectEdits(true);
}
else
{
this.EnableDefectEdits(false);
}
Everything makes sense in someone's mind
modified on Thursday, October 8, 2009 6:40 PM
|
|
|
|
|
Looks like you maintain the same persons code I do! 
I swear this guy has only heard of the string datatype. Everything is in strings, booleans "true", "false". And my favorite, arrays "1|12|34|56|". 
|
|
|
|
|
GibbleCH wrote: arrays "1|12|34|56|"
Depends on the language. In VB6, you do not have array initializers. Loading a list by parsing a string can be a very compact way of presenting code. Not as fast as manually setting each array element, but it does make the code shorter and still readable.
Visual Studio is an excellent GUIIDE.
|
|
|
|
|
May not be a horror, but what a waste of space
private void appendBooleanField( boolean value, boolean lastField )
{
if ( value )
{
appendField( "y", lastField );
}
else
{
appendField( "n", lastField );
}
}
Why couldn't they replace the whole function with
appendField(value ? "y" : "n", lastField);
That reduces the line count by 10 (not counting the 7 lines of comment for the function), and removes the overhead of a function call.
And, the example above is repeated over and over in the code.
SS => Qualified in Submarines
"We sleep soundly in our beds because rough men stand ready in the night to visit violence on those who would do us harm". Winston Churchill
"Real programmers can write FORTRAN in any language". Unknown
|
|
|
|
|
This looks quiet obvious to C, C++, C#, JavaScript and Java programmers, but maybe not for others.
Are you refering to some specific implementation?
Best regards,
Jaime.
|
|
|
|
|
It was in Java, but I guess I was thinking of any C derived language.
SS => Qualified in Submarines
"We sleep soundly in our beds because rough men stand ready in the night to visit violence on those who would do us harm". Winston Churchill
"Real programmers can write FORTRAN in any language". Unknown
|
|
|
|
|
Your reply appears to imply that C# code should look obvious to people who don't know C#, or even C(++), Java, JavaScript, or quite a few others that have the ternary operator. Not to mention the fact that a semi-intelligent person who does not recognize this construct in C# at least should know, if maintaining the code, that the code *is* C# and be able to find out what it is.
I am not sure what kind of programmers you think C# code should be written for, but personally I think it would be a mistake to attempt making C# read like VB6 for the sake of accomodating maintenance by someone who in essence is a non-programmer.
I'd point out that the "overhead of a function (sic!) call" is academic - at best. The compiler may well decide to inline the code. And even if it did not, there are many cases where the microseconds (if that much) spent on pushing and popping the stack would simply make no difference whatsoever to the value of the program. To me, it is still a horror though - but because it demonstrates a lack of knowledge about the language on the part of the author more than anything else.
|
|
|
|
|
I've been doing code cleanup like that for about a month. i find it everywhere.
|
|
|
|
|
No, no, you want a Dictionary<bool,string>, then use appendField( booldic [ value ] , lastField); 
|
|
|
|
|
value ? "y" : "n" personally I don't really like this structure. (unreadable)
|
|
|
|
|
Unreadable!? You may not be *used* to reading it, but don't mistake infamiliarity with anything intrinsic to the syntax. There are however some objective aspects of code: The more there is to read, the more work it is to read. The more complex the code, the more work it is. The ternary adds absolutely *no* complexity compared to a branch and is far more compact. So how can it possibly be anything but more readable?
If you know of some other alternative to the ternary, other than branching, please enlighten me.
|
|
|
|
|
don't be so upset. I just disagree with you, everybody is entitled to an opinion.
|
|
|
|
|
I'm not upset. I just found your comment ("I personally find.." with no substantiation whatsoever) stupid. I think you'll agree I'm entitled to that opinion.
|
|
|
|
|
|
I don't know why you are being flamed. I like your answer.
SS => Qualified in Submarines
"We sleep soundly in our beds because rough men stand ready in the night to visit violence on those who would do us harm". Winston Churchill
"Real programmers can write FORTRAN in any language". Unknown
|
|
|
|
|
I am familiar with the ternary. I have never liked and probably never will. I find it insufficiently readable. Also, I would like to think my opinion is not due to stupidity.
|
|
|
|
|
Are you familiar with the words obviate, relegate and denigrate? Many people aren't, but that doesn't mean that they shouldn't be used in common english language. By using words that have more meaning in them, we are able to shorten (use concision) our sentences while still conveying the same meaning.
I think the same thing applies in writing software. By using appropriate structures and controls we are able to shorten the code while still conveying the same meaning both to the compiler and the reader.
Please don't think that I am denigrating your code reading skills, I am just saying that sometimes extraneous words can be obviated.
SS => Qualified in Submarines
"We sleep soundly in our beds because rough men stand ready in the night to visit violence on those who would do us harm". Winston Churchill
"Real programmers can write FORTRAN in any language". Unknown
|
|
|
|
|
I suspose you also believe lambda expressions shouldn't be used either? They are even harder to read but shorten the amount of written code by huge degrees.
I wrote my first one today. Granted it's quite simple but its effective.
return ModifiedKeys.First(kvp => kvp.Value == id).Key;
Just to help you out, ModifiedKeys is a Dictionary<string, int="">.
This line returns the first key in my dictionary who's value matches id.
|
|
|
|
|
I can think of one reason for making it a method call. Imagine you have this all through your code, all over the place:
<br />
appendField(value ? "y" : "n", lastField)<br />
Now, someone comes along and says, 'We've just sold this to the Germans. "y" and "n" don't mean anything to them, it should be "J" and "N"'. You have to trawl through all the code, find every occurence of this (and any variations where Joe The Contractor has decided to do it his own way because that's how he's always done it in the past) and change them all to accept either Y/N or J/N depending on whether it's in German or English. The method itself could be more compact, but I would probably follow the DRY principal and put this sort of thing in a reusable method if it is being used a lot.
I have to ask: what kind of software are you writing where the overhead of a method call is such a big performance hit? I don't know your software and it's always risky to pontificate about the "right" and "wrong" way to do things when you don't know the requirements.
|
|
|
|
|
Ah, see? My Dictionary technique works with that situation too!
|
|
|
|
|
I agree with PIEBALDConsult's comment. A dictionary would work, or constants for the strings.
A function is reasonable if you just put the one line in it.
Now the question about performance hits. If performance wasn't an issue (and function calls do take extra time), why would we be on a quest for ever faster processors, and more cores. I guess I come from a era when performance was always an issue on any platform and for any processing.
We were also limited to 25 row by 80 column displays (green, amber, black and white). That's part of the reason that the wasted space bothers me so much. That one function (with the comments) would take up most of the screen. 
SS => Qualified in Submarines
"We sleep soundly in our beds because rough men stand ready in the night to visit violence on those who would do us harm". Winston Churchill
"Real programmers can write FORTRAN in any language". Unknown
|
|
|
|
|
Jim (SS) wrote: A function is reasonable if you just put the one line in it.
Yes, that would make maintenance easier, and I would hope that the compiler would inline it.
Jim (SS) wrote: 25 row by 80 column displays
Your display had 25 rows? Lucky dog. I was stuck with 24, but I could select 132 columns.
|
|
|
|
|
PIEBALDconsult wrote: I was stuck with 24, but I could select 132 columns
Youngster.
Of course I forgot about the 1 line by 80 display I had before that (punchcard).
SS => Qualified in Submarines
"We sleep soundly in our beds because rough men stand ready in the night to visit violence on those who would do us harm". Winston Churchill
"Real programmers can write FORTRAN in any language". Unknown
|
|
|
|
|
Luxury. All we had was a single red light and a toggle switch.
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
