none
Best Practice for Data Layer of Entity Framework Code RRS feed

  • Question

  • Hi, below is an example of a simple method of mine to query a user by phone number, and return that User's ID.  In my data layer, every method has a "using" declaration, a try/catch block like the below, etc.  Basically, most methods look similiar to the below.  My question is, is there a way to reduce the duplicated code, specifically the "using" block?  Is there a way to refactor the below method, or does the below method look well formatted to you folks?  Any comments/criticisms welcome.  Thank you!

     

     

     

    public static int QueryPhoneNumberOwner(string PhoneNumber)
            {
                try
                {
                    using (myDB db = new myDB())
                    {
                        // Check to see if phone exists
                        User existingUserWithPhone = new User();
    
                        existingUserWithPhone = db.Users.FirstOrDefault(c => c.PhoneNumber == PhoneNumber);
                        if (existingUserWithPhone == null)
                        {
                            // this phone does not exist
                            return -1;
                        }
                        else
                        {
                            return existingUserWithPhone.Id;
                        }
                    }
                }
                catch (Exception Exc)
                {
                    string CurrentMethodName = "ArcticDataLayer:QueryPhoneNumberOwner()";
                    InsertLogEntry(exc, CurrentMethodName);
                    return -1;
                }
            }
    

     

     


    • Edited by OceanMorning Tuesday, November 22, 2011 5:58 PM typo
    Tuesday, November 22, 2011 5:58 PM

Answers

  • Hi,

    Well.. It's not that easy to tell without knowing how your code actually works. But generally you need to have the using block to ensure the disposal of your context when you don't need it.

    However, what you could do is that stop having the function as static, and then having a member variable in your class that has the context. It is generally not advisable to have a context to be too long living due to the complexity that the context may receive if you use it in serveral CRUD operations. But if you are keeping the class simple or an object based on it short lived you can share a context within your class. You can then dispose it when your class is disposed. But I really recommend to have the using statement in your code to ensure problems with a long living context.

    A another thing you can do is to simplify the retrival of the user id with the phone number a bit by using Select(c => c.Id) in your query.

    Finally. The try...catch isn't really good to do on a general Exception, it's normally adviced in a .NET application to catch only those exceptions you are expecting and will handle that should happend and let the framework take care of the rest. An unwinding to a general exception is also very time consuming.

    If we take the two latter suggestions first your function would look something like this:

     

    public static int QueryPhoneNumberOwner(string PhoneNumber)
    {
        using (myDB db = new myDB())
        {
            return db.Users.Where(c => c.PhoneNumber == PhoneNumber).Select(c => c.Id).FirstOrDefault();
        }
    }
    
    

    NOTE: If not found it will return 0 instead of -1...

     

    If you would like to do the first one the class code could look something like this:

     

    public class MyClass : IDisposable
    {
        private bool disposed;
        private myDB db;
    
        public MyClass()
        {
            db = new myDB();
        }
    
        ~MyClass()
        {
            Dispose(false);
        }
    
        public void Dispose()
        {
            Dispose(true);
            GC.SuppressFinalize(this);
        }
    
        protected virtual void Dispose(bool disposing)
        {
            if(disposed == false)
            {
                if(disposing == true)
                {
                    //Free managed resources
                    db.Dispose();
                }
                //Free unmanaged resources
                disposed = true;
            }
        }
    
        public int QueryPhoneNumberOwner(string phoneNumber)
        {
            return db.Users.Where(c => c.PhoneNumber == phoneNumber).Select(c => c.Id).FirstOrDefault();
        }
    
        public string QueryPhoneNumberOwnerName(string phoneNumber)
        {
            return db.Users.Where(c => c.PhoneNumber == phoneNumber).Select(c => c.Name).FirstOrDefault();
        }
    }
    
    //Then when you want to call you need to new MyClass and dispose it when you don't need it anymore
    //either use using or a try...finally call
    using(MyClass dal = new MyClass())
    {
        int phoneNumberUserId = dal.QueryPhoneNumberOwner("123456");
        string name = dal.QueryPhoneNumberOwnerName("123456");
    }
    
    MyClass dal = new MyClass();
    try
    {
        int phoneNumberUserId = dal.QueryPhoneNumberOwner("123456");
        string name = dal.QueryPhoneNumberOwnerName("123456");
    }
    finally
    {
        if(dal != null)
            dal.Dispose();
    }
    

     

    Hope this helps a bit! :)


    --Rune

    If a post answers your question, please click "Mark As Answer" on that post and "Mark as Helpful" if the post helped you to a solution of your problem.

    Tuesday, November 22, 2011 6:45 PM

