Use Request with Fileupload in Controller

Question

I wrote a fileupload function for my application. Basically the application is a web based folder structure, which has files included.

I don't know, but I am not happy with it. Is it too long? Can I do some parts easier? Is the logging ok? So many questions... The FileModel is inerhiting from ParentModel.

   public ActionResult FileUpload()
        {
            int id = Convert.ToInt32(base.Request.Form["id"]);
            bool isJson = Convert.ToBoolean(base.Request.Form["isJson"]);
            try
            {
                if (id < 1)
                {
                    id = Convert.ToInt32(base.Request.Form["folderId"]);
                }
                if (id < 1)
                {
                    Logging.LogToFile("Could not load " + id ".", 3);
                    base.Response.StatusCode = 500;
                    return Json(new
                    {
                        success = false,
                        errFileUploadMsg = string.Concat(DateTime.Now, " Folder ID ", id, " couldn't load. Please try again.")
                    }, JsonRequestBehavior.AllowGet);
                }
                ParentModel pm = new ParentModel
                {
                    Files = new Files()
                };
                
                Files fm = pm.Files;
                int lastPosition = _mainFolderRepository.GetLastNumberOfFileInFolder(id);
                int position = ((lastPosition.GetType() != typeof(DBNull)) ? (lastPosition + 1) : 0);
                
                for (int i = 0; i < base.Request.Files.Count; i++)
                {
                    HttpPostedFileBase file = base.Request.Files[i];
                    Stream str = base.Request.Files[i].InputStream;
                    BinaryReader br = new BinaryReader(str);
                    byte[] FileBytes = br.ReadBytes((int)str.Length);
                    fm.FullTextSearch = "";
                    fm.File = FileBytes;
                    fm.ContentLength = file.ContentLength;
                    fm.FileName = Path.GetFileName(file.FileName);
                    
                    if (Regex.IsMatch(fm.FileName, "\\d+_\\d+"))
                    {
                        string[] fileNameSections = Path.GetFileNameWithoutExtension(fm.FileName).Split(' ')[0].Split('_');
                        if (fileNameSections[0].All(char.IsDigit) && fileNameSections[1].All(char.IsDigit))
                        {
                            fm.FileName = fm.FileName.Replace("_", "/");
                        }
                    }
                    
                    if (_mainFolderRepository.IsFileNameInFolder(id, fm.FileName))
                    {
                        int fileIdForChange = _mainFolderRepository.GetFileId(id, fm.FileName);
                        _mainFolderRepository.UpdateFile(fileIdForChange, fm.File, fm.ContentLength);
                    }
                    else
                    {
                        if (!Helper.CheckIfExtensionStringAllowed(Path.GetExtension(file.FileName).ToLower()))
                        {
                            throw new FileFormatException(fm.Extension);
                        }
                        fm.Extension = Path.GetExtension(file.FileName).ToLower();

                        if ((fm.Extension == ".pdf" || fm.Extension == ".doc" || fm.Extension == ".docx") && fm.File.Length != 0)
                        {
                            fm.VolltextSuche = ConverterController.ConvertFileToText(fm.File, fm.FileName, fm.Extension);
                        }
                        _mainFolderRepository.FileUpload(fm.File, fm.ContentLength, fm.FileName, "", "", base.User.Identity.Name, DateTime.Now, isPublic: false, fm.Extension, id, fm.RessortId, position, fm.FullTextSearch, "");
                    }
                    position++;
                }
            }
            catch (FileFormatException ex)
            {
                Logging.LogToFile(ex, 3);
                if (!isJson)
                {
                    base.TempData["Error"] = string.Concat(DateTime.Now, " - Extension ", ex.Message, " not allowed.");
                    return RedirectToAction("Index", "MainFolder");
                }
                base.Response.StatusCode = 500;
                return Json(new
                {
                    success = false,
                    errFileUploadMsg = string.Concat(DateTime.Now, " - Extension ", ex.Message, " not allowed.")
                }, JsonRequestBehavior.AllowGet);
            }
            return RedirectToAction("Index", "MainFolder");
        }

Answer 1

