+972 52-548-6969
Code reviewThe 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?
And I really don't like to see SQL inside my code.
And then there is this:
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:
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:
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.
More posts in "Code review" series:
- (26 Jun 2016) The bounded queue
- (15 Apr 2011) Who Can Help Me
- (01 Apr 2011) SharpArchitecture.MultiTenant
- (28 Nov 2007) PetShop 3.0
- (28 Nov 2007) The PetShop Application
Comments
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
Interesting - I found the original English translation here:
http://web.archive.org/web/20050403223511/http://www.ejb-sig.org/docs/PetShopArchitecture.html
Would be interesting to know what FxCOP thinks of it ...
What would be a good example of an application to learn from if I wanted to learn better coding techniques?
recorded videl of your talk will be available?
recorded video of your talk will be available?
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."
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.
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.
Nagarajan,
Sorry, no.
Arild & Ko,
Because PetShop 2 was the result that I found when I searched for it.
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.
Luke,
Cuyahoga is a good application with good architecture.
Comment preview