All replies

  • Hi,

    Well.. It's not that easy to tell without knowing how your code actually works. But generally you need to have the using block to ensure the disposal of your context when you don't need it.

    However, what you could do is that stop having the function as static, and then having a member variable in your class that has the context. It is generally not advisable to have a context to be too long living due to the complexity that the context may receive if you use it in serveral CRUD operations. But if you are keeping the class simple or an object based on it short lived you can share a context within your class. You can then dispose it when your class is disposed. But I really recommend to have the using statement in your code to ensure problems with a long living context.

    A another thing you can do is to simplify the retrival of the user id with the phone number a bit by using Select(c => c.Id) in your query.

    Finally. The try...catch isn't really good to do on a general Exception, it's normally adviced in a .NET application to catch only those exceptions you are expecting and will handle that should happend and let the framework take care of the rest. An unwinding to a general exception is also very time consuming.

    If we take the two latter suggestions first your function would look something like this:

     

    public static int QueryPhoneNumberOwner(string PhoneNumber)
    {
        using (myDB db = new myDB())
        {
            return db.Users.Where(c => c.PhoneNumber == PhoneNumber).Select(c => c.Id).FirstOrDefault();
        }
    }
    
    

    NOTE: If not found it will return 0 instead of -1...

     

    If you would like to do the first one the class code could look something like this:

     

    public class MyClass : IDisposable
    {
        private bool disposed;
        private myDB db;
    
        public MyClass()
        {
            db = new myDB();
        }
    
        ~MyClass()
        {
            Dispose(false);
        }
    
        public void Dispose()
        {
            Dispose(true);
            GC.SuppressFinalize(this);
        }
    
        protected virtual void Dispose(bool disposing)
        {
            if(disposed == false)
            {
                if(disposing == true)
                {
                    //Free managed resources
                    db.Dispose();
                }
                //Free unmanaged resources
                disposed = true;
            }
        }
    
        public int QueryPhoneNumberOwner(string phoneNumber)
        {
            return db.Users.Where(c => c.PhoneNumber == phoneNumber).Select(c => c.Id).FirstOrDefault();
        }
    
        public string QueryPhoneNumberOwnerName(string phoneNumber)
        {
            return db.Users.Where(c => c.PhoneNumber == phoneNumber).Select(c => c.Name).FirstOrDefault();
        }
    }
    
    //Then when you want to call you need to new MyClass and dispose it when you don't need it anymore
    //either use using or a try...finally call
    using(MyClass dal = new MyClass())
    {
        int phoneNumberUserId = dal.QueryPhoneNumberOwner("123456");
        string name = dal.QueryPhoneNumberOwnerName("123456");
    }
    
    MyClass dal = new MyClass();
    try
    {
        int phoneNumberUserId = dal.QueryPhoneNumberOwner("123456");
        string name = dal.QueryPhoneNumberOwnerName("123456");
    }
    finally
    {
        if(dal != null)
            dal.Dispose();
    }
    

     

    Hope this helps a bit! :)


    --Rune

    If a post answers your question, please click "Mark As Answer" on that post and "Mark as Helpful" if the post helped you to a solution of your problem.

    Tuesday, November 22, 2011 6:45 PM
  •  
    This was just on a demo project I was working on, but I use this method
    in enterprise level solutions to explicitly close the connection,
    because using a Using statement in the way you are using it when I was
    doing it your way left connection open when it blew. I use it no matter
    if I am using EF or straight-up ADO.NET with SQL Command Objects. That
    is to explicitly close the connection.
     namespace DataAccessLayer
    {
          public class Article :IArticle
          {
              private const string pcdb = "name=PublishingCompanyEntities";
               public DTOArticle GetArticle(int id)
              {
                  var dto = new DTOArticle();
                   using (var conn = new EntityConnection(pcdb))
                  using (var db = new PublishingCompanyEntities(conn))
                  {
                      try
                      {
                          var article = (from a in db.Articles.Where(a =>
                               a.ArticleID == id)select a).First();
                           dto.ArticleID = article.ArticleID;
                          dto.AuthorID = (int) article.AuthorID;
                          dto.Body = article.Body;
                          dto.Tille = article.Title;
                       }
                      finally
                      {
                          conn.Close();
                      }
                  }
                   return dto;
              }
         }
    }
     
    Tuesday, November 22, 2011 9:16 PM
  • Hi,

    I am writing to check the status of the issue on your side. Would you mind letting us know the result of the suggestions?

    If you need further assistance, please feel free to let me know. I will be more than happy to be of assistance.

    Have a nice day.


    Alan Chen[MSFT]
    MSDN Community Support | Feedback to us
    Get or Request Code Sample from Microsoft
    Please remember to mark the replies as answers if they help and unmark them if they provide no help.

    Monday, November 28, 2011 3:51 AM
    Moderator
  • Thanks Rune, the time you spent on your reply is very generous and much appreciated.  In regards to the following, if the database is down or has issues (for example, the past few weeks Microsoft has had widespread issues with SQL Azure login failures across every data center) - then the exception will bubble up to the calling method.  Isn't this undesirable?  Then your application layer (the calling method) has to catch database connection failure exceptions.  How am I thinking about this incorrectly?  Thanks again.  Matt

    public static int QueryPhoneNumberOwner(string PhoneNumber)
    {
        using (myDB db = new myDB())
        {
            return db.Users.Where(c => c.PhoneNumber == PhoneNumber).Select(c => c.Id).FirstOrDefault();
        }
    }

    Monday, November 28, 2011 2:44 PM
  • Thank you for your reply.  My understanding is that the point of the Using statement is that database connections are closed automatically, even if exceptions are thrown inside of the using statement.  For example, see this link: http://www.w3enterprises.com/articles/using.aspx

    What is going on here?  Your statement is in direct conflict with the stated definition of the Using statement, from my perspective.  What am I misunderstanding or not considering here?

    Thank you.

    Monday, November 28, 2011 2:50 PM
  • Hi Alan, I have posted some follow up replies to the above answers to receive further clarification.  Thank you for bringing this back to my attention and your continued support on this issue.  Matt
    Monday, November 28, 2011 2:51 PM
  • Hi again,

    Well.. That is a matter of taste... But the general rule is that you only handle the exceptions you need to handle, and let all other pass through... Of course if you want to handle a connection problem you should do this by catching the specific exceptions, but don't use a catch all on Exception "just in case" because of the huge performance issue this causes.

    In addition, if we take your code as an example, you don't distinguish between an exception and a data not found negative result, you just return -1 as the result of the function. So when your function fails, the caller doesn't know if it got -1 because it didn't find the result or it was a fatal error in the environment. I would prefer an exception that said "Hey, something is wrong with the environment, we may not be able to do anything further for you" if I was the caller.

     


    --Rune

    If a post answers your question, please click "Mark As Answer" on that post and "Mark as Helpful" if the post helped you to a solution of your problem.
    Monday, November 28, 2011 4:42 PM
  • No, you don't need to explicit call close, the using statement is actually a try...finally scope under the hood.

    But there are some people that does this just in case. Eg. There has been a history of badly developed components that has IDisposable, but the Dispose object doesn't do it's job (like closing a database connection).

     


    --Rune

    If a post answers your question, please click "Mark As Answer" on that post and "Mark as Helpful" if the post helped you to a solution of your problem.
    Monday, November 28, 2011 4:57 PM
  • On 11/28/2011 9:50 AM, Matt in Cambridge wrote:
    > Thank you for your reply. My understanding is that the point of the
    > Using statement is that database connections are closed automatically,
    > even if exceptions are thrown inside of the using statement. For
    >
    > What is going on here? Your statement is in direct conflict with the
    > stated definition of the Using statement, from my perspective. What am I
    > misunderstanding or not considering here?
    >
     
     
    And if you think that the using statement can't choke on doing things
    correctly with a database connection when I have seen the using
    statement go down leaving a connection open sucking up all the
    connections as the nightly process ran over and over throughout the
    night as a Windows service, then what can I say.
     
    It happens that the using statement can miss the close, just like it can
    short-circuit and miss the close in the example above with a typed client.
     
    I have seen it, been there and had it happen with straight-up ADO.NET
    and with EF A guy I worked with  thought that it couldn't happen too,
    until he started testing the using statement with EF and abort testing.
    He found out that the connection was not close using the using statement
    not at all times.
     
    If it's doing it in the link above, then give me a reason why it can't
    happen else where?
     
    Monday, November 28, 2011 5:50 PM
  • Thank you for the enlightening article.  You are certainly correct about the Using statement, and I was misinformed.

    I must say, I consider it a bug in the Framework that the Dispose method throws an exception for the Entity Framework or ADO.NET.  It defeats the stated purposed of the "Using" statement.  I wonder what Vance Morrison or Scott Guthrie would say about this.  

    Alan Chen, do you know why this is the case?

    Monday, November 28, 2011 6:06 PM
  • On 11/28/2011 1:06 PM, Matt in Cambridge wrote:
    > Thank you for the enlightening article. You are certainly correct about
    > the Using statement, and I was misinformed.
    >
    > I must say, I consider it a bug in the Framework that the Dispose method
    > throws an exception for the Entity Framework or ADO.NET. It defeats the
    > stated purposed of the "Using" statement. I wonder what Vance Morrison
    > or Scott Guthrie would say about this.
    >
    > Alan Chen, do you know why this is the case?
    >
     
    Most would land on the side that it can't happen. I have seen otherwise. :)
     
    Monday, November 28, 2011 7:02 PM
  • Hey two quick questions for you on this...

     

    1.  What if your database is down, or there is an intermittent error, etc. ?  Where are you catching that Exception?

    2.  Why don't you put your "return dto;" immediately after the line "dto.Tille = article.Title;" ?

     

    Thanks,

    Matt

    Wednesday, December 7, 2011 7:15 PM
  • On 12/7/2011 2:15 PM, Matt in Cambridge wrote:
    > Hey two quick questions for you on this...
    >
    > 1. What if your database is down, or there is an intermittent error,
    > etc. ? Where are you catching that Exception?
     In this case, I was not concerned about it. It was just a demo. But the
    BLL and DAL were behind a WCF Web service, and the Service Layer was
    consuming the Web Service is where I caught exceptions.
             public DTOArticle GetArticle(int id)
            {
                var client = new WCFServiceReference.Service1Client();
                var dto = new DTOArticle();
                 try
                {
                    dto = client.GetArticle(id);
                    client.Close();
                }
                catch (Exception ex)
                {
                    client.Abort();
                }
                 return dto;
            }
     
    But there is nothing to stop one from putting a try/catch/finally in the
    DAL or anywhere to catch the exception and log exceptions, as I am sure
    you know.
     >
    > 2. Why don't you put your "return dto;" immediately after the line
    > "dto.Tille = article.Title;" ?
     
    That's the way I like to do it. But don't hold me to it, but I think if
    you put the the return in the try, the compiler thinks that all paths do
    not lead to a return and give a compiler error.
     
    Wednesday, December 7, 2011 8:59 PM
  • On 12/7/2011 2:15 PM, Matt in Cambridge wrote:
    > Hey two quick questions for you on this...
    >
    > 1. What if your database is down, or there is an intermittent error,
    > etc. ? Where are you catching that Exception?
    >
    > 2. Why don't you put your "return dto;" immediately after the line
    > "dto.Tille = article.Title;" ?
    >
     
    Oh, one other thing, don't confuse the Service Layer as being there
    because I was using a WCF service. I always use a Service Layer in
    N-Tier or Multilayer development once I discover how to develop a
    Service Layer. The SL can be between the BLL and DAL too.
     
    Wednesday, December 7, 2011 9:14 PM