Ayende @ Rahien

It's a girl

There are so many things wrong with this code…

I just want to cry:

image

Comments

justin
04/13/2009 09:03 PM by
justin

lol that's hilarious

Peter
04/13/2009 09:07 PM by
Peter

Lol thats too funny, or to put it into terms the original author would recognize.

try

{

  LearnFromArticle();

}

catch (AuthorIsAnIdiotException ex)

{

 throw new  SackableOffenceException(ex);

}

lol

Laurion Burchall
04/13/2009 09:53 PM by
Laurion Burchall

At least the exception is being rethrown! ;-)

Brian
04/13/2009 09:53 PM by
Brian

Beautiful!

Jimmy Zimmerman
04/13/2009 09:53 PM by
Jimmy Zimmerman

Is it wrong that I started laughing out loud when I read this?

OH WAIT. NO. This person's an idiot.

meisinger
04/13/2009 09:55 PM by
meisinger

first off... i am surprised that this isn't a generic function (just think of the potential boxing and unboxing performance issues)

as far as i am concerned... you can't get any deeper than this type of "deep copy"

at least the author is using the Single Responsibility principle by calling this method from the Assemble and Disassemble methods

Ayende Rahien
04/13/2009 09:59 PM by
Ayende Rahien

No, the exception is NOT being REthrown.

It is being thrown.

Seeing that is a BIG WTF sign in most cases.

Pop Catalin
04/13/2009 10:05 PM by
Pop Catalin

Oh my %$%@ god, I can't believe I saw what I saw on that article!!! And the author even has a screenshot with a unhandled application exception O_O.

Microsoft should make FxCop and StyleCop mandatory and include it in every VS version including Express, they would do a great favor to humanity, by eliminating whole classes of "stupid" coding mistakes. Not that will be enough ... but ... still ...

This is enough to make a grown man cry ...

Dmitry
04/13/2009 10:08 PM by
Dmitry

Wow! Everything is wrong with this code starting from the name and ending with exception handling.

Ayende Rahien
04/13/2009 10:09 PM by
Ayende Rahien

Pop,

The exception dialog make sense, he is showing that the concurrency strategy works.

Vince
04/13/2009 10:13 PM by
Vince

haha, the proper "rethrow" should be:

catch(Exception ex){

throw;

}

João P. Bragança
04/13/2009 10:14 PM by
João P. Bragança

This is reassuring! If this is the quality of the code base coming out of East Asia I think we're gonna have job security for quite some time.

Dmitry
04/13/2009 10:15 PM by
Dmitry

@Vince,

I would not call you example proper as well. A proper re-throw could be:

catch (SqlException)

{ throw; }

catch (Exception ex)

{

// handle ex

}

Rethrowing a general System.Exception is pointless.

Dmitry
04/13/2009 10:16 PM by
Dmitry

Something like this would be valid as well:

catch (Exception ex)

{

// handle ex

throw;

}

zihotki
04/13/2009 10:21 PM by
zihotki

Really great article!!! Every time when I'll be in a sad mood I'll read it!

Pop Catalin
04/13/2009 10:25 PM by
Pop Catalin

Pop, The exception dialog make sense, he is showing that the concurrency strategy works.

Yes, but what I was finding funny in this, is that he has catch blocks all over, without doing anything (except eating stacktrace info), and the only place where he should have cough a expected exception, (the point of the article) he didn't.

Rory Primrose
04/13/2009 10:39 PM by
Rory Primrose

LOL, that is fantastic. I haven't laughed at code that quickly in ages (it's normally crying).

So I guess it's a deep copy of a memory pointer. Maybe that's what the author intended.... :P

Igor Ostrovsky
04/13/2009 11:09 PM by
Igor Ostrovsky

@Ayende, Vince, Dmitry:

Thrown or rethrown... does it even matter? It is dead code anyways, as "return value" cannot possibly throw an exception.

Or am I missing something?

Dmitry
04/13/2009 11:13 PM by
Dmitry

@Igor,

Click the article link. He uses try/catch blocks like this in every method.

Justin Etheredge
04/13/2009 11:31 PM by
Justin Etheredge

