What I am working on...
I am just going to post that, and watch what happens. I will note that this is code that I just wrote, from scratch.
public class TaxCalculator { private string conStr; private DataSet rates; public TaxCalculator(string conStr) { this.conStr = conStr; using (SqlConnection con = new SqlConnection(conStr)) { con.Open(); using (SqlCommand cmd = new SqlCommand("SELECT * FROM tblTxRtes", con)) { rates = new DataSet(); new SqlDataAdapter(cmd).Fill(rates); Log.Write("Read " + rates.Tables[0].Rows.Count + " rates from database"); if (rates.Tables[0].Rows.Count == 0) { MailMessage msg = new MailMessage("important@legacy.org", "joe@legacy.com"); msg.Subject = "NO RATES IN DATABASE!!!!!"; msg.Priority = MailPriority.High; new SmtpClient("mail.legacy.com", 9089).Send(msg); Log.Write("No rates for taxes found in " + conStr); throw new ApplicationException("No rates, Joe forgot to load the rates AGAIN!"); } } } } public bool Process(XmlDocument transaction) { try { Hashtable tx2tot = new Hashtable(); foreach (XmlNode o in transaction.FirstChild.ChildNodes) { restart: if (o.Attributes["type"].Value == "2") { Log.Write("Type two transaction processing"); decimal total = decimal.Parse(o.Attributes["tot"].Value); XmlAttribute attribute = transaction.CreateAttribute("tax"); decimal r = -1; foreach (DataRow dataRow in rates.Tables[0].Rows) { if ((string)dataRow[2] == o.SelectSingleNode("//cust-details/state").Value) { r = decimal.Parse(dataRow[2].ToString()); } } Log.Write("Rate calculated and is: " + r); o.Attributes.Append(attribute); if (r == -1) { MailMessage msg = new MailMessage("important@legacy.org", "joe@legacy.com"); msg.Subject = "NO RATES FOR " + o.SelectSingleNode("//cust-details/state").Value + " TRANSACTION !!!!ABORTED!!!!"; msg.Priority = MailPriority.High; new SmtpClient("mail.legacy.com", 9089).Send(msg); Log.Write("No rate for transaction in tranasction state"); throw new ApplicationException("No rates, Joe forgot to load the rates AGAIN!"); } tx2tot.Add(o.Attributes["id"], total * r); attribute.Value = (total * r).ToString(); } else if (o.Attributes["type"].Value == "1") { //2006-05-02 just need to do the calc decimal total = 0; foreach (XmlNode i in o.ChildNodes) { total += ProductPriceByNode(i); } try { // 2007-02-19 not so simple, TX has different rule if (o.SelectSingleNode("//cust-details/state").Value == "TX") { total *= (decimal)1.02; } } catch (NullReferenceException) { XmlElement element = transaction.CreateElement("state"); element.Value = "NJ"; o.SelectSingleNode("//cust-details").AppendChild(element); } XmlAttribute attribute = transaction.CreateAttribute("tax"); decimal r = -1; foreach (DataRow dataRow in rates.Tables[0].Rows) { if ((string)dataRow[2] == o.SelectSingleNode("//cust-details/state").Value) { r = decimal.Parse(dataRow[2].ToString()); } } if (r == -1) { MailMessage msg = new MailMessage("important@legacy.org", "joe@legacy.com"); msg.Subject = "NO RATES FOR " + o.SelectSingleNode("//cust-details/state").Value + " TRANSACTION !!!!ABORTED!!!!"; msg.Priority = MailPriority.High; new SmtpClient("mail.legacy.com", 9089).Send(msg); throw new ApplicationException("No rates, Joe forgot to load the rates AGAIN!"); } attribute.Value = (total * r).ToString(); tx2tot.Add(o.Attributes["id"], total * r); o.Attributes.Append(attribute); } else if (o.Attributes["type"].Value == "@") { o.Attributes["type"].Value = "2"; goto restart; // 2007-04-30 some bastard from northwind made a mistake and they have 3 months release cycle, so we have to // fix this because they won't until sep-07 } else { throw new Exception("UNKNOWN TX TYPE"); } } SqlConnection con2 = new SqlConnection(conStr); SqlCommand cmd2 = new SqlCommand(); cmd2.Connection = con2; con2.Open(); foreach (DictionaryEntry d in tx2tot) { cmd2.CommandText = "usp_TrackTxNew"; cmd2.Parameters.Add("cid", transaction.SelectSingleNode("//cust-details/@id").Value); cmd2.Parameters.Add("tx", d.Key); cmd2.Parameters.Add("tot", d.Value); cmd2.ExecuteNonQuery(); } con2.Close(); } catch (Exception e) { if (e.Message == "UNKNOWN TX TYPE") { return false; } throw e; } return true; } private decimal ProductPriceByNode(XmlNode item) { using (SqlConnection con = new SqlConnection(conStr)) { con.Open(); using (SqlCommand cmd = new SqlCommand("SELECT * FROM tblProducts WHERE pid=" + item.Attributes["id"], con)) { DataSet set = new DataSet(); new SqlDataAdapter(cmd).Fill(set); return (decimal)set.Tables[0].Rows[0][4]; } } } }
Comments
Funny timestamps you've got there for code you just made, unless you are into copy/paste orgies.
It looks like code that has grown hyster...err..historically. Maybe you want to post it to the daily wtf?
Why? Why are you doing this? Who's forcing you to do this?
I'm not sure I even knew that there was a "goto" in C#.
Was checking my calendar... is it April 1st in some other time zone?
ugly
Looks like most existing production code I've seen.
It always amazes me just how quickly that unsettling feeling kicks in, you know the one, the one right after you look at code that REALLY needs to be refactored. ;)
Looks familiar to me, LOL.
You're working on legacy code, probably in .NET 1.1, where you're unable to introduce new infrastructure, and which isn't under unit test!?
I'll probably hear crap for saying this, but gotos aren't pure evil. If there is a piece of code that they can simplify, then they might be a good thing. Making that choice though is a slippery slope. There are other constructs, like exceptions and breaks that do essentially the same thing.
Seems to break numerous "good" coding practices but without understanding the context within which this was built passing judgement on it is pointless.
If this was built as part of a production system and runs on a server etc etc then shame on the coder, but if this was built to spike a concept / prove a point / quickly process and identify problems in a critical environment (i.e. one off use code) then as long as it does the intended job and is acknowledged as a one-off, just right now, never use it again, i am deleting the code in a minute type app it's perfect.
Care to enlighten us on the context within which you wrote this Ayende?
A really good example of JFHCI. I assume the rest of the code in this project is great. Since this class has worked perfectly in production since february 2007, there is no need to change it. TaxCalculator is hidden in a corner and called by LegacyTaxCalculatorMess : INewFancyProcessingPipeElement. If the code causes any problem, or the hero joe@legacy.com gets promoted into management., you need to change it. Then you can easily configure in another INewFancyProcessingPipeElement implementation, and tell IT that they can shut down the old mail and data servers you have forced them to keep alive.
This would look better if it was implemented in the code behind of your UI.
Blah, like @Shane wrote it looks like most crappy production code out there. I think the term used to describe (and validate) a mess like this is "Job Security"...I loathe that term.
Hehe :)
But still. Ew. :/
aside about goto: there is one main case where i am okay with using them, and that is for exception handling code where i want to retry the the code that failed. consider:
int failures = 0;
int maxFailures = 5;
restart:
try
{
// do something that might fail in a recoverable way, such as http requests
}
catch
{
failures++;
if(failures < maxFailures)
{
}
}
There are definitely better ways of doing this in the present, but back in the dark ages (1.0 and 1.1), this was pretty useful and isn't egregious enough that I've yet felt the need to refactor it.
Can you say "Seperation of Concerns"? I don't care how legacy a system is, there's no excuse to not split this bugger up into something maintainable. If you have to, you can always make the TaxCalc a facade to the rest of the system.
Gah, so busy reading the code I ignored the code comments (dates) to simulate historical changes. The intro "I will note that this is code that I just wrote, from scratch" confused me - although I stand by the fact that context is king - I just ignored the context set by some of your previous posts.
Given that JFHCI enables developers to write "bad" code safely (paraphrased from earlier post), this would be a classic example of exactly that. Also looks like a prime candidate for extraction and encapsulation of a few rules and some common processing / error handling & notification.
Interesting tactic for understanding your readers too - show them some code and you see what / how they are thinking.
I especially like the creative use of a NullReferenceException to infer the state is "NJ". Makes perfect sense to future maintenance developers AND to all the business users.
My guess is that this is TDD taken to the extreme. A developer writes his specification for what TaxCalculator should do as unit tests, and a generator creates the full class with working code that makes these tests succeed.
Since this is a generator, it has no idea of the underlying logic for these tests, it just makes code that is guaranteed to make the specific tests pass, without consideration for readability and simplicity.
my first thought was it needs this:
http://castleproject.org/container/gettingstarted/part1/code.html
There's more but I'm not going into a refactor session via blog comments. Ayende probably sees more in there than I do anyway. also, the post doesn't even mention framework version requirements, any related constraints, or whether the coder is at liberty to hack away at it.
actually this is generated from a DSL
Man, I didn't even know C# had a goto statement!?!?!
I knew that you also needed job security.
I'm about to sleep. You're going to give me nightmares now. Thanks!
Justin, Shawn you're mistaken gotos are the spawn of the devil. They have no place in a modern language (i.e. one with functions).
//goto example rewrite in .NET 1.0:
const int maxFailures = 5;
for (int failures = 0; failures < maxFailures; failures++)
{
try {
// do something that might fail in a recoverable way, such as http requests
break; // or better return; should be in a function anywayz
} catch { /logging.../ }
}
BTW this code is just BAD. You must've felt dirty writing it, hope you washed your hands after :)
why not just start the foreach with the if clause for reassigning the type?
if (o.Attributes["type"].Value == "@")
No need for the goto. I have yet to see a goto in code where it was really needed. IMHO they just cause maintenance problems.
looks like your trying to dig someone out of a hole, and a big one too! :-)
Actually, on second passing of the above code, I believe I am beginning to get "The Fear"
I think everyone is missing something very important. This is obviously a secret message sent by Oren who has been kidnapped and is desperately trying to get help. Either that or he is following in the foot steps of Mr. Donald Belcham... I'm not quite sure yet...
The kidnapping version is close to the truth.
<sarcasm>I really like that coding style, you don't have to search thru a bunch of classes and methods to figure stuff out.. it's all in front of your face</sarcasm>
It's some sort of viral stuff. In the next week, he'll disclose the Rhino <whatever the posted source does>.
I somehow remotely thought it was possible to write "VB6 like" code in C# but never dare to. :)
You're trying to prove a point, probably of mal-practice. It's easier to show what seperation of conerns means, what single responsability is and what DI is when you start off with something bad.
I think Oren is working on an Auto-Refactor tool and this is the input to one of the tests :)
Looks like you used that expression dsl you mentioned before to take some old code (probably vb.net) and spit out C#.
goto 80ths !!
I am now wondering why am I reading your post for a few months :(
I think that Oren is going to write a sequel to "Refactoring to patterns" and he is basing his all book on this code sample :-)
Or maybe he just started working for a big company and he is trying to conform to their coding standards.
But hey, not all is bad - at least the connection string is injected to the TaxCalculator and not read directly from app.config...
Everything is a cycle. First, you want to write the perfect code, you reach there, and come back here again.
"All this happened before and will happen again."
I agree with Joe. Surely this code was written as an example of how things go bad, as the basis for some lesson.
restart:
goto restart
Geez... never hoped to see it ever again... oO
Reshef,
I actually got a comment on that being an issue with the code, not reading it from app.config
Ha ha, welcome to my world and the world of the majority of .net coders out there.
http://www.amazon.com/Working-Effectively-Legacy-Robert-Martin/dp/0131177052 this book is your friend.
enjoy
BOO has really come a long way
on a more serious note...
i would make the private members "conStr" and "rates" readonly
keep up the good work!
i just LOVE the personal touch that was put into this code
"Joe forgot ... AGAIN" :D
It's an example for a book your writing.
my eyes actually hurt from reading that. Unfortunately I've seen far worse. . .
Except for a few peculiarities this is excellent code requiring a minimum of training for new junior developers to maintain.
Obviously he's trolling, look at the comments. Trying to improve participation with something that isn't as difficult as his usual fare. Watch I can do it too:
Abortion should be illegal and anyone who thinks otherwise is retarded and ugly.
Fimally, I see a beautiful code on this blog.
One nitpick though, I think that name 'Joe' in e-mail and message should be loaded from the systemwide string constatnt.
Its an example showing poorly factored code and some common programming mistakes. Excellent fodder for refactoring with a tool, or pre-interview screening.
This is called "highly maintainable" code.
And you can see all the logic right in one file like it should be.
;-)
Reading Oren being funny: Priceless!
The one I can't figure out is DT's comment. Hope he was being sarcastic!
No Ken, DT has hit the nail on its head. I am in a situation where the client doesn't want to use NHibernate because not many developers know it !!! I wish Oren (or anyone else) is not in such a situation.
Hey, wait a second...
http://www.ayende.com/Blog/archive/2008/07/15/Why-I-will-not-code-review-your-code.aspx
"If the source isn't freely available, or if I don't find it interesting, then I would suggest contacting me for a consulting engagement, in which case I would be more than happy to go over any code base..."
Isn't this a bit of a double standard? :)
Louis,
Where is the double standard here?
I am not willing to go over uninteresting code bases for free. If you are willing to pay, that is another matter. I am not seeing the problem here.
I've ran out of all my fingers trying to count principles of good code producing that were broken in this shippet.
Do I have too few fingers? =)
No problem at all. I just thought it was funny to see, six weeks later, the opposite situation - an unsolicited code review coming back to you in the comments. Ironic, no?
Trust me, I don't need someone else to point out the may faults here
AR>I actually got a comment on that being an issue with the code, not reading it from app.config
I thought you had asked how else you could make the code more confusing?
Comment preview