Web Api to upload excel file

Question

I work on asp.net core 2.2 web API using C# language

I need to rewrite function below with best syntax and with best practice

web API below get Excel file from upload and return Excel file

it working without any issue but I need to rewrite it with best syntax and practice

some points I need to changes :

concatenate path is best thing using

are streaming using correctly

are memory copying file is correct

[HttpPost, DisableRequestSizeLimit]
 [Route("Upload")]
 public IActionResult Upload()
 {
 try
 {
 var DisplayFileName = Request.Form.Files[0];
 string fileName = DisplayFileName.FileName.Replace(".xlsx", "-") + Guid.NewGuid().ToString() + ".xlsx";
 string Month = DateTime.Now.Month.ToString();
 string DirectoryCreate = myValue1 + "\\" + Month + "\\" + fileName;
 string exportDirectory = myValue2 + "\\" + Month;
 string exportPath = myValue2 + "\\" + Month + "\\" + fileName;
 string FinalPath = exportPath;
 
 if (!Directory.Exists(DirectoryCreate))
 {
 Directory.CreateDirectory(DirectoryCreate);
 
 }
 if (!Directory.Exists(exportDirectory))
 {
 Directory.CreateDirectory(exportDirectory);
 
 }
 CExcel ex = new CExcel();
 if (DisplayFileName.Length > 0)
 {
 var filedata = ContentDispositionHeaderValue.Parse(Request.Form.Files[0].ContentDisposition).FileName.Trim('"');
 var dbPath = Path.Combine(DirectoryCreate, fileName);
 
 using (var stream = new FileStream(dbPath, FileMode.Create))
 {
 Request.Form.Files[0].CopyTo(stream);
 stream.Flush();
 stream.Close();
 }
 GC.Collect();
 string error = "";
 int rowCount = 0;
 string inputTemplatePath = "";
 
 var InputfilePath = System.IO.Path.Combine(GetFilesDownload, "DeliveryGeneration_Input.xlsx");
 bool areIdentical = ex.CompareExcel(dbPath, InputfilePath, out rowCount, out error);
 if (areIdentical == true)
 {
 List<InputExcel> inputexcellist = new List<InputExcel>();
 inputexcellist = ex.Import(dbPath);
 List<string> mods = new List<string>();
 mods = inputexcellist.Select(x => x.ModuleName).Distinct().ToList();
 var OutputfilePath = System.IO.Path.Combine(GetFilesDownload, "DeliveryGeneration_Output.xlsx");
 if (System.IO.Directory.Exists(Path.Combine(exportDirectory, fileName)))
 {
 throw new Exception("Ok so the error message IS right.");
 }
 System.IO.File.Copy(OutputfilePath, Path.Combine(exportDirectory, fileName), true);
 
 SqlConnection con;
 foreach (var m in mods)
 {
 List<InputExcel> inputmodulelist = new List<InputExcel>();
 inputmodulelist = inputexcellist.Where(x => x.ModuleName == m).ToList();
 var dtimport = DatatableConversion.ToDataTable(inputmodulelist);
 DataTable dtexport = new DataTable();
 dtexport = _deliveryService.LoadExcelToDataTable(_connectionString, dtimport);
 ex.Export(dtexport, m, exportPath);
 
 }
 }
 var memory2 = new MemoryStream();
 using (var stream = new FileStream(exportPath, FileMode.Open))
 {
 stream.CopyTo(memory2);
 }
 memory2.Position = 0;
 
 return File(memory2, "text/plain", Path.GetFileName(exportPath));
 
 }
 else
 {
 return BadRequest();
 }
 }
 catch (Exception ex)
 {
 return StatusCode(500, $"Internal server error: {ex}");
 }
 }

Update original post

function above get excel file as input with some data

then I search on database for matched data input

then get matched data on database on output file

then on last download it with output data

I using name space

using ClosedXML.Excel;
using OfficeOpenXml;

• iSR5: wait a minute, how does it work ? I can see you're storing the file in a directory, and in the end you're loading an excel from the database. Is the file stored twice ? as it seems you're getting a full DataTable from the database, not some metadata that could reference the actual file. please give us more details.
• ahmed barbary: 1-get Excel file I uploaded from front end 2- create folder path as input 3-get data from database and export data to excel sheet with multi tab 4-then download out file multi tab it
• ahmed barbary: I get input file then fill it with data from database then output Excel file with data come from database
• iSR5: What Excel library are you using ?
• ahmed barbary: i updated original post

