0
\$\begingroup\$

I created this save method that has two conditions. One condition is checking if coordinatorsId is not 0 and another one is checking if Subject is not empty string. Is there a better way to do the two conditions?

I have come up with the below for the SaveRequest entity. I am looking for advice on ways to improve this. I am using EntityFramework version=6.4.4

public ResultStatus SaveRequest(List<int> RequestIds,int DictionaryId, string Subject, int coordinatorsId)
{
    ResultStatus resultStatus = new ResultStatus();
    using (var db = new Entities())
    {
        using (DbContextTransaction dbTran = db.Database.BeginTransaction())
        {
            try
            {

                foreach (var RequestId in RequestIds)
                {
                    var efRequest = db.Requests.Where(x => x.RequestId == RequestId).FirstOrDefault();
                    if (DictionaryId != 0)
                    {
                        efRequest.DictionaryId= DictionaryId;
                    }
                    if (Subject != ""){
                        efRequest.Subject = Subject;
                    }
                    if (coordinatorsId != 0)
                    {
                        efRequest.coordinatorsId = coordinatorsId ;
                    }
                    db.SaveChanges();
                }

                dbTran.Commit();

                resultStatus.ResultCode = Convert.ToInt32(Enums.Result.Code.Success);
                resultStatus.Message = "Successfully Saved.";
            }

            catch (Exception ex)
            {
                resultStatus.ResultCode = Convert.ToInt32(Enums.Result.Code.Error);
                resultStatus.Message = ex.ToString();
                dbTran.Rollback();
            }
        }
        return resultStatus;
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ RequestIds and DictionaryId parameters never used. What is SubmissionIds? SubmissionId never used. What is RequestId and efSubmissionRequest? / Please review your code and correct the typos! I think you have a lot of them. \$\endgroup\$ Commented Jan 10 at 19:08
  • \$\begingroup\$ Which version of EF are you using? Look at ExecuteUpdate. \$\endgroup\$ Commented Jan 10 at 19:09
  • \$\begingroup\$ Sorry about that I updated by question. \$\endgroup\$
    – Jefferson
    Commented Jan 10 at 19:37
  • \$\begingroup\$ what does coordinatorsId do? because it seems it's overwriting efRequest.Title = Subject. and where ItemDictionaryId variable came from ? is it a global property? and what does DictionaryId do ? clearly we need more explanation on the context. \$\endgroup\$
    – iSR5
    Commented Jan 10 at 19:47

2 Answers 2

2
\$\begingroup\$

Let's start with proper names. Parameters and local variables must have camelCase style, properties must have PascalCase.

When working with a database, the most important thing is to reduce the number of queries to it.
Therefore, it may make sense to do some checks at the beginning of the method:

if (requestIds.Count == 0)
    return resultStatus;

if (dictionaryId == 0 && subject == "" && coordinatorsId == 0)
    return resultStatus;

You receive one value from the database in a loop. This is very inefficient.
It is much more profitable to get a set of values at once with one query.

var requests = db.Requests.Where(x => requestIds.Contains(x.RequestId)).ToList();

foreach (var request in requests)
{
    if (dictionaryId != 0)
        request.DictionaryId = dictionaryId;

    if (subject != "")
        request.Subject = subject;

    if (coordinatorsId != 0)
        request.CoordinatorsId = coordinatorsId;
}

db.SaveChanges();
\$\endgroup\$
1
\$\begingroup\$

For string checks, instead of Subject != "", it's more robust to use string.IsNullOrEmpty(Subject) or string.IsNullOrWhiteSpace(Subject) as they cover more cases (null, empty, or whitespace strings).

Your method fetches each Request individually within a loop, which can be inefficient if there are many RequestIds. You might consider fetching all relevant Requests in one query before the loop:

var efRequests = db.Requests.Where(x => RequestIds.Contains(x.RequestId)).ToList();

Iterate over efRequests to update their properties to reduce the number of database roundtrips.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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