This looks like an entry in a contest to see who can write the highest density of bad code.

Seshan
04/13/2009 11:38 PM by
Seshan

First, it seems to me as if it is Not a Copy at all - Neither a shallow Copy nor a deep copy. The method just returns the same object, as objects are passed by ref. There is no use of returning an object when it is passed by reference. There would be a warning that "Not all Code paths return value". No need of Try catch around a return. No need to enclose an Exception object in catch construct when the exception is rethrown.

Walter Poch
04/14/2009 01:51 AM by
Walter Poch

At least he could made it static LOL :)

pcm
04/14/2009 05:30 AM by
pcm

I can't believe it! There are people who actually vote 5 on this article!

Peter Morris
04/14/2009 09:24 AM by
Peter Morris

I'm sure we all write crap code sometimes. At least the guy could accept the criticism of the comment telling him why his exception "handling" is wrong rather than replying

"It was just a DEMO. I didn't intent to show HOW TO USE "TRY ... CATCH" in C#."

I'd fix it out of shame, not defend it!

Sindri
04/14/2009 09:52 AM by
Sindri

Good programmers can make mistakes, even as bad as this one. Then there are bad programmers that will always do something like this.

You can tell who's who by their response when you point it out.

Benny Thomas
04/14/2009 09:56 AM by
Benny Thomas

Things like this I've seen at lots of project I have been working with. And always the coder of the ugly code defends himself like the auther does here too.

Sometimes is good to cry.

Chris
04/14/2009 11:08 AM by
Chris

BRILLANT

Grunties
04/14/2009 11:37 AM by
Grunties

There's some things wrong with this code, yes, and it was clearly written by someone with little understanding of what's going on 'under the bonnet/hood'. But, here, there's a lot of Dunning-Kruger effect. There are some good reasons to be doing what this code is attempting, and yet no one here wants to mention any of it. Shameful and misguided elitism, IMHO.

jay vaughan
04/14/2009 11:54 AM by
jay vaughan

// tl;dr

duh, everyone knows that this is just a hook for an out-of-VM exception handler.

John Chapman
04/14/2009 12:13 PM by
John Chapman

Why did you leave out the Disassemble method? It really ties the whole thing together. I actually feel kind of bad for him, but worse for the people reading this as an expert example.

public object DeepCopy(object value)

    {

        try

        {

            return value;

        }

        catch (Exception ex)

        {

            throw ex;

        }

    }

    public object Disassemble(object value)

    {

        try

        {

            return DeepCopy(value);

        }

        catch (Exception ex)

        {

            throw ex;

        }

    }
Pop Catalin
04/14/2009 12:19 PM by
Pop Catalin

@Grunties, I don' claim to be a smart fellow or an enlightened .Net developer, but what are the reasons you are talking about?

This is not about elitism, this is about spreading bad coding practices, in a community where thousands of developers passionate about their framework and language invest huge amounts of time and dedication to write blog posts, articles and books about good coding practices as a community effort to raise the quality bar of the code .Net developers write in general.

We are all better of when bad coding isn't spread around in articles like this one.

Aslam Khan
04/14/2009 01:22 PM by
Aslam Khan

Yes, this is just bad code. Ayende has rightfully surfaced it as such. But I really don't see the value of hosing the author as a useless developer. Have you asked yourself "Why am I laughing at this - publicly, and loudly?".

I watched the tail-end of Romy and Michelle's High School Reunion on TV last night. The laugh comments here reminds me of Christine and her girlfriends ridiculing Romy and Michele at the reunion.

I think we are all capable of being better people than this.

Matt H
04/14/2009 01:36 PM by
Matt H

maybe the code was poorly ported from C++ where the object was returned by value and the exception was caught in case the copy constructor failed

Min
04/14/2009 02:09 PM by
Min

Wow...

I do not know what disturbs me more, the fact that this article was accepted by CodeProject or the fact that someone out there will use this article in production code.

I've googled some of the code in my company's codebase and have found some CodeProject articles come up.

Fabio Maulo
04/14/2009 02:58 PM by
Fabio Maulo