• Method has multiple mix responsibilities, it should have one responsibility and clearer scope.
• variable naming doesn't follow naming convention (e.g. id , fm, str) you should always pick a clearer names for your variables.
• when parsing or converting types, you should always validate them or use the appropriate conversions methods such as int.TryParse.
• always work with a copy of the source, don't work with the source directly. this would avoid you a lot of work and mistakes. like the id you're reassigning it with folderId which is confusing, it should be declared separately then switch between them.
• Didn't use the using clause with Disposable object.
• ParentModel is misused, you could declare Files without the need of ParentModel.
• catch clause, will only catch FileFormatException you should also consider catch other exceptions as will don't catch one and ignore the rest.
• when working with Controller response, it's a good idea to create a method for recurring actions to unify your controllers, and keep things simple and manageable.
• returning status code 500 for every error is not a good practice, you should return the appropriate status code, for instance, if folder or file does not exist, you can return 404 status code, if the operation is not allowed then use 405 status code, and use 500 code to all unknown exceptions.
• Not using the appropriate built-in status objects such as HttpNotFoundResult ..etc.
• Helper methods or extensions are very helpful, however Helper.CheckIfExtensionStringAllowed does not tell me where it can be used ? which actions can be applied to ?. If is it for a global scope (meaning this extension should be applied on all files) then, it should be implemented inside FileUpload and not used as Helper. but if is it for current scope, then you need to find a clearer naming or implement it else where with a clearer name.

Create your base Controller using the base Controller class is fine, but sometimes you need to implement an abstract controller of Controller itself, this abstract would contain the main functions that would be applied to all controllers as a way to unify your work and make your code more manageable.

For instance. Say that base.Request.Form["id"] is called in every Action, and also many actions would return Json(). we can implement something like :

public abstract class BaseController : Controller 
{
    protected int Id => int.TryParse(base.Request.Form["id"], out int id) && id > 0 ? id : -1;
    
    protected bool IsJson => bool.TryParse(base.Request.Form["isJson"], out bool isJson) && isJson;

    private ActionResult JsonResponse(HttpStatusCode code, int logId, string message, bool isSuccess)
    {
        Logging.LogToFile(message, logId);
        base.Response.StatusCode = (int) code;
        return Json(new
        {
            success = isSuccess,
            errFileUploadMsg = $"{DateTime.Now} {message}";
        }, JsonRequestBehavior.AllowGet);
    }
    
    protected ActionResult JsonSuccess()
    {
        return JsonResponse(HttpStatusCode.OK, logId, "success", true); 
    }
    
    protected ActionResult JsonFailed(HttpStatusCode code, int logId, string message)
    {
        return JsonResponse(code, logId, message, false); 
    }
    
    protected ActionResult JsonInternalServerError(int logId, string message)
    {
        return JsonResponse(HttpStatusCode.InternalServerError, logId, message, false); 
    }
    
    protected ActionResult JsonNotFound(int logId, string message)
    {
        return JsonFailed(HttpStatusCode.NotFound, logId, message);
    }

    protected ActionResult JsonNotAllowed(int logId, string message)
    {
        return JsonFailed(HttpStatusCode.MethodNotAllowed, logId, message);
    }   
}

now if you inherit this base class you can do this :

public class FileController : BaseController
{
    protected int FolderId => int.TryParse(base.Request.Form["folderId"], out int id) && id > 0 ? id : -1;

    protected ActionResult RedirectToMainFolder()
    {
        return RedirectToAction("Index", "MainFolder");
    }
    
    private bool UploadFile(Files file, int folderId, int position)
    {
        // upload file logic
    }

