1
\$\begingroup\$

I have the following method that I am trying to optimize. Currently, the method works great, however, it takes a little over 3 hours to complete. I feel like, using some nifty LINQ joins, it should be able to run in minutes. Here is the method:

    public async Task UpdateEmailTable(IEnumerable<StudentEmail> students)
    {
        Console.WriteLine("Updating Email table");
        var studentEmails = students.ToList();
        int totalStudents = studentEmails.Count;
        int currentCount = 0;
        float lastPercentage = -1;

        foreach (var student in studentEmails)
        {
            var isIn = await _context.BarcDemoGraphicEmail.AnyAsync(z => z.Sridentifier == student.StudentId);

            if (!isIn)
            {
                var ar = await _context.Barcaccount.FirstOrDefaultAsync(z => z.SrIdentifier == student.StudentId);

                await _context.BarcDemoGraphicEmail.AddAsync(new BarcDemoGraphicEmail
                {
                    Sridentifier = student.StudentId,
                    Email = student.EmailAddress,
                    Updatedate = DateTime.Now,
                    Aridentifier = ar == null ? "" : ar.ArIdentifier
                });
            }
            else
            {
                var rec = await _context.BarcDemoGraphicEmail.Where(z => z.Sridentifier == student.StudentId)
                    .FirstAsync();
                rec.Email = student.EmailAddress;
                rec.Updatedate = DateTime.Now;
            }

            await _context.SaveChangesAsync();

            int percentage = (int)Math.Round((float)currentCount / totalStudents * 100);
            if (lastPercentage != percentage) Console.Write("\r" + percentage + "%");
            lastPercentage = percentage;
            currentCount++;
        }
    }

So this method is called with a list of student ids and emails. This list does not come from the database. This list comes from a web call and must be passed into the method. It then iterates through the list, one at a time. It takes the student's id and checks to see if it exists in the BarcDemoGraphicEmail table, there by setting the isIn flag.

If the student ID is NOT in the BarcDemoGraphicEmail table, it pulls the account information from another table called BarcAccount. It then inserts this student's email into the BarcDemoGraphicEmail using the information from the BarcAccount table and the email from the students current iteration.

If the student ID IS in the BarcDemoGraphicEmail table, it simply pulls that record and updates the UpdateDate field and the Email field from the current iteration.

As I said earlier, this method currently works fine, however it takes way too long. How can I optimize my code (or possibly optimize the database) so this executes faster?

This is a console .NET Core 3.1 C# application using Entity Framework Core 3.1. The database is a SQL Server database.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ I'm guessing it's slow because it is one.at.a.time. Take a look at the System.Data.SqlClient namespace. You can handle your data as if it were in tables. But there is no C# code that will be faster than the DB itself. We had a 4+ hour process rewritten. As stored procedure it took about 12 seconds to complete. \$\endgroup\$ Commented Dec 14, 2020 at 19:51
  • \$\begingroup\$ Your problem cannot be solved on a pure Entity Framework Core. You either need to use some other ORM, like LINQ to DB, or extensions to EF, such as linq2db.EntityFrameworkCore. But this is beyond the scope of Code Review. I think you should ask a question on the main stackoverflow site. \$\endgroup\$ Commented Dec 15, 2020 at 4:51
  • 3
    \$\begingroup\$ To be able to answer this well it's necessary to see the classes, StudentEmail, BarcDemoGraphicEmail, Barcaccount. Also, how many items will students contain, typically? There seems to be much room for improvement but that's hard to tell now. \$\endgroup\$ Commented Dec 18, 2020 at 17:44

2 Answers 2

Reset to default
2
\$\begingroup\$

Some quick remarks:

  • DemoGraphic isn't a compound word so there shouldn't be a capital letter in the middle. However, Barcaccount is so it should be called BarcAccount. Same for Sridentifier, Updatedate, Aridentifier.

  • Use meaningful variable names. I have no idea what ar is, or sr.

  • UpdateEmailTable(IEnumerable<StudentEmail> students) and then var studentEmails = students.ToList(); Use properly named and typed parameters. Your method requires a List<StudentEmail> studentEmails (or perhaps ICollection<StudentEmail> studentEmails), write it like that and make the calling code to pass the correct argument.

  • This method is peppered with Console.WriteLine. This means it mixes business logic and UI. Bad idea. At least change this to use a logging system, and then configure that so it outputs to the command line.

You say this takes three hours, which suggests either a massive volume of data or particularly slow code (or perhaps your tables are not correctly indexed?). Perhaps implementing logic that uses SqlBulkInsert could solve the performance issue. Perhaps take the incoming data and store it in a table, and then apply one or two SQL queries to update the data in BarcDemoGraphicEmail.

\$\endgroup\$
0
\$\begingroup\$

You query the same BarcDemoGraphicEmail table twice in the same loop iteration.

You can improve the query as follows

var rec = await _context.BarcDemoGraphicEmail
    .FirstOrDefaultAsync(z => z.Sridentifier == student.StudentId);

if (rec == null)
{
    var ar = ...
    ...
}
else
{
    rec.Email = student.EmailAddress;
    rec.Updatedate = DateTime.Now;
}

Also you should add .ConfigureAwait(false) to all asynchronous methods.

\$\endgroup\$

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.