Ayende @ Rahien

It's a girl

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

Frank Quednau
08/27/2008 08:08 PM by
Frank Quednau

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?

Jeremy D. Miller
08/27/2008 08:14 PM by
Jeremy D. Miller

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#.

Aaron Erickson
08/27/2008 08:16 PM by
Aaron Erickson

Was checking my calendar... is it April 1st in some other time zone?

mike
08/27/2008 08:20 PM by
mike

ugly

Shane Bauer
08/27/2008 08:20 PM by
Shane Bauer

Looks like most existing production code I've seen.

Jeremy Gray
08/27/2008 08:24 PM by
Jeremy Gray

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. ;)

alberto
08/27/2008 08:24 PM by
alberto

Looks familiar to me, LOL.

Tobin Harris
08/27/2008 08:24 PM by
Tobin Harris

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!?

Justin Etheredge
08/27/2008 08:25 PM by
Justin Etheredge

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.

Neal Blomfield
08/27/2008 08:28 PM by
Neal Blomfield

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?

Hallgrim
08/27/2008 08:36 PM by
Hallgrim

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.

Steve Sheldon
08/27/2008 08:44 PM by
Steve Sheldon

This would look better if it was implemented in the code behind of your UI.

Throwspoop
08/27/2008 08:47 PM by
Throwspoop

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.

shawn
08/27/2008 08:53 PM by
shawn

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)

{

goto restart;

}

}

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.

Lucas Goodwin
08/27/2008 08:53 PM by
Lucas Goodwin

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.

Neal Blomfield
08/27/2008 08:54 PM by
Neal Blomfield

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.

Christos Karras
08/27/2008 09:01 PM by
Christos Karras

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.

josh
08/27/2008 09:19 PM by
josh

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.

Matt
08/27/2008 09:45 PM by
Matt

actually this is generated from a DSL

Sean Kearon
08/27/2008 10:06 PM by
Sean Kearon

Man, I didn't even know C# had a goto statement!?!?!

Joann
08/27/2008 10:10 PM by
Joann

I knew that you also needed job security.

Rik Hemsley
08/27/2008 10:20 PM by
Rik Hemsley

I'm about to sleep. You're going to give me nightmares now. Thanks!

Demis
08/27/2008 10:38 PM by
Demis

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 :)

Brian
08/27/2008 11:00 PM by
Brian

why not just start the foreach with the if clause for reassigning the type?

if (o.Attributes["type"].Value == "@")

            {

                o.Attributes["type"].Value = "2";

                // 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

            }

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.

DaveTheNinja
08/27/2008 11:16 PM by
DaveTheNinja

looks like your trying to dig someone out of a hole, and a big one too! :-)

DaveTheNinja
08/27/2008 11:18 PM by
DaveTheNinja

Actually, on second passing of the above code, I believe I am beginning to get "The Fear"

Shane Courtrille
08/27/2008 11:38 PM by
Shane Courtrille

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...

Andriy Volkov
08/28/2008 01:10 AM by
Andriy Volkov

The kidnapping version is close to the truth.

Josh Schwartzberg
08/28/2008 01:16 AM by
Josh Schwartzberg

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

Conspiracy
08/28/2008 01:46 AM by
Conspiracy

It's some sort of viral stuff. In the next week, he'll disclose the Rhino .

Francois Germain
08/28/2008 03:31 AM by
Francois Germain

I somehow remotely thought it was possible to write "VB6 like" code in C# but never dare to. :)

Joe
08/28/2008 04:14 AM by
Joe

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.

Doron Yaacoby
08/28/2008 06:04 AM by
Doron Yaacoby

I think Oren is working on an Auto-Refactor tool and this is the input to one of the tests :)

Mike Brown
08/28/2008 06:48 AM by
Mike Brown

Looks like you used that expression dsl you mentioned before to take some old code (probably vb.net) and spit out C#.

Blaise Braye
08/28/2008 06:51 AM by
Blaise Braye

goto 80ths !!

I am now wondering why am I reading your post for a few months :(

Reshef Mann
08/28/2008 07:41 AM by
Reshef Mann

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...

ibrahim dursun
08/28/2008 08:37 AM by
ibrahim dursun

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."

Paul Batum
08/28/2008 09:12 AM by
Paul Batum

I agree with Joe. Surely this code was written as an example of how things go bad, as the basis for some lesson.

Ray
08/28/2008 09:56 AM by
Ray

restart:

goto restart

Geez... never hoped to see it ever again... oO

Ayende Rahien
08/28/2008 11:45 AM by
Ayende Rahien

Reshef,

I actually got a comment on that being an issue with the code, not reading it from app.config

Dermot
08/28/2008 12:03 PM by
Dermot

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

meisinger
08/28/2008 01:50 PM by
meisinger

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!

grega g
08/28/2008 02:01 PM by
grega g

i just LOVE the personal touch that was put into this code

"Joe forgot ... AGAIN" :D

Bryan
08/28/2008 03:52 PM by
Bryan

It's an example for a book your writing.

Jeff Tucker
08/28/2008 07:56 PM by
Jeff Tucker

my eyes actually hurt from reading that. Unfortunately I've seen far worse. . .

DT
08/28/2008 08:16 PM by
DT

Except for a few peculiarities this is excellent code requiring a minimum of training for new junior developers to maintain.

pb
08/28/2008 09:26 PM by
pb

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.

Alex Simkin
08/28/2008 10:24 PM by
Alex Simkin

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.

Steve Campbell
08/28/2008 10:51 PM by
Steve Campbell

Its an example showing poorly factored code and some common programming mistakes. Excellent fodder for refactoring with a tool, or pre-interview screening.

Yitzchok
08/29/2008 12:11 AM by
Yitzchok

This is called "highly maintainable" code.

And you can see all the logic right in one file like it should be.

;-)

Ken Blood
08/29/2008 12:19 AM by
Ken Blood

Reading Oren being funny: Priceless!

The one I can't figure out is DT's comment. Hope he was being sarcastic!

Manoj Waikar
08/29/2008 01:51 AM by
Manoj Waikar

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.

Louis DeJardin
08/29/2008 04:40 AM by
Louis DeJardin

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? :)

Ayende Rahien
08/29/2008 04:49 AM by
Ayende Rahien

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.

meowh
08/29/2008 06:21 AM by
meowh

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? =)

Louis DeJardin
08/29/2008 06:33 AM by
Louis DeJardin

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?

Ayende Rahien
08/29/2008 11:16 AM by
Ayende Rahien

Trust me, I don't need someone else to point out the may faults here

Andriy
08/29/2008 06:25 PM by
Andriy

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?

Comments have been closed on this topic.