views:

379

answers:

7

I've seen this is various codebases, and wanted to know if this generally frowned upon or not.

For example:

public class MyClass
{
   public int Id;

   public MyClass()
   {
      Id = new Database().GetIdFor(typeof(MyClass));
   }
}
+1  A: 

The only problem I can think of with this approach is that any errors from the DB initialization will be propagated as exceptions from the constructor.

Franci Penov
+8  A: 

Well.. I wouldn't. But then again my approach usually involves the class NOT being responsible for retrieving it's own data.

Chris Lively
+1  A: 

Yea, you CAN do it, but it's not the best design, and error handling in constructors isn't as tidy as elsewhere.

stu
+4  A: 

It will also make it difficult to write unit tests for the class as you won't be able to force the class to use a Mock/Stub version of the db class. See here: http://en.wikipedia.org/wiki/Dependency_injection

Mark Roddy
+7  A: 

You can use the disposable pattern if you refer to a DB connection:

 public class MyClass : IDisposable
  { Database db

    public MyClass()  { db = new Database();}

    public int Id 
     { get { if (_id == null) _id = db.GetIdFor(typeof(MyClass)); 
             return _id.Value;     
           }
     } int? _id;

    IDisposable.Dispose() { db.Close(); } 
 }

using (var x = new MyClass()) 
 { /* ... */
 } //closes DB
Mark Cidade
+1 to the answer.What I don't like is the yet another non-standard awkward coding style that makes the everyday's programmer life a little bit harder.:)
Trap
+3  A: 

There are several reasons this is not generally considered good design some of which like causing difficult unit testing and difficulty of handling errors have already been mentioned.

The main reason I would choose not to do so is that your object and the data access layer are now very tightly coupled which means that any use of that object outside of it original design requires significant rework. As an example what if you came across an instance where you needed to use that object without any values assigned for instance to persist a new instance of that class? you now either have to overload the constructor and then make sure all of your other logic handles this new case, or inherit and override.

If the object and the data access were decoupled then you could create an instance and then not hydrate it. Or if your have a different project that uses the same entities but uses a different persistence layer then the objects are reusable.

Having said that I have taken the easier path of coupling in projects in the past :)

A: 

Why would anyone want to use a mock object/stub instead of the real thing? Would you agree that car manufacturers should use paperboard models for crashtests?