Agreed. Code reviews without any feedback as to the specific problems is definitely no where near optimal. Code reviews should be a learning experience
Shane On 10/17/07, Sébastien Lorion <[EMAIL PROTECTED]> wrote: > > 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(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
