2
\$\begingroup\$

Scenario:

  • Check if the user exists
    • If yes: update the details
    • Else: insert the user details

public bool AddUsers(IEnumerable<UserDetail> users)
    {
        using (var dbTransaction = System.Data.Entity.Database.DefaultConnectionFactory.CreateConnection("MyCon").BeginTransaction())
        {
            using (var entities = new MyDbEntities())
            {
                try
                {
                    foreach (var user in users)
                    {
                        // LINQ under observation
                        var userId = entities.tUsers.Where(u => u.UserName == user.UserName).Select(x => x.UserUID).FirstOrDefault();

                        if (userId > 0)
                        {
                            //update

                        }
                        else
                        {
                             //insert
                        }

                    }
                    dbTransaction.Commit();
                    return true;
                }
                catch (Exception ex)
                {
                    dbTransaction.Rollback();
                    clsErrorHandling.WriteLog("ImportUsersHandler", "AddUsers", ex.Message.ToString());
                    return false;
                }
                finally
                {
                    dbTransaction.Dispose();
                }
            }
        }
    }

Please find the comment //LINQ under observation.

Is there a better way to find the user?

Because if I do this way:-

 tblUser newUser = new tblUser() { Username = user.Username } ;

If the user exists then no issue then in case not then how do I check whether I need to do insert or update.

\$\endgroup\$
6
\$\begingroup\$
using (var dbTransaction = System.Data.Entity.Database.DefaultConnectionFactory.CreateConnection("MyCon").BeginTransaction())
// ...
finally
{
    dbTransaction.Dispose();
}

You're already using the transaction in a using statement which is shorthand for try/finally, where the finally block disposes the IDisposable. So you can remove your manual finally block completely, otherwise you're calling Dispose on the transaction twice.

clsErrorHandling.WriteLog("ImportUsersHandler", "AddUsers", ex.Message.ToString());

ex.Message is already a string, no need to call ToString on it.

\$\endgroup\$
  • \$\begingroup\$ Points noted :) \$\endgroup\$ – Kgn-web Dec 30 '16 at 12:12
4
\$\begingroup\$

LINQ expression to find user

c# < 6

var user = entities.tUsers.FirstOrDefault(u => u.UserName == user.UserName);
var userId = user == null ? default(int) : user.UserUID;

c# 6

var userId = entities.tUsers
    .FirstOrDefault(u => u.UserName == user.UserName)?.UserUID;

tblUser newUser = new tblUser() { Username = user.Username };

Don't create new instance of entity. Use Entity which was found by LINQ expression.


entities.tUsers.Where(u => u.UserName == user.UserName)

It isn't good idea to find user by UserName when UserName is not unique.

Is UserName in your database primary key or unique?

\$\endgroup\$
  • \$\begingroup\$ Sorry, you are right of course. I'm writing from my memory. I updated my answer. \$\endgroup\$ – Dawid Wekwejt Dec 30 '16 at 11:17
2
\$\begingroup\$

You tagged your post with "entity framework". I'm using an older version of EF, but hopefully my comments won't be too far off the mark.

                    var userId = entities.tUsers.Where(u => u.UserName == user.UserName).Select(x => x.UserUID).FirstOrDefault();

                    if (userId > 0)
                    {
                        //update

                    }
                    else
                    {
                         //insert
                    }

The //update comment hides some rather important stuff from us. Is it the entity you are updating? If so, you presumably have to perform another query to fetch the entity itself.

How about simply fetching the entity to begin with?

var userent = entities.tUsers.FirstOrDefault(u => u.UserName == user.UserName);
if (userent == null) // then we need to insert a new one
{
    userent = entities.tUsers.CreateObject(); // or new myentity(); whatever you prefer
    entities.tUsers.AddObject(userent);
}
AutoMap.Mapper.Map(user, userent); // or assign these properties yourself

Finally, a transaction? Where is your call to entities.SaveChanges()? (I'm assuming "entities" is your EF context)

Generally speaking, EF will handle the transaction part for you and will complain loudly if one of your entities can't be saved for some reason.

So just put a entities.SaveChanges(); after the for-loop and ditch the transaction object.

Finally, someone suggested:

var userId = entities.tUsers.FirstOrDefault(u => u.UserName == user.UserName)?.UserUID;

Avoid this. This fetches the entire entity from the database (use SQL Profiler!) and then leaves you using only that single property. I.e. a select * followed by only accessing a single field. An entity can be quite complex (involving joins and interesting bits) and one should take care to .Select only relevant fields (like you did initially).

Mitigating factor: EF might cache the entity within your current context, so in this instance it probably isn't that bad. Regardless of this, my recommendation is to use Select() whenever possible. Unlike plain LINQ, a lack of .Select() will force EF to do more work than what is sometimes necessary.

\$\endgroup\$
  • \$\begingroup\$ Good Informative post. Thanks :) but one thing that is confusing me in your post but do not make a habit of avoiding .Select() as it carries an impact not normally seen with plain LINQ) so should I use .select() or not? \$\endgroup\$ – Kgn-web Jan 4 '17 at 6:00
  • \$\begingroup\$ Apologies, there is a double (or triple) negative in that sentence. I will try to correct it shortly. :) \$\endgroup\$ – 9Rune5 Jan 4 '17 at 6:04
1
\$\begingroup\$

I don't think one big transaction is good. If one fails everything gets rolled back. Consider putting each update / insert in a transaction.

If you do TSQL you use a merge statement so you don't need a transaction at all. Search on upsert.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.