Import Xml and Csv files into database

Question

I have a simple service class here which import csv or xml file into database using .NET Standard library C#.

Do you have any comments? Are there any recommended techniques to use instead of the switch statement in the method ProcessPaymentFile?

public class AbcPaymentService : IAbcPaymentService
{
 private readonly IAbcPaymentContext _abcPaymentContext;
 private readonly IConfiguration _configuration;
 public AbcPaymentService(IAbcPaymentContext abcPaymentContext, IConfiguration configuration)
 {
 _abcPaymentContext = abcPaymentContext;
 _configuration = configuration;
 }
 public List<PaymentTransactionDetailResponse> GetTransactionsByCurrency(string currency)
 {
 var paymentTransactions = _abcPaymentContext.PaymentTransactions.Where(p => p.CurrencyCode == currency).ToList();
 return MapPaymentTransactions(paymentTransactions);
 }
 public List<PaymentTransactionDetailResponse> GetTransactionsByDateRange(DateTime dateFrom, DateTime dateTo)
 { 
 var paymentTransactions = _abcPaymentContext.PaymentTransactions
 .Where(p => p.TransactionDate >= dateFrom && p.TransactionDate <= dateTo).ToList();
 return MapPaymentTransactions(paymentTransactions);
 }
 public List<PaymentTransactionDetailResponse> GetTransactionsByStatus(string status)
 {
 // add more validation. ie. check length. 
 var paymentTransactions = _abcPaymentContext.PaymentTransactions.Where(p => p.Status == status).ToList();
 return MapPaymentTransactions(paymentTransactions);
 }
 public void ProcessPaymentFile(IFormFile file)
 {
 #region Validation
 var fileExtension = Path.GetExtension(file.FileName);
 var validFileTypes = new[] { ".csv",".xml"}; // move to appsetting for easier configuration. 
 bool isValidType = validFileTypes.Any(t => t.Trim() == fileExtension.ToLower());
 if (isValidType == false)
 throw new ArgumentException($"Unknown format.");
 if(file.Length > 1000) // move to appsetting for easier configuration.
 throw new ArgumentException($"Invalid file size. Only less than 1 MB is allowed.");
 #endregion
 // Upload file to server
 var target = _configuration["UploadPath"];
 var filePath = Path.Combine(target, file.FileName);
 try
 {
 using (var stream = new FileStream(filePath, FileMode.Create))
 {
 file.CopyTo(stream);
 }
 switch (fileExtension.ToLower())
 {
 case ".csv":
 var config = new CsvConfiguration(CultureInfo.InvariantCulture)
 {
 HasHeaderRecord = false, 
 };
 using (var reader = new StreamReader(filePath)) {
 using (var csv = new CsvReader(reader, config))
 {
 csv.Context.RegisterClassMap<CsvMap>();
 var paymentTransactionCsv = csv.GetRecords<Models.Xml.Transaction>().ToList();
 SaveToDb(paymentTransactionCsv);
 }
 } 
 break;
 case ".xml":
 var serializer = new XmlSerializer(typeof(Models.Xml.Transactions));
 using (TextReader reader = new StreamReader(new FileStream(filePath, FileMode.Open)))
 {
 var paymentTransactionXml = (Models.Xml.Transactions)serializer.Deserialize(reader);
 SaveToDb(paymentTransactionXml.Transaction);
 }
 break;
 default:
 throw new ArgumentException($"Invalid file type. Only {string.Join(",", validFileTypes)} allowed.");
 }
 }
 catch (Exception ex)
 {
 throw new Exception(ex.Message);
 } 
 }
 #region PrivateFunctions
 private void SaveToDb(List<Models.Xml.Transaction> paymentTransactions)
 {
 if (PaymentTransactionXmlIsValid(paymentTransactions) == false)
 throw new Exception("Invalid transaction."); // todo: write into log file or db
 // if all validation passed, map objects and 
 var paymentTransactionsEntity = paymentTransactions.Select(p => new PaymentTransaction()
 {
 TransactionId = p.Id,
 TransactionDate = p.TransactionDate,
 Amount = Convert.ToDecimal(p.PaymentDetails.Amount),
 CurrencyCode = p.PaymentDetails.CurrencyCode,
 Status = Mapper.MapStatus(p.Status)
 })
 .ToList();
 // save into db.
 _abcPaymentContext.PaymentTransactions.AddRange(paymentTransactionsEntity);
 _abcPaymentContext.SaveChanges();
 // todo: don't insert duplicate transaction
 }
 private bool PaymentTransactionXmlIsValid(List<Models.Xml.Transaction> paymentTransactions)
 {
 foreach (var trans in paymentTransactions)
 {
 if (string.IsNullOrEmpty(trans.Id)) return false;
 if (trans.TransactionDate == null) return false;
 if(trans.PaymentDetails.Amount == 0) return false;
 if (string.IsNullOrEmpty(trans.PaymentDetails.CurrencyCode)) return false;
 if (string.IsNullOrEmpty(trans.Status)) return false; 
 }
 return true;
 }
 private List<PaymentTransactionDetailResponse> MapPaymentTransactions(List<PaymentTransaction> paymentTransactions) {
 // Construct Dto responses model.
 var paymentTransactionDetailResponses = paymentTransactions.Select(p => new PaymentTransactionDetailResponse()
 {
 Id = p.TransactionId.ToString(),
 Payment = $"{p.Amount} {p.CurrencyCode}",
 Status = p.Status
 }).ToList();
 return paymentTransactionDetailResponses;
 }
 #endregion
}

