Problem
In my .net core console application, I am reading multiple excel files and bulk copy data into a database table. 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.
class Program
{
static async Task Main(string[] args)
{
try
{
string s = null;
var d = new DirectoryInfo(@"C:Test");
var files = d.GetFiles("*.xlsx");
var usersList = new List<User>();
foreach (var file in files)
{
var fileName = file.FullName;
using var package = new ExcelPackage(file);
ExcelPackage.LicenseContext = LicenseContext.NonCommercial;
var currentSheet = package.Workbook.Worksheets;
var workSheet = currentSheet.First();
var noOfCol = workSheet.Dimension.End.Column;
var noOfRow = workSheet.Dimension.End.Row;
for (int rowIterator = 2; rowIterator <= noOfRow; rowIterator++)
{
var user = new User
{
GameCode = workSheet.Cells[rowIterator, 1].Value?.ToString(),
Count = Convert.ToInt32(workSheet.Cells[rowIterator, 2].Value),
Email = workSheet.Cells[rowIterator, 3].Value?.ToString(),
Status = Convert.ToInt32(workSheet.Cells[rowIterator, 4].Value)
};
usersList.Add(user);
}
}
var conn = ConfigurationManager.ConnectionStrings["Development"].ConnectionString;
await using var connString = new SqlConnection(conn);
connString.Open();
await BulkWriter.InsertAsync(usersList, "[Orders]", connString, CancellationToken.None);
}
catch (Exception e)
{
Console.WriteLine(e);
throw;
}
}
private class BulkWriter
{
private static readonly ConcurrentDictionary<Type, SqlBulkCopyColumnMapping[]> ColumnMapping =
new ConcurrentDictionary<Type, SqlBulkCopyColumnMapping[]>();
public static async Task InsertAsync<T>(IEnumerable<T> items, string tableName, SqlConnection connection,
CancellationToken cancellationToken)
{
using var bulk = new SqlBulkCopy(connection);
await using var reader = ObjectReader.Create(items);
bulk.DestinationTableName = tableName;
foreach (var colMap in GetColumnMappings<T>())
bulk.ColumnMappings.Add(colMap);
await bulk.WriteToServerAsync(reader, cancellationToken);
}
private static IEnumerable<SqlBulkCopyColumnMapping> GetColumnMappings<T>() =>
ColumnMapping.GetOrAdd(typeof(T),
type =>
type.GetProperties()
.Select(p => new SqlBulkCopyColumnMapping(p.Name, p.Name)).ToArray());
}
}
Updated version:
I also added excel extension change and moving excel into another folder. Any comments?
public static class Program
{
private static async Task ReadAllExcelFilesInDirectory(string path, List<User> usersList)
{
var dirInfo = new DirectoryInfo(path);
var files = dirInfo.GetFiles("*.xlsx");
foreach (var file in files)
await ReadExcelFile(file, usersList);
}
private static async Task ReadExcelFile(FileInfo fileName, List<User> usersList)
{
const string destinationFile = @"T:Test";
if (fileName == null) throw new ArgumentNullException(nameof(fileName));
if (usersList == null) throw new ArgumentNullException(nameof(usersList));
using var package = new ExcelPackage(fileName);
ExcelPackage.LicenseContext = LicenseContext.NonCommercial;
var currentSheet = package.Workbook.Worksheets;
var workSheet = currentSheet.First();
var noOfCol = workSheet.Dimension.End.Column;
var noOfRow = workSheet.Dimension.End.Row;
for (var rowIterator = 2; rowIterator <= noOfRow; rowIterator++)
{
var user = new User
{
GameCode = workSheet.Cells[rowIterator, 1].Value?.ToString(),
Count = Convert.ToInt32(workSheet.Cells[rowIterator, 2].Value),
Email = workSheet.Cells[rowIterator, 3].Value?.ToString(),
Status = Convert.ToInt32(workSheet.Cells[rowIterator, 4].Value)
};
usersList.Add(user);
}
//Change extension
await using var newfile = File.OpenWrite(fileName.FullName + ".old");
await package.SaveAsAsync(newfile);
// To move a file or folder to a new location
File.Move(fileName.FullName, destinationFile + "\" + fileName.Name);
}
private static async Task InsertUsersIntoDatabase(List<User> usersList)
{
var conn = ConfigurationManager.ConnectionStrings["Development"].ConnectionString;
await using var connString = new SqlConnection(conn);
connString.Open();
await InsertAsync(usersList, "[GameAPI].[dbo].[A101Orders]", connString, CancellationToken.None);
}
private static async Task Main(string[] args)
{
try
{
var usersList = new List<User>();
await ReadAllExcelFilesInDirectory(
@"C:Test", usersList);
await InsertUsersIntoDatabase(usersList);
}
catch (Exception e)
{
Console.WriteLine(e);
throw;
}
}
private static async Task InsertAsync<T>(IEnumerable<T> items, string tableName, SqlConnection connection,
CancellationToken cancellationToken)
{
using var bulk = new SqlBulkCopy(connection);
await using var reader = ObjectReader.Create(items);
bulk.DestinationTableName = tableName;
var properties = typeof(T).GetProperties();
foreach (var prop in properties)
bulk.ColumnMappings.Add(new SqlBulkCopyColumnMapping(prop.Name, prop.Name));
await bulk.WriteToServerAsync(reader, cancellationToken);
}
}
Solution
Missing classes
You have failed to include your definition of the User class in the code snippet so I had to create my own.
class User
{
public string GameCode { get; set; }
public int Count { get; set; }
public string Email { get; set; }
public int Status { get; set; }
}
For ConfigurationManager there are 3 different packages that I could install.
- Microsoft.IdentityModel.Protocols
- System.Configuration.ConfigurationManager
- NLog
After consulting my magic 8-ball, I decided to go for System.Configuration.ConfigurationManager.
Incorrect usage of try…catch
Placing your entire program into one try…catch block serves no purpose and makes debugging much harder.
Splitting concerns into functions
Currently, your main program does it all. Consider splitting it out into the following functions:
class Program
{
static void ReadExcelFile(string fileName, List<User> usersList)
{
// ...
}
static void ReadAllExcelFilesInDirectory(string path, List<User> usersList)
{
var dirInfo = new DirectoryInfo(path);
var files = dirInfo.GetFiles("*.xlsx");
foreach (var file in files)
ReadExcelFile(file.FullName, usersList);
}
static async void InsertUsersIntoDatabase(List<User> usersList)
{
// ...
}
static async Task Main(string[] args)
{
var usersList = new List<User>();
ReadAllExcelFilesInDirectory(@"C:Test", usersList);
InsertUsersIntoDatabase(usersList);
}
}
Opening the database connection
Here you have mixed up conn and connString.
var conn = ConfigurationManager.ConnectionStrings["Development"].ConnectionString;
await using var connString = new SqlConnection(conn);
connString.Open();
The following would have made much more sense:
var connString = ConfigurationManager.ConnectionStrings["Development"].ConnectionString;
using var conn = new SqlConnection(connString);
await conn.OpenAsync();
BulkWriter
I think you are overcomplicating things here. The following is much simpler and easier to read:
class BulkWriter
{
public static async Task InsertAsync<T>(IEnumerable<T> items, string tableName, SqlConnection connection,
CancellationToken cancellationToken)
{
using var bulk = new SqlBulkCopy(connection);
await using var reader = ObjectReader.Create(items);
bulk.DestinationTableName = tableName;
var properties = typeof(T).GetProperties();
foreach (var prop in properties)
bulk.ColumnMappings.Add(new SqlBulkCopyColumnMapping(prop.Name, prop.Name));
await bulk.WriteToServerAsync(reader, cancellationToken);
}
}
You can actually remove the whole BulkWriter class and simply make InsertAsync a method of Program
Unused variables
Right at the top on Main, you have this line that does not do anything:
string s = null;
Final thoughts
Every time you make an asynchronous call, you immediately use await to block execution until it is done. Is it really worth it?