Answer 1

Naming Convention

current code is not using camelCase naming convention on local variables, and also is not given a good naming to some variables like DisplayFileName, Month, DirectoryCreate ..etc.it should be cameled-case.

Some variables have wrong naming like DisplayFileName it should be file because it's used for Request.Form.Files[0] and not to Request.Form.Files[0].FileName. dbPath is not best name for explaining a file that is related to the stored one. It could be changed to fileToComparePath since it's going to be compared with the input file and the exportPath can be fileExportPath.

it's important to always differs between Directory and File paths by either suffix the file or dir to the name, otherwise, it won't be a clear enough to other developers.

When dealing with paths you should always use the built-in methods for like System.IO.File and System.IO.Directory. This would ensure the compatibility with the current system requirements.

Short names are fast to type, easy to miss, and hard to follow. So, try to avoid short names like CExcel ex = new CExcel(); it would be better to name it excel rather than ex, and also var m in mods in the foreach loop.

Always give your variables a good explainable naming.

using

with using clause, there is no need to Flush or Close or Dispose, as it would automatically do that at the end of the block.

So this :

using (var stream = new FileStream(dbPath, FileMode.Create))
{
 Request.Form.Files[0].CopyTo(stream);
 stream.Flush();
 stream.Close();
}

should be like this :

using (var stream = new FileStream(dbPath, FileMode.Create))
{
 // you should use Write(stream) instead, or reset the stream position.
 Request.Form.Files[0].CopyTo(stream); 
}

Unused variables FinalPath, filedata, inputTemplatePath, con why they're still there ? you should remove unused variables.

Other Notes

• When working with files and directories, it would be wise to use the built-in methods Directory, File and Path to avoid any unexcepted exceptions that have been already covered by them.
• with if conditions, don't put the exception or the error at the else, keep it at the top as it would be clearer than to be at the end of the code.
• when doing ToList() it'll return a new List.
• with Stream always use Write instead of CopyTo as Write is specific use and optimized for Stream, while CopyTo is built for all compatible collections types.

Code Comments

here is some comments on the code (code has been shorten for brevity).

if (DisplayFileName.Length > 0) // should be inverted.
{
// var filedata = ContentDispositionHeaderValue.Parse(Request.Form.Files[0].ContentDisposition).FileName.Trim('"'); //unused 
 
 var dbPath = Path.Combine(DirectoryCreate, fileName); // this would return ../path/month/fileName-SomeGUID.xlsx/fileName-SomeGUID.xlsx
 using (var stream = new FileStream(dbPath, FileMode.Create))
 {
 Request.Form.Files[0].CopyTo(stream);
 // the Flush() and Close are handled by the `using` blocks
// stream.Flush();
// stream.Close();
 }
 
// GC.Collect(); // unnecessary
 
 string error = ""; // use null;
 
 int rowCount = 0;
 
// string inputTemplatePath = ""; //unused
 
 var InputfilePath = System.IO.Path.Combine(GetFilesDownload, "DeliveryGeneration_Input.xlsx");
 
 bool areIdentical = ex.CompareExcel(dbPath, InputfilePath, out rowCount, out error); // rowCount and error have never been checked. what happens if they have values ? 
 if (areIdentical == true)
 {
// List<InputExcel> inputexcellist = new List<InputExcel>();// unnecessary
 
 List<InputExcel> inputexcellist = ex.Import(dbPath);
 
// List<string> mods = new List<string>(); // unnecessary
 
 // it could be used in the foreach directlry 
 List<string> mods = inputexcellist.Select(x => x.ModuleName).Distinct().ToList();
 
 var OutputfilePath = System.IO.Path.Combine(GetFilesDownload, "DeliveryGeneration_Output.xlsx");
 
 // This is unnecessary as the `File.Copy` ovewrite flag is true.
 // Although Directory.Exists should be File.Exists 
 // if (System.IO.Directory.Exists(Path.Combine(exportDirectory, fileName)))
 // {
 // throw new Exception("Ok so the error message IS right.");
 // }
 // Path.Combine(exportDirectory, fileName) has the same value of exportPath 
 System.IO.File.Copy(OutputfilePath, Path.Combine(exportDirectory, fileName), true);
// SqlConnection con; // unused
 foreach (var m in mods) // m should be moduleName or name
 {
// List<InputExcel> inputmodulelist = new List<InputExcel>(); // unnecessary
 List<InputExcel> inputmodulelist = inputexcellist.Where(x => x.ModuleName == m).ToList();
 DataTable dtimport = DatatableConversion.ToDataTable(inputmodulelist); 
 
// DataTable dtexport = new DataTable(); // unnecessary
 DataTable dtexport = _deliveryService.LoadExcelToDataTable(_connectionString, dtimport);
 ex.Export(dtexport, m, exportPath);
 }
 }
 
 
 var memory2 = new MemoryStream();
 
 using (var stream = new FileStream(exportPath, FileMode.Open))
 {
 stream.CopyTo(memory2);
 }
 
 memory2.Position = 0;
 
 
 // Path.GetFileName(exportPath) is equal to fileName
 // "text/plain" ? why ? you should use the appropriate mime type for Excel 
 // for .xls = application/vnd.ms-excel
 // for .xlsx = application/vnd.openxmlformats-officedocument.spreadsheetml.sheet
 // or just use the ClosedXML Web API Extension
 return File(memory2, "text/plain", Path.GetFileName(exportPath)); 
}
else
{
 // invert the if, and move this at the top of blocks 
 return BadRequest();
}