• radarbob: file.Length > 1000 A megabyte is 1024 kilobytes

Answer 1

Quick remarks:

• IMHO the code inside ProcessPaymentFile (and its related methods) should be a class of its own. It's 50+ lines and should be broken into smaller methods; the fact that you're using #regions is already telling.

• I'm not a fan of _configuration["UploadPath"];. You should consider the approach detailed here: have a POCO object representing the configuration.

• ".csv" and ".xml" appear multiple times, and thus should be const string.

• Instead of doing .ToLower() when comparing strings, consider using string.Equals(). Alternatively, since you always need the lowercase fileExtension, why not make it lowercase when you call Path.GetExtension(file.FileName)?

• Comments should say why something is done (if necessary), not what is done. // save into db. is useless, I can see that is what is happening. Most of the time your code should be self-explanatory and comments shouldn't be necessary.

• Do not pointlessly abbreviate. trans doesn't gain you anything, just call it transaction.

• Consider logging precisely why a transaction is invalid. Moreover, consider validating all transactions and logging all deficiencies, that way you can report back all possible issues in one go, instead of having one problem, reporting that to your users, them fixing it, them re-uploading the file and again being confronted with an error message.

  I have experienced this recently when writing code to access an external service and it was deeply frustrating to experience that when I fixed one issue and re-did my test -- which took a significant amount of time to run -- I experienced another issue, because the service I was connecting to broke on the first error. If I had gotten a full list of problems (which were caused by their documentation being incorrect, BTW) I could have solved them in one go, instead it took two days of whack-a-mole to iron out all the kinks.

• There is no point to have throw new ArgumentException($"Invalid file type. Only {string.Join(",", validFileTypes)} allowed."); However, that is a far clearer error message than the one users will get when they upload an invalid file: throw new ArgumentException($"Unknown format.");.

• "Invalid file size. Only less than 1 MB is allowed." is barely comprehensible. Sure, the user will figure out what you mean, but it would be better to present them with a message that they don't need to "translate". Simply say something like "Uploaded files should be smaller than 1 MB.".

Answer 2

Just two things.

The using keyword is just synctatic sugar for a try/finally block. There's no need here to embed using block in a try block. In general you can use either:

 try {
 sr = new StreamReader(filename);
 txt = sr.ReadToEnd();
 }
 finally {
 if (sr != null) sr.Dispose();
 }

or

using (StreamReader sr = new StreamReader(filename)) {
 txt = sr.ReadToEnd();
 }

If you use a try/catch block you can catch the exceptions for both the StreamReader and DBContext, so I would add a finally block and get rid of using.

In general is good that a method does a single thing. Here, ProcessPaymentFile does more than one thing. I would break it into smaller parts by adding the following methods:

bool IsValidFile()
FileType GetFileType()
bool ProcessCSVFile()
bool ProcessXMLFIle()

The ProcessPaymentFile would be something along these lines:

public bool ProcessPaymentFile(IFormFile file)
 {
 if(IsValidFile(file))
 {
 var fileType = GetFileType(file);
 
 result = fileType is FileType.CSVFile ? ProcessCSVFile(file) : ProcessXMLFile(file);
 
 return result;
 }
 
 return false;
 }

To ease reading, following the flow, testing and to improve thread safety a method should not mutate global variables (i.e. variables not defined in its scope).