1
\$\begingroup\$

I'm not much familiar with Task Api programming in C#. Can anyone review my following code?

The following is a ElapsedEventHandler of a System.Timers.Timer. In this event, I'm getting pending notifications from database and sending them. After I send the notification, I update that notification in database by setting HasSent = true. We require our notifications to be sent as soon as possible. This is the reason, I'm updating HAsSent in a separate thread by using Task Api. I would like to know if this is a bad practise? Should I use this? If not then why?

protected void ProcessNotifications(object source, ElapsedEventArgs e)
{
    UtilityMethods.StopTimer(_notificationTimer);

    try
    {
        var notificationService = new NotificationService();
        var pendingNotifications = notificationService.GetPendingNotifications();

        if (pendingNotifications.Count > 0 && LoadConfigurations())
        {
            foreach (var notification in pendingNotifications)
            {
                try
                {
                    var notificationType = (NotificationType)notification.NotificationTypeId;

                    notification.NotificationTemplate = new NotificationTemplateService().GetById(notification.NotificationTemplateId);
                    notification.NotificationAttachments = notificationType == NotificationType.Email ?
                        new NotificationAttachmentService().GetByNotificationId(notification.NotificationId) : null;

                    using (var sender = NotificationSenderFactory.Create(notificationType))
                    {
                        if (sender.Send(notification))
                        {
                            Task.Factory.StartNew(() =>
                            {
                                notificationService.MarkNotificationsAsSent(notification.NotificationId);// A database Hit
                            });
                        }
                    }
                }
                catch (Exception ex)
                {
                    AppLogger.LogException(ex);
                }
            }
        }
    }
    catch (Exception ex)
    {
        AppLogger.LogException(ex);
    }
    finally
    {
        UtilityMethods.StartTimer(_notificationTimer);
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Can you please post the rest of the code? \$\endgroup\$ – t3chb0t Jan 31 at 11:57
0
\$\begingroup\$

Should I use this?

NO

If not then why?

Reference TaskFactory.StartNew Method

Remarks

Starting with the .NET Framework 4.5, the Task.Run method is the recommended way to launch a compute-bound task. Use the StartNew method only when you require fine-grained control for a long-running, compute-bound task. .......

Which would end up as

//...omitted for brevity

Task.Run(() => {
    notificationService.MarkNotificationsAsSent(notification.NotificationId);// A database Hit
});

//...omitted for brevity
\$\endgroup\$
  • \$\begingroup\$ Is it good to run a task in loop? Or I should hit the database without task? Would it really a good way to achieve performance or it can degrade performance? \$\endgroup\$ – Muhammad Qasim Feb 4 at 7:39

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.