Suggested Revision

if(Request.Form.Files.Length == 0)
{
 throw new InvalidOperationException(" No Files"); 
}
var file = Request.Form.Files[0];
var fileName = $"{Path.GetFileNameWithoutExtension(file.FileName)}-{Guid.NewGuid()}.xlsx";
Func<string, string> generateFilePath = (path) => 
 !string.IsNullOrWhiteSpace(path) && !string.IsNullOrWhiteSpace(fileName) 
 ? Path.Combine(Directory.CreateDirectory(path).FullName, DateTime.Today.Month.ToString() , fileName) 
 : null;
var fileToComparePath = generateFilePath(myValue1);
var fileToExportPath = generateFilePath(myValue2);
if(fileToComparePath == null)
{
 throw new ArgumentNullException(nameof(fileToComparePath));
}
if(fileToExportPath == null)
{
 throw new ArgumentNullException(nameof(fileToExportPath));
}
using(var fileStream = File.Create(dbPath))
{
 fileStream.Write(file);
}
if(!ex.CompareExcel(fileToComparePath, Path.Combine(GetFilesDownload, "DeliveryGeneration_Input.xlsx"), out int rowCount, out string error))
{
 return BadRequest();
}
File.Copy(Path.Combine(GetFilesDownload, "DeliveryGeneration_Output.xlsx"), fileToExportPath, true);
CExcel excel = new CExcel();
foreach (var moduleName in excel.Import(dbPath).Select(x => x.ModuleName).Distinct())
{
 var inputModuleList = inputexcellist.Where(x => x.ModuleName == moduleName).ToList();
 var tableImport = DatatableConversion.ToDataTable(inputModuleList);
 
 var tableExport = _deliveryService.LoadExcelToDataTable(_connectionString, tableImport);
 excel.Export(tableExport, moduleName, fileToExportPath);
}
MemoryStream excelFileStream = new MemoryStream();
using(var fileStream = File.Open(fileToExportPath, FileMode.Open))
{
 fileStream.Write(excelFileStream);
}
return File(excelFileStream, "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", fileName); 

• ahmed barbary: thank you very much for your advices but there are more things i need to ask 1-string exportDirectory = myValue2 + "\\" + Month; so my question are there are another way to concate path without using "\\"
• ahmed barbary: also another thing how to avoid error The process cannot access the file because it is being used by another process when create new file on my code
• iSR5: @ahmedbarbary use Path.Combine to concatenate paths as of my example above. For file processing, you should consider using SafeFileHandle docs.microsoft.com/en-us/dotnet/api/…
• ahmed barbary: can you please help me how to use safefilehandle according to code above please only that what i need how to use safe file handle on code above. can you please help me
• ahmed barbary: also how to use async task on web api above