Ayende @ Rahien

Hi!
My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:

ayende@ayende.com

+972 52-548-6969

, @ Q c

Posts: 18 | Comments: 72

filter by tags archive

What I am working on...

time to read 16 min | 3178 words

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

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

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

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

mike

ugly

Shane Bauer

Looks like most existing production code I've seen.

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

Looks familiar to me, LOL.

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

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

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

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

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

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

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

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

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

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

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

actually this is generated from a DSL

Sean Kearon

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

Joann

I knew that you also needed job security.

Rik Hemsley

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

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

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

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

DaveTheNinja

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

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

The kidnapping version is close to the truth.

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

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

Francois Germain

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

Joe
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

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

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

goto 80ths !!

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

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

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

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

Ray
Ray

restart:

goto restart

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

Ayende Rahien

Reshef,

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

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

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

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

"Joe forgot ... AGAIN" :D

Bryan

It's an example for a book your writing.

Jeff Tucker

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

DT
DT

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

pb
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

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

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

Yitzchok

This is called "highly maintainable" code.

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

;-)

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

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

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

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

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

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

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

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?

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

  1. RavenDB 3.0 New Stable Release - 5 hours from now
  2. Production postmortem: The industry at large - about one day from now
  3. The insidious cost of allocations - 2 days from now
  4. Buffer allocation strategies: A possible solution - 5 days from now
  5. Buffer allocation strategies: Explaining the solution - 6 days from now

And 3 more posts are pending...

There are posts all the way to Sep 11, 2015

RECENT SERIES

  1. Find the bug (5):
    20 Apr 2011 - Why do I get a Null Reference Exception?
  2. Production postmortem (10):
    01 Sep 2015 - The case of the lying configuration file
  3. What is new in RavenDB 3.5 (7):
    12 Aug 2015 - Monitoring support
  4. Career planning (6):
    24 Jul 2015 - The immortal choices aren't
View all series

RECENT COMMENTS

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats