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];

            }
        }
    }
}

Print | posted on Wednesday, August 27, 2008 10:39 PM

Feedback


Gravatar

# re: What I am working on... 8/27/2008 11:08 PM 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?


Gravatar

# re: What I am working on... 8/27/2008 11:14 PM 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#.


Gravatar

# re: What I am working on... 8/27/2008 11:16 PM Aaron Erickson

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


Gravatar

# re: What I am working on... 8/27/2008 11:20 PM mike

ugly


Gravatar

# re: What I am working on... 8/27/2008 11:20 PM Shane Bauer

Looks like most existing production code I've seen.


Gravatar

# re: What I am working on... 8/27/2008 11:24 PM 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. ;)


Gravatar

# re: What I am working on... 8/27/2008 11:24 PM alberto

Looks familiar to me, LOL.


Gravatar

# re: What I am working on... 8/27/2008 11:24 PM 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!?


Gravatar

# re: What I am working on... 8/27/2008 11:25 PM 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.


Gravatar

# re: What I am working on... 8/27/2008 11:28 PM 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?


Gravatar

# re: What I am working on... 8/27/2008 11:36 PM 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.


Gravatar

# re: What I am working on... 8/27/2008 11:44 PM Steve Sheldon

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


Gravatar

# re: What I am working on... 8/27/2008 11:47 PM 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.


Gravatar

# re: What I am working on... 8/27/2008 11:51 PM Omer van Kloeten

Hehe :)

But still. Ew. :/


Gravatar

# re: What I am working on... 8/27/2008 11:53 PM 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.


Gravatar

# re: What I am working on... 8/27/2008 11:53 PM 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.


Gravatar

# re: What I am working on... 8/27/2008 11:54 PM 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.


Gravatar

# re: What I am working on... 8/28/2008 12:01 AM 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.


Gravatar

# re: What I am working on... 8/28/2008 12:19 AM 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.


Gravatar

# re: What I am working on... 8/28/2008 12:45 AM Matt

actually this is generated from a DSL


Gravatar

# re: What I am working on... 8/28/2008 1:06 AM Sean Kearon

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


Gravatar

# re: What I am working on... 8/28/2008 1:10 AM Joann

I knew that you also needed job security.


Gravatar

# re: What I am working on... 8/28/2008 1:20 AM Rik Hemsley

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


Gravatar

# re: What I am working on... 8/28/2008 1:38 AM 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 :)


Gravatar

# re: Goto 8/28/2008 2:00 AM 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.


Gravatar

# re: What I am working on... 8/28/2008 2:16 AM DaveTheNinja

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


Gravatar

# re: What I am working on... 8/28/2008 2:18 AM DaveTheNinja

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


Gravatar

# re: What I am working on... 8/28/2008 2:38 AM 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...


Gravatar

# re: What I am working on... 8/28/2008 4:10 AM Andriy Volkov

The kidnapping version is close to the truth.


Gravatar

# re: What I am working on... 8/28/2008 4:16 AM Josh Schwartzberg

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


Gravatar

# re: What I am working on... 8/28/2008 4:46 AM Conspiracy

It's some sort of viral stuff. In the next week, he'll disclose the Rhino <whatever the posted source does>.


Gravatar

# re: What I am working on... 8/28/2008 6:31 AM Francois Germain

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


Gravatar

# re: What I am working on... 8/28/2008 7:14 AM 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.


Gravatar

# re: What I am working on... 8/28/2008 9:04 AM Doron Yaacoby

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


Gravatar

# re: What I am working on... 8/28/2008 9:48 AM Mike Brown

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


Gravatar

# re: What I am working on... 8/28/2008 9:51 AM Blaise Braye

goto 80ths !!
I am now wondering why am I reading your post for a few months :(


Gravatar

# re: What I am working on... 8/28/2008 10:41 AM 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...


Gravatar

# re: What I am working on... 8/28/2008 11:37 AM 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."


Gravatar

# re: What I am working on... 8/28/2008 12:12 PM 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.


Gravatar

# re: What I am working on... 8/28/2008 12:56 PM Ray

restart:

goto restart


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


Gravatar

# re: What I am working on... 8/28/2008 2:45 PM Ayende Rahien

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


Gravatar

# re: What I am working on... 8/28/2008 3:03 PM 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


Gravatar

# re: What I am working on... 8/28/2008 4:50 PM 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!


Gravatar

# re: What I am working on... 8/28/2008 5:01 PM grega g

i just LOVE the personal touch that was put into this code
"Joe forgot ... AGAIN" :D


Gravatar

# re: What I am working on... 8/28/2008 6:52 PM Bryan

It's an example for a book your writing.


Gravatar

# re: What I am working on... 8/28/2008 10:56 PM Jeff Tucker

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


Gravatar

# re: What I am working on... 8/28/2008 11:16 PM DT

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


Gravatar

# re: What I am working on... 8/29/2008 12:26 AM 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.


Gravatar

# re: What I am working on... 8/29/2008 1:24 AM 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.


Gravatar

# re: What I am working on... 8/29/2008 1:51 AM 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.


Gravatar

# re: What I am working on... 8/29/2008 3:11 AM Yitzchok

This is called "highly maintainable" code.
And you can see all the logic right in one file like it should be.

;-)


Gravatar

# re: What I am working on... 8/29/2008 3:19 AM Ken Blood

Reading Oren being funny: Priceless!

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


Gravatar

# re: What I am working on... 8/29/2008 4:51 AM 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.


Gravatar

# re: What I am working on... 8/29/2008 7:40 AM 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? :)


Gravatar

# re: What I am working on... 8/29/2008 7:49 AM 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.


Gravatar

# re: What I am working on... 8/29/2008 9:21 AM 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? =)


Gravatar

# re: What I am working on... 8/29/2008 9:33 AM 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?


Gravatar

# re: What I am working on... 8/29/2008 2:16 PM Ayende Rahien

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


Gravatar

# re: What I am working on... 8/29/2008 9:25 PM 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.