Patterns are just that, patterns. They may (or may not) be the building blocks of a solution. You don't try to make your solution fit a pattern, you design your solution and then see if some patterns could further make it more modular, robust, performant, etc. Leave Java world with so much classes doing nothing. Use common sense and design your architecture using a scenario driven approach.
As for code review, whatever the metric, just stick to it. Only putting a letter over a piece of code is quite useless thought. There should be comments inside or beside the code explaining what is wrong exactly with optionally a possible solution. Having a piece of code rated as D does not mean the dev is a D. Maybe that dev is just not working on things that he knows and he is in over his head ? Writing core libraries is really hard. If no one in your team has experience for that, why not use some existing libraries like the Application Blocks or whatever else you think is good ? If you insist on reinventing the wheel, then one or more senior architects should work closely with the devs to prevent such mistakes to happen in the first place. Sébastien On 10/18/07, Abhijit Gadkari <[EMAIL PROTECTED]> wrote: > > Thanks for this excellent responses. I agree with Ben that code > consistency > is important and I am trying to enforce it in my team. Well, the reason I > posted this code is to see the dev reaction on this topic. Pattern > beginners > think that pattern is the panacea to all oops problems. This singleton > code > was written with great passion [I guess, little to much] and the idea was > great - to optimize the resource utilization. But the dev who wrote this > code was new to the patterns and could not foresee the threading / pooling > problems. > > > So this discussion was really educational for me. > > And one more thing, I personally think that grading the code as A or D is > not a good idea. We are not evaluating a person / dev but the code. What > do > you think? Well - I revised my code review comments to - Please check the > discussion on this thread and do the needful. > > > > Thanks. > > Abhijit > > > On 10/17/07, Baris Acar <[EMAIL PROTECTED]> wrote: > > > > why not use Enterprise Data Application Block? > > > > On 10/17/07, Ben Joyce <[EMAIL PROTECTED]> wrote: > > > Many thanks for the clarification, gentelmen. > > > > > > On 10/17/07, Richard Blewett <[EMAIL PROTECTED]> wrote: > > > > Dispose also returns the connection to the connection pool > > > > > > > > Regards > > > > > > > > Richard Blewett - DevelopMentor > > > > > > > > > -----Original Message----- > > > > > From: Discussion of advanced .NET topics. [mailto:ADVANCED- > > > > > [EMAIL PROTECTED] On Behalf Of Smotritsky, Alex > > > > > Sent: 16 October 2007 22:01 > > > > > To: [email protected] > > > > > Subject: Re: [ADVANCED-DOTNET] Singleton and Database Connection > > > > > challange > > > > > > > > > > The open and close methods of the connection object in > ado.netusually > > > > > take the connection from the connection pool on open and return it > > on > > > > > close so you get connection pooling for free. > > > > > > > > > > -----Original Message----- > > > > > From: Discussion of advanced .NET topics. > > > > > [mailto:[EMAIL PROTECTED] On Behalf Of Mike > > Andrews > > > > > Sent: Tuesday, October 16, 2007 2:34 PM > > > > > To: [email protected] > > > > > Subject: Re: [ADVANCED-DOTNET] Singleton and Database Connection > > > > > challange > > > > > > > > > > This is not the place for a singleton and the singleton is not > > > > > implemented > > > > > correctly since it doesn't take into account any race conditions > > that > > > > > might > > > > > occur with a multi-threaded app such as asp.net. Also you would > not > > > > > want to > > > > > dispose of the connection in the singleton since that defeats the > > > > > purpose of > > > > > the singleton. > > > > > The connection should be created and destroyed when needed and not > > kept > > > > > live > > > > > by using the singleton. > > > > > > > > > > > > > > > On 10/16/07, Abhijit Gadkari <[EMAIL PROTECTED]> wrote: > > > > > > > > > > > > We are writing an ASP.NET application. We have used singleton > for > > > > > > instantiating a database connection as explained in the > following > > > > > code > > > > > > sample. > > > > > > > > > > > > > > > > > > > > > > > > Design thought was to use only one instance of MyDb across all > the > > > > > pages > > > > > > in > > > > > > this web application. Is this a correct singleton implementation > > for > > > > > > Asp.NETweb application for managing database connection? In > fact, > > > > > this > > > > > > code got C - > > > > > > or D in our internal code review. Don't know why? Any idea on > how > > to > > > > > > improve > > > > > > this code to A level. > > > > > > > > > > > > > > > > > > > > > > > > public sealed class AppDbProvider : IDisposable > > > > > > > > > > > > { > > > > > > > > > > > > public static readonly AppDbProvider instance = new > > > > > > AppDbProvider(); > > > > > > > > > > > > > > > > > > > > > > > > private readonly MyDb _d = new MyDb(ConfigurationManager > > > > > > .ConnectionStrings["my_connection"].ConnectionString, > > > > > > > > > > > > > > ConfigurationManager.ConnectionStrings[ > > > > > > "my_connection"].ProviderName); > > > > > > > > > > > > > > > > > > > > > > > > public MyDb GetDbConnection() > > > > > > > > > > > > { > > > > > > > > > > > > if (_d != null) > > > > > > > > > > > > return _d; > > > > > > > > > > > > else > > > > > > > > > > > > throw new Exception("Problem with Database > > Connection > > > > > in > > > > > > AppDbProvider."); > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > #region IDisposable Members > > > > > > > > > > > > > > > > > > > > > > > > public void Dispose() > > > > > > > > > > > > { > > > > > > > > > > > > if (_d != null) > > > > > > > > > > > > _d.Dispose(); > > > > > > > > > > > > > > > > > > > > > > > > Dispose(); > > > > > > > > > > > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > #endregion > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > Now in code behind file, we have following code > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > public partial class CreatePage > > > > > > > > > > > > { > > > > > > > > > > > > private AmgDb _d; > > > > > > > > > > > > > > > > > > > > > > > > try > > > > > > > > > > > > { > > > > > > > > > > > > _d = AppDbProvider.instance.GetDbConnection(); > > > > > > > > > > > > } > > > > > > > > > > > > Catch (Exception exce) > > > > > > > > > > > > { > > > > > > > > > > > > Response.write(exce.message); > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > And in the end, in the page unload event we have following > cleanup > > > > > code > > > > > > > > > > > > > > > > > > > > > > > > protected void Page_Unload(object sender, EventArgs e) > > > > > > > > > > > > { > > > > > > > > > > > > if (_d != null) > > > > > > > > > > > > { > > > > > > > > > > > > _d = null; > > > > > > > > > > > > } > > > > > > > > > > > > } > > > > > > > > > > > > =================================== > > > > > > This list is hosted by DevelopMentor(r) http://www.develop.com > > > > > > > > > > > > View archives and manage your subscription(s) at > > > > > > http://discuss.develop.com > > > > > > > > > > > > > > > > =================================== > > > > > This list is hosted by DevelopMentor(r) http://www.develop.com > > > > > > > > > > View archives and manage your subscription(s) at > > > > > http://discuss.develop.com > > > > > > > > > > =================================== > > > > > This list is hosted by DevelopMentor. http://www.develop.com > > > > > > > > > > View archives and manage your subscription(s) at > > > > > http://discuss.develop.com > > > > > > > > =================================== > > > > This list is hosted by DevelopMentor(r) http://www.develop.com > > > > > > > > View archives and manage your subscription(s) at > > http://discuss.develop.com > > > > > > > > > > > > > -- > > > http://www.flickr.com/photos/benbenbenbenben > > > > > > =================================== > > > This list is hosted by DevelopMentor(r) http://www.develop.com > > > > > > View archives and manage your subscription(s) at > > http://discuss.develop.com > > > > > > > > > -- > > Devrim Baris Acar > > http://www.barisacar.com > > > > =================================== > > This list is hosted by DevelopMentor(r) http://www.develop.com > > > > View archives and manage your subscription(s) at > > http://discuss.develop.com > > > > =================================== > This list is hosted by DevelopMentor(R) http://www.develop.com > > View archives and manage your subscription(s) at > http://discuss.develop.com > -- Sébastien www.sebastienlorion.com =================================== This list is hosted by DevelopMentor® http://www.develop.com View archives and manage your subscription(s) at http://discuss.develop.com
