0
\$\begingroup\$

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;
\$\endgroup\$
5
  • \$\begingroup\$ 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. \$\endgroup\$
    – iSR5
    Aug 28, 2021 at 17:38
  • \$\begingroup\$ 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 \$\endgroup\$ Aug 28, 2021 at 17:40
  • \$\begingroup\$ I get input file then fill it with data from database then output Excel file with data come from database \$\endgroup\$ Aug 28, 2021 at 17:43
  • \$\begingroup\$ What Excel library are you using ? \$\endgroup\$
    – iSR5
    Aug 28, 2021 at 17:54
  • \$\begingroup\$ i updated original post \$\endgroup\$ Aug 28, 2021 at 18:23

1 Answer 1

2
\$\begingroup\$

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); 
\$\endgroup\$
5
  • \$\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\$ Aug 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\$ Aug 29, 2021 at 7:07
  • \$\begingroup\$ @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/… \$\endgroup\$
    – iSR5
    Aug 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\$ Aug 29, 2021 at 15:57
  • \$\begingroup\$ also how to use async task on web api above \$\endgroup\$ Aug 30, 2021 at 6:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.