Probably you don't have realized that the NH's users generation are changing; this is not a minor problem and sure it is a problem.

James E. Ervin
04/14/2009 03:49 PM by
James E. Ervin

There is nothing wrong with the code, that is just it, there is nothing. It makes me feel almost zen like... There is nothing wrong... There is no spoon... There is no special ingredient... There is just nothing....

Scott R
04/14/2009 04:02 PM by
Scott R

i haven't laughed so hard in ages. thank you for brightening my day.

Tim
04/14/2009 05:42 PM by
Tim

Shame on CodeProject.com.

I'll be extra skeptical next time I see a CodeProject article from an unknown author.

Peter Morris
04/14/2009 09:38 PM by
Peter Morris

@Grunties : What good reasons are those?

@Aslam : If you are going to compare with something, try something that people have heard of :-)

Seriously though, I thought this post was a bit pathetic until I saw the author's apparent inability to accept his mistakes and fix the code in the article. Then I thought the article was worth mocking.

It's counts not only how good you are when things go right, but also how good you are when things go wrong!

If my code is shit I will be glad for the criticism as I learn from changing it.

Krylen
04/14/2009 11:37 PM by
Krylen

Wow, that's something else right there. The creator of that code should give this new fangled learning technique called "reading books" a try.

Alex
04/15/2009 03:26 AM by
Alex

I call fizzbuzz.

Leo
04/15/2009 11:44 AM by
Leo

guys.,

I am one of those junior developers who can't see what is so wrong with the code. How would it be different if one would do

Catch(Exception ex)

{ throw; }

In the case of the author he is still throwing eventually. I miss the point.

Also I am not sure what are the inherent problems with accepting a parameter as an object and then returning it? Let me go further and say I don't even understand how does that mean deep copy? Just accepting an object by reference and returning the reference back.

I would be glad for any pointers.

Ayende Rahien
04/15/2009 11:49 AM by
Ayende Rahien

throw; // keep the stack trace

throw ex; // lose the stack trace

Losing the stack trace is bad.

Putting a try catch block without doing any error handling is wrong.

DeepCopy should perform a DEEP copy, not a ref copy.

What this method does is return the same reference, so it lies about its impl.

Leo
04/15/2009 11:59 AM by
Leo

Thanks Ayende. That was quick. I am glad you restored my sanity.

Dmitry
04/15/2009 01:53 PM by
Dmitry

The method actually returns a copy of the reference. .NET uses a copy of the original reference inside a method unless the "ref" keyword is used in the parameter declaration. This is why setting the reference to null inside the method will not change the outside reference.

Dave G
04/15/2009 08:22 PM by
Dave G

Well it seems that your criticism of the author has now caused the article to be removed..

Moran
04/16/2009 01:32 PM by
Moran

I've seen much worse:

try { ... }

catch(Exception ex)

{

Response.Write(ex.Message + "

" + ex.StackTrace);

}

Green WIlliams
04/16/2009 06:58 PM by
Green WIlliams

I have an idea. Let me throw up lousy code, lay back and watch herds of women-deprived geeks laugh about it and throw up more code. Oh, no need to. It has already been done here.

Jeff Hadfield
04/16/2009 11:14 PM by
Jeff Hadfield

It's already been voted into oblivion by the community. Hooray. Thanks for reading The Code Project, though. Hope you voted it down!

Pop Catalin
04/17/2009 02:49 PM by
Pop Catalin

Well it seems that your criticism of the author has now caused the article to be removed..

It's a shame, that the criticism didn't make the author rewrite it ...

Ayende Rahien
04/21/2009 07:34 AM by
Ayende Rahien

Aslam,

Several reasons:

a) bad code need to be made public, to make sure that people _recognize it as bad code

b) this is actually fairly high on the search for a few important nhibernate concurrency searches

Sung Meister
04/27/2009 03:12 AM by
Sung Meister

It has now been deleted... I can't get myself into context any more.

Brian Henderson
05/02/2009 06:28 AM by
Brian Henderson

How to make this code throw an exception? Would like to see an unit test that demonstrates. Thx.

Comments have been closed on this topic.