Naming Convention

current code is not usingcamelCase naming convention on localvariables, and also is not given a good naming to some variables likeDisplayFileName,Month,DirectoryCreate ..etc.it should be cameled-case.

Some variables have wrong naming likeDisplayFileName it should befile because it's used forRequest.Form.Files[0] and not toRequest.Form.Files[0].FileName.dbPath is not best name for explaining a file that is related to the stored one. It could be changed tofileToComparePath since it's going to be compared with the input file and theexportPath can befileExportPath.

it's important to always differs betweenDirectory andFile paths by either suffix thefile ordir to the name, otherwise, it won't be a clear enough to other developers.

When dealing withpaths you should always use the built-in methods for likeSystem.IO.File andSystem.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 likeCExcel ex = new CExcel(); it would be better to name itexcel rather thanex, and alsovar m in mods in theforeach loop.

Always give your variables a good explainable naming.

using

withusing clause, there is no need toFlush orClose orDispose, 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 variablesFinalPath,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 methodsDirectory,File andPath to avoid any unexcepted exceptions that have been already covered by them.
• withif conditions, don't put the exception or the error at theelse, keep it at the top as it would be clearer than to be at the end of the code.
• when doingToList() it'll return a newList.
• withStream always useWrite instead ofCopyTo asWrite is specific use and optimized forStream, whileCopyTo 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);

• \$\begingroup\$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 "\\"\$\endgroup\$

  ahmed barbary

  – ahmed barbary

  2021-08-29 07:04:57 +00:00

  CommentedAug 29, 2021 at 7:04
• \$\begingroup\$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\$\endgroup\$

  ahmed barbary

  – ahmed barbary

  2021-08-29 07:07:26 +00:00

  CommentedAug 29, 2021 at 7:07
• \$\begingroup\$@ahmedbarbary usePath.Combine to concatenate paths as of my example above. For file processing, you should consider usingSafeFileHandledocs.microsoft.com/en-us/dotnet/api/…\$\endgroup\$

  iSR5

  – iSR5

  2021-08-29 14:14:22 +00:00

  CommentedAug 29, 2021 at 14:14
• \$\begingroup\$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\$\endgroup\$

  ahmed barbary

  – ahmed barbary

  2021-08-29 15:57:12 +00:00

  CommentedAug 29, 2021 at 15:57
• \$\begingroup\$also how to use async task on web api above\$\endgroup\$

  ahmed barbary

  – ahmed barbary

  2021-08-30 06:22:34 +00:00

  CommentedAug 30, 2021 at 6:22

• c#
• asp.net-core
• asp.net-web-api
• asp.net-mvc-4
• asp.net-core-webapi