Entity Framework code. Efficiency Suggestions

Question

I am a newbie learning entity framework ( VS2012) and writing a simple CRUD application for testing. I have created the following function for a simple insert / update. I want to know if its ok or have any flaws and can be improved?

This function will be in a class library class file and will be called from the web UI on form submission.

Here is the function :

public static bool Save(int id, string hospitalname, string hospitaladdress, int cityid,
                            string postcode, int countryid, string email, string     phone, string fax, string contactperson,
                            string otherdetails, bool isactive, DateTime createddate)
    {
        bool flag = false;
        using (var dataContext = new pacsEntities())
        {
            if (id == 0)
            {
                // insert
                var newhospital = new hospital_master();

                newhospital.hospitalname = hospitalname;
                newhospital.hospitaladdress = hospitaladdress;
                newhospital.cityid = cityid;
                newhospital.postcode = postcode;
                newhospital.countryid = countryid;
                newhospital.email = email;
                newhospital.phone = phone;
                newhospital.fax = fax;
                newhospital.contactperson = contactperson;
                newhospital.otherdetails = otherdetails;
                newhospital.isactive = isactive;
                newhospital.createddate = DateTime.Now;

                dataContext.hospital_master.AddObject(newhospital);
                dataContext.SaveChanges();
                flag = true;
            }
            else
            {
                // update
                var hospital = dataContext.hospital_master.First(c => c.hospitalid == id);
                if (hospital != null)
                {
                    hospital.hospitalname = hospitalname;
                    hospital.hospitaladdress = hospitaladdress;
                    hospital.cityid = cityid;
                    hospital.postcode = postcode;
                    hospital.countryid = countryid;
                    hospital.email = email;
                    hospital.phone = phone;
                    hospital.fax = fax;
                    hospital.contactperson = contactperson;
                    hospital.otherdetails = otherdetails;
                    hospital.isactive = isactive;

                    dataContext.SaveChanges();
                    flag = true;
                }
            }
        }
        return flag;
    }

You could have all those parameters as a class and pass/update the object — Zaki, Oct 29 '13 at 09:18

Sadek Noureddine · Answer 1 · 2013-10-29T09:44:48.667

There are multiple improvements you could do:

1- You could create a class Hospital and pass it

2- I personally think that throwing an exception is better than returning flag to determine failure, but still can return true on success

3- save context can be written once outside the if

4- .First in the update scenario can be FirstOrDefault, and check for null if you want to return flag for success/failure; or just let First fail in case you want to throw exceptions as opposed to returning true/false, but one way or the other, be consistent, you either throw exceptions or return true/false. You are using First and checking for null!

5- It is good to have your own model classes (Hospital) different than your entity classes (hospital_master), and in your higher layers you use your Hospital class and in your data layer, where you have your Save method, you convert, or Hydrate/dehydrate your objects.

6 - You could simplify creation of object by:

new hospital_master
{
    hospitalname = hospital.hospitalname,
    hospitaladdress = hospital.hospitaladdress,
   ...
};

7- You could have extension methods on your Hospital class (ToEntity) and simply call it to save:

dataContext.hospiltal_master.AddObject(hospital.ToEntity());

8- It seems the EntitySet (set of hospital_master containing instances of hospital_masters) has the same name as the entity itself. You are creating hospital_master entity, and then adding it to hospital_master: datacontext.hospital_master.AddObject... I would think there should be datacontext.hospital_masters.AddObect... plural.

Thanks a lot. Very helpful suggestions, will incorporate into it — Chris, Oct 29 '13 at 10:39
For my opinion First is more appropriate way to retrieve data, because you know that this record should be in database, if it returns null than something go wrong and here is place for exception. Read this: http://stackoverflow.com/questions/1024559/when-to-use-first-and-when-to-use-firstordefault-with-linq — Roman Bats, Oct 29 '13 at 12:26
Agreed, First throws an Exception if it doesn't return one, and FirstOrDefault returns null. In his question, he is doing First and then checking for null, which is what I wanted to point out, either let it fail fast (First) or check for null (FirstOrDefault), but not mix... — Sadek Noureddine, Oct 29 '13 at 12:55

score 0 · Answer 2 · answered Oct 29 '13 at 12:23

0

You may implement a generic repository that will be responsible for all CRUD operations.

For example: http://geekswithblogs.net/seanfao/archive/2009/12/03/136680.aspx

You will be able to use it for all entities that you have in your application.

answered Oct 29 '13 at 12:23

Roman Bats

1,775
3
30
40

Entity Framework code. Efficiency Suggestions

2 Answers2