In my .net core hosted service console application, I am reading a database table, retrieving some values, and based on a status, I am sending emails. It is working as it is expected, I wonder if there are any improvements to make it better. So I would be glad if you can share your comments.
Thanks in advance.
Program.cs
using System.IO;
using System.Net;
using System.Threading.Tasks;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Serilog;
using Serilog.Events;
using Serilog.Sinks.Email;
namespace SendEmail
{
class Program
{
static async Task Main(string[] args)
{
Log.Logger = new LoggerConfiguration()
.MinimumLevel.Information()
.Enrich.FromLogContext()
.WriteTo.Console()
.WriteTo.File("log-.txt", rollingInterval: RollingInterval.Day)
.WriteTo.Email(new EmailConnectionInfo
{
FromEmail = "[email protected]",
ToEmail = "[email protected]",
MailServer = "smtp.yandex.com.tr",
NetworkCredentials = new NetworkCredential
{
UserName = "[email protected]",
Password = "123456"
},
EnableSsl = false,
Port = 587,
EmailSubject = "Send Email Service Error"
}, restrictedToMinimumLevel: LogEventLevel.Error, batchPostingLimit: 1)
.CreateLogger();
var builder = new HostBuilder()
.ConfigureServices((hostContext, services) =>
{
//Serilog
services.AddLogging(loggingBuilder =>
loggingBuilder.AddSerilog(dispose: true));
services.AddSingleton<IHostedService, BusinessService>();
//Setting up app settings configuration
var config = LoadConfiguration();
services.AddSingleton(config);
});
await builder.RunConsoleAsync();
}
private static IConfiguration LoadConfiguration()
{
var builder = new ConfigurationBuilder()
.SetBasePath(Directory.GetCurrentDirectory())
.AddJsonFile("appsettings.json", optional: false, reloadOnChange: true)
.AddJsonFile(path: "serilog.json", optional: false, reloadOnChange: true);
return builder.Build();
}
}
}
BusinessService.cs
using System;
using System.Collections.Generic;
using System.Data.SqlClient;
using System.Net.Http;
using System.Net.Http.Headers;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using MailKit.Net.Smtp;
using MailKit.Security;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
using MimeKit;
namespace SendEmail
{
public class BusinessService : IHostedService
{
private readonly IHostApplicationLifetime _applicationLifetime;
private readonly IConfiguration _configuration;
private readonly ILogger _logger;
public BusinessService(IHostApplicationLifetime applicationLifetime, IConfiguration configuration, ILogger<BusinessService> logger)
{
_applicationLifetime = applicationLifetime;
_configuration = configuration;
_logger = logger;
}
public async Task StartAsync(CancellationToken cancellationToken)
{
_logger.LogInformation("Application starts: ");
await GetData();
//Stop Application
_applicationLifetime.StopApplication();
_logger.LogInformation("Application stops: ");
}
private async Task SendEmailAsync(string email, string serial, string pin, long id)
{
var emailMessage = new MimeMessage();
emailMessage.From.Add(new MailboxAddress("Test","[email protected]" ));
emailMessage.To.Add(new MailboxAddress("", email));
emailMessage.Subject = "Test Codes";
emailMessage.Body = new TextPart("html") { Text = serial +" " + pin };
try
{
var client = new SmtpClient();
await client.ConnectAsync("smtp.gmail.com", 465, true);
await client.AuthenticateAsync("[email protected]", "12345");
await client.SendAsync(emailMessage);
await client.DisconnectAsync(true);
//Update Table
await UpdateData(id);
}
catch (Exception ex)
{
_logger.LogError("Error: {Error} ", ex.Message);
throw; // Throw exception if this exception is unexpected
}
}
private async Task GetData()
{
var connString = _configuration["ConnectionStrings:Development"];
await using var sqlConnection = new SqlConnection(connString);
sqlConnection.Open();
await using var command = new SqlCommand {Connection = sqlConnection};
const string sql = @"Select ID, Email, Serial, Pin from Orders where status = 1";
command.CommandText = sql;
try
{
await using var reader = await command.ExecuteReaderAsync();
while (reader.Read())
{
_logger.LogInformation("Order {ID} , {Email}, {Serial}, {Pin} ", reader.GetInt32(0).ToString(), reader.GetString(1), reader.GetString(2), reader.GetString(3));
await SendEmailAsync(reader.GetString(1).Trim(), reader.GetString(2).Trim(), reader.GetString(3).Trim(), reader.GetInt32(0));
}
}
catch (SqlException exception)
{
_logger.LogError("Error: {Error} ", exception.Message);
throw; // Throw exception if this exception is unexpected
}
}
private async Task UpdateData(long id)
{
var connString = _configuration["ConnectionStrings:Development"];
await using var sqlConnection = new SqlConnection(connString);
sqlConnection.Open();
await using var command = new SqlCommand {Connection = sqlConnection};
const string sql = @"Update Orders set status = 2, Date = GetDate() where id = @id";
command.CommandText = sql;
command.Parameters.Add(new SqlParameter("id", id));
try
{
await command.ExecuteNonQueryAsync();
}
catch (SqlException exception)
{
_logger.LogError("Error: {Error} ", exception.Message);
throw; // Throw exception if this exception is unexpected
}
}
public Task StopAsync(CancellationToken cancellationToken)
{
return Task.CompletedTask;
}
}
}
2 Answers 2
Some quick remarks:
IMHO things like sending emails or retrieving data should be their own class, not methods in a
BusinessService
. Per Wikipedia: "every module, class or function in a computer program should have responsibility over a single part of that program's functionality, and it should encapsulate that part. All of that module, class or function's services should be narrowly aligned with that responsibility."_logger.LogError("Error: {Error} ", ex.Message);
is not sufficient when you have anInnerException
. Look at something like this to get the nested messages as well.GetData()
is far too generic a name. You're retrieving orders, so call your methodGetOrders
. Or rather: put that logic into its own class, name that classOrderService
and name your methodGetAll
. Same forUpdateData
. And if yourOrderService
gets too crowded, consider moving the contents of those methods to their own classed as well, e.g.OrderUpdater
,OrdersRetriever
,...Don't use ADO.NET, use an ORM like Dapper. Or EntityFramework. Learning to use those is a valuable skill, plus they make life much easier for you. Honestly, no ADO.NET code would pass a code review ay any place I've worked at in the past 15 years or so unless you had an extremely good reason for using it.
I know that there's some guideline telling people to name their methods
XxxxAsync
if they'reasync
, but IMHO that was only relevant when you had both a sync and an async version of a method. At least be consistent:SendEmailAsync
vsGetData
andUpdateData
is not consistent.
When use NET Core
and DI
, it would be better if you add customized extensions based on your application requirement, rather than putting everything in one class. This would make it easier to manage and extend.
If we take Main
method, you have :
HostBuilder
-> to configure the host services and configurations.LoggerConfiguration
-> to configureSerilog
logging.
First, let's move the HostBuilder
into Startup
class to configure the services in cleaner way (similar to ASP.NET Core
).
public static class Program
{
static async Task Main(string[] args)
{
await Host
.CreateDefaultBuilder(args)
.ConfigureHostConfiguration(builder =>
{
builder
.SetBasePath(Directory.GetCurrentDirectory())
.AddJsonFile("appsettings.json", optional: false, reloadOnChange: true)
.AddJsonFile(path: "serilog.json", optional: false, reloadOnChange: true);
})
.ConfigureServices((host, services) =>
{
var startup = new Startup(host.Configuration);
// Configure Services
startup.ConfigureServices(services);
})
.Build()
.RunAsync();
}
}
public class Startup
{
public Startup(IConfiguration configuration) => Configuration = configuration;
public IConfiguration Configuration { get; }
public void ConfigureServices(IServiceCollection services)
{
}
}
From now on, use Startup
to configure the services.
For LoggerConfiguration
, you just add an extension method to IServiceCollection
that will contain the Serilog
configuration.
public static class CustomExtensions
{
public static IServiceCollection AddSerilog(this IServiceCollection services)
{
var logger = new LoggerConfiguration()
.MinimumLevel.Information()
.Enrich.FromLogContext()
.WriteTo.Console()
.WriteTo.File("log-.txt", rollingInterval: RollingInterval.Day)
.WriteTo.Email(new EmailConnectionInfo
{
FromEmail = "[email protected]",
ToEmail = "[email protected]",
MailServer = "smtp.yandex.com.tr",
NetworkCredentials = new NetworkCredential
{
UserName = "[email protected]",
Password = "123456"
},
EnableSsl = false,
Port = 587,
EmailSubject = "Send Email Service Error"
}, restrictedToMinimumLevel: LogEventLevel.Error, batchPostingLimit: 1);
Log.Logger = logger.CreateLogger();
//Serilog
services.AddLogging(loggingBuilder => loggingBuilder.AddSerilog(dispose: true));
return services;
}
}
Now, you just need to add this service under Startup.ConfigureServices
public void ConfigureServices(IServiceCollection services)
{
// add Serilog
services.AddSerilog();
// add the hosted services as well
services.AddHostedService<BusinessService>();
}
For the Email
configuration part, it would be better if you just store the configuration in the appsettings.json
and used IConfiguration
to restore them. I can see you have two providers Yandex
and Gmail
, you could do this in the appsettings.json
:
"SmtpProviders": {
"YandexProvider": {
"MailServer": "smtp.yandex.com.tr",
"Port": 587,
"EnableSsl": false,
"FromEmail": "[email protected]",
"ToEmail": "[email protected]",
"Credentials": {
"UserName": "[email protected]",
"Password": "123456"
},
"EmailSubject": "Send Email Service Error"
},
"GmailProvider": {
"MailServer": "smtp.gmail.com",
"Port": 465,
"EnableSsl": true,
"FromEmail": "[email protected]",
"ToEmail": "",
"Credentials": {
"UserName": "[email protected]",
"Password": "123456"
},
"EmailSubject": ""
}
}
Then, implement a class for it :
public interface IMailSettings
{
string MailServer { get; set; }
int Port { get; set; }
bool EnableSsl { get; set; }
string FromEmail { get; set; }
string ToEmail { get; set; }
string EmailSubject { get; set; }
NetworkCredential Credentials { get; set; }
}
public class MailSettings : IMailSettings
{
public string MailServer { get; set; }
public int Port { get; set; }
public bool EnableSsl { get; set; }
public string FromEmail { get; set; }
public string ToEmail { get; set; }
public string EmailSubject { get; set; }
public NetworkCredential Credentials { get; set; }
}
After that, add it to the service collection, and reuse it :
services.AddScoped<IMailSettings, MailSettings>();
var yandexMailSettings = Configuration.GetSection("SmtpProviders:YandexProvider").Get<MailSettings>();
var gmailMailSettings = Configuration.GetSection("SmtpProviders:GmailProvider").Get<MailSettings>();