With regards to #4, the example given showed both braced-IFs and non-braced-IFs so I would recommend consistency over any personal preference on coding style.
*ahem* :) Ben On 10/17/07, Mark Brackett <[EMAIL PROTECTED]> wrote: > 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(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® http://www.develop.com View archives and manage your subscription(s) at http://discuss.develop.com