    public ActionResult UploadFiles()
    {
        if(!int.TryParse(base.Request.Form["id"], out int folderId) || folderId < 1)    
        {
            return JsonNotFound(3, $"Could not load Folder ID {folderId}, Please try again.");
        }
        //...
        try
        {
            //... uploading file logic
        
        }
        catch (FileFormatException ex)
        {
            Logging.LogToFile(ex, 3);
            
            var message = $"Extension {ex.Message} not allowed.";
            
            if (!IsJson)
            {
                base.TempData["Error"] = message;
                
                return RedirectToMainFolder();
            }
            
            return JsonNotAllowed(3, message);
        }
        catch(Exception ex)
        {           
            return JsonInternalServerError(3, ex.Message);  
        }
        
        return RedirectToMainFolder();
}

There are many methods are already exist in the MVC controller that already defined for each HttpStatusCode, such as NotFound(), Forbidden ..etc., which you should use, however, I just want to show you how you can have your custom results which would help you to have a better management to your controllers.

For the current UploadFiles Action, you will need to extract the file processing into a separate method, and then recall it from inside the action. Something like this :

private bool UploadFile(Files file, int folderId, int position)
{
    if(folderId < 1)
    {
        throw new InvalidOperationException("Folder ID {folderId} couldn't load. Please try again.");
    }
    
    if(file.File?.Length == 0)
    {
        throw new ArgumentNullException(nameof(file));
    }
    
    if(!Regex.IsMatch(file.FileName, "\\d+_\\d+"))
    {
        return false;
    }
    
    string[] fileNameSections = Path.GetFileNameWithoutExtension(file.FileName).Split(' '); 
        
    if(fileNameSections?.Length == 0)
    {
        throw new Exception("File name couldn't be read.");
    }

    var sectionParts = fileNameSections[0].Split('_');
    
    if(sectionParts?.Length >= 2 && sectionParts[0].All(char.IsDigit) && sectionParts[1].All(char.IsDigit))
    {
        file.FileName = file.FileName.Replace("_", "/");
    }
    
    if (_mainFolderRepository.IsFileNameInFolder(folderId, file.FileName))
    {
        int fileIdForChange = _mainFolderRepository.GetFileId(folderId, file.FileName);

        _mainFolderRepository.UpdateFile(fileIdForChange, file.File, file.ContentLength);
    }
    else
    {
        if (!Helper.CheckIfExtensionStringAllowed(Path.GetExtension(file.FileName).ToLower()))
        {
            throw new FileFormatException(file.Extension);
        }
        
        fm.Extension = Path.GetExtension(file.FileName).ToLower();

        if ((file.Extension == ".pdf" || file.Extension == ".doc" || file.Extension == ".docx") && file.File.Length != 0)
        {
            file.VolltextSuche = ConverterController.ConvertFileToText(file.File, file.FileName, file.Extension);
        }
        
        _mainFolderRepository.FileUpload(file.File, file.ContentLength, file.FileName, "", "", base.User.Identity.Name, DateTime.Now, isPublic: false, file.Extension, id, file.RessortId, position, file.FullTextSearch, "");
    }           
    
    return true;
}

public ActionResult UploadFiles()
{
    if(FolderId < 1)    
    {
        return JsonNotFound(3, $"Could not load Folder ID {FolderId}, Please try again.");
    }
    
    try
    {
        Files fileModel = new Files();

        int lastPosition = _mainFolderRepository.GetLastNumberOfFileInFolder(id);
        
        int position = ((lastPosition.GetType() != typeof(DBNull)) ? (lastPosition + 1) : 0);
        
        for (int i = 0; i < base.Request.Files.Count; i++)
        {
            HttpPostedFileBase file = base.Request.Files[i];
            
            int fileLength = file.InputStream.Length;
            
            byte[] fileBytes = new byte[fileLength];
            
            using(var stream = new Stream(file.InputStream))            
            {
                stream.Seek(0 , SeekOrigin.Begin);
                stream.Read(fileBytes , 0 , (int) stream.Length);
            }

            fileModel.FullTextSearch = "";
            fileModel.File = fileBytes;
            fileModel.ContentLength = file.ContentLength;
            fileModel.FileName = Path.GetFileName(file.FileName);
            
            var isSuccess = UploadFile(fileModel, FolderId, position++);
            
            if(!isSuccess)
            {
                // do something 
            }
        }
    }
    catch (FileFormatException ex)
    {
        Logging.LogToFile(ex, 3);
        
        var message = $"Extension {ex.Message} not allowed.";
        
        if (!IsJson)
        {
            base.TempData["Error"] = message;
            
            return RedirectToMainFolder();
        }
        
        return JsonNotAllowed(3, message);
    }
    catch(Exception ex)
    {           
        return JsonInternalServerError(3, ex.Message);  
    }
    
    return RedirectToMainFolder();
}

This is just a demonstration on the above points, you should focus on clearing the code, dividing it to chucks based on role and responsibility, make use of other OPP principles such as inheriting's to have a better code experience. Also, make use of pre-existing functions that shipped with the .NET.