Assuming you actually *want* a singleton (see the numerous other replies for the reasons that may not be the case), I still see a couple of issues with your implementation.
1. The null check in GetDbConnection is worthless. If _d is null, then you failed to initialize it. If you failed to initialize it, then the _d = new MyDb() line threw a TypeInitializationException - which would propagate to the instance = new AppDbProvider() line - which would be thrown to whoever accesses the object. 2. You should have a private AppDbProvider() constructor to avoid others creating an instance (by default, you'll get a no-args public ctor) 3. I'm not sure of the point of implementing IDisposable, since you won't be disposed of. That said, Dispose() sure looks like a StackOverflowException to me....Also, a 2nd Dispose call may or may not throw an exception since you're calling _d.Dispose() again. You should keep a _disposed = true around to detect redundant calls. Again, the null check is worthless. 4. I despise the no braces on the if statements - your internal guidelines may vary, however. 5. You open yourself up to NullReferenceExceptions in the _d = new MyDb() line if "my_connection" isn't defined. This will lead to the same TypeInitializationException I mentioned in 1. Assuming that MyDb is thread-safe, I don't spot any threading issues. The _d = null line the page strikes me as superfluous, though. The page will be GC'ed in due course and there's no concern about it keeping the singleton alive. In order of likelihood of code review problems by a team who decided that you should have singleton db connection, I'd guess #2, #3 and #5 are the ones they've spotted. ;) --MB > -----Original Message----- > From: Discussion of advanced .NET topics. [mailto:ADVANCED- > [EMAIL PROTECTED] On Behalf Of Abhijit Gadkari > Sent: Tuesday, October 16, 2007 2:49 PM > To: [email protected] > Subject: [ADVANCED-DOTNET] Singleton implementation of database > connection in ASP.NET application > > 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.NET web application for managing database connection? > > //Code Reference > > public sealed class AppDbProvider : IDisposable > { > public static readonly AppDbProvider instance = new > AppDbProvider > (); > > private readonly MyDb _d = new MyDb > (ConfigurationManager.ConnectionStrings["my_connection"].ConnectionStri > ng, > 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 the 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 page unload event, we have following cleanup code > > protected void Page_Unload(object sender, EventArgs e) > { > if (_d != null) > { > _d = null; > } > } > > Thanks. > > Abhi > > =================================== > 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
