Ayende @ Rahien

Refunds available at head office

Code review: The PetShop Application

I gave a talk about ReSharper today, and I used the PetShop demo app as the base code. I have purposefully avoided looking at the source code of the sample until today, because I wanted to get a gueniue experience, rather than a rehearsed one. I don't think it went as well as it could have, but that is not the point of this post. The point is to talk about just the code quality of the PetShop application.

First, let us see what the PetShop application is:

The Microsoft .NET Pet Shop 2.0 illustrates basic and advanced coding concepts within an overall architectural framework

The Pet Shop example is supposed to illustrate "coding concepts", but mostly it demonstrate those that you want to avoid. I was shocked by what I was seeing there.

I am going to touch just the account class, because if has enough issues all on its own. The account class is supposed to be a domain entity, but what do you see when you open it?

image

And I really don't like to see SQL inside my code.

And then there is this:

image

And suddenly I am confused, because I see a class that is doing the work of three classes, and that is just by casual browsing.

And then we had this:

image

Somehow, public fields and violating the .NET naming conventions doesn't really strikes me as a good idea.

Code duplication, like between the Account.Insert() and Account.Update(), both have significant duplication in the form of:

image 

I thought that the whole idea of a sample app was to show of best practices, and that is... quite not so, in this case.

Comments

jdn
11/28/2007 07:33 AM by
jdn

Yeah, this is an oldie but a goodie. I remember reading about this a number of years ago.

The originally translated paper seems to be missing, but here are links to the original in French and the Google translation (which seems remarkably good) of something written in 2002.

http://www.dotnetguru.org/articles/Reflexion/PetShopArchitecture/PetShopArchitecture.htm

http://72.14.203.104/translate_c?hl=en&sl=fr&u=http://www.dotnetguru.org/articles/Reflexion/PetShopArchitecture/PetShopArchitecture.htm&prev=/search%3Fq%3DPet%2BShop%2BArchitecture%2BAnti%2Bpatterns%26hl%3Den%26sa%3DG%26pwst%3D1

Richard
11/28/2007 07:58 AM by
Richard

Interesting - I found the original English translation here:

http://web.archive.org/web/20050403223511/http://www.ejb-sig.org/docs/PetShopArchitecture.html

Casey
11/28/2007 07:58 AM by
Casey

Would be interesting to know what FxCOP thinks of it ...

Luke
11/28/2007 08:26 AM by
Luke

What would be a good example of an application to learn from if I wanted to learn better coding techniques?

Nagarajan
11/28/2007 08:59 AM by
Nagarajan

recorded videl of your talk will be available?

Nagarajan
11/28/2007 09:00 AM by
Nagarajan

recorded video of your talk will be available?

Arild
11/28/2007 09:30 AM by
Arild

Shouldn't you have been reviewing Petshop 3.0, though?

From http://msdn2.microsoft.com/en-US/library/ms954623.aspx: "Version 3.x of the .NET Pet Shop addresses the valuable feedback given by reviewers of the .NET Pet Shop 2.0 and was created to ensure that the application is aligned with the architecture guideline documents being produced by the Microsoft."

Ko
11/28/2007 09:52 AM by
Ko

PetShop 2.0 is in deed a little out of date. Could have used the Global Bank sample packaged with WCSF / WCAB as this is essentially what P&P are touting as best practices right now. Alternatively, DinnerNow.NET would have also been an up to date example.

John Chapman
11/28/2007 12:56 PM by
John Chapman

Was the point of the .NET Pet Shop really to show a good architecture? I always thought it was about showing how .NET is faster than Java. Wasn't the Pet Shop originally a Java demo after all? I thought the goal was to put together charts that showed the .NET bar far beyond the Java bar, and as such the architecture was second fiddle to performance numbers.

As far as sample architectures go, I thought that was the initial role of IBuySpy.

Ayende Rahien
11/28/2007 01:12 PM by
Ayende Rahien

Arild & Ko,

Because PetShop 2 was the result that I found when I searched for it.

Ayende Rahien
11/28/2007 01:13 PM by
Ayende Rahien

John Chapman,

No, that was PetShop 1.0, where MS was accused of cheating, and they re-wrote it to make it match the Java impl better.

Ayende Rahien
11/28/2007 01:19 PM by
Ayende Rahien

Luke,

Cuyahoga is a good application with good architecture.

Comments have been closed on this topic.