Open closed principle in controller action method

Question

I have an ASP.NET MVC project where I need to follow to open closed principles.

The project converts a .csv file to a model from database, but in the future we might have to convert excel files too to the same model from database.

Now, I have this code in the controller:

public ActionResult Index(HttpPostedFileBase file)
{
        // Verify that the user selected a file
        if (file != null && file.ContentLength > 0)
        {
            var path = FileHelper.GetFilePath(file);                
            string filePath = path.Item1;

            if (!string.IsNullOrEmpty(filePath))
            {                    
                if (path.Item2 == "." + Enums.FileType.csv.ToString())
                {
                    List<Company> companiesList = Convertor.ConvertCsvToCompanyModel(filePath);

                    if (companiesList != null && companiesList.Count() > 0)
                    {
                        try
                        {
                            foreach (var company in companiesList)
                            {
                                companyRepository.SaveCompanyItem(company);
                            }
                        }
                        catch(Exception ex)
                        {
                            obj.Handle("Something went wrong at save" + ex.Message);
                        }
                    }

                    TempData["Message"] = "The file import was sucessfull!";
                }
            }
        }

        TempData["ErrorMessage"] = "Please select file";

        // redirect back to the index action to show the form once again           
        return RedirectToAction("Index");
}

Can you give me some hints how to achieve that? I thought that it would be nice to not have more if-s in this action method in case if we want to convert to Excel to file. Thanks!

Answer 1

Trust me the controller or even Index action is not concerned with anything that you are doing on file. All they are concerned about is the data from file that user uploads.

I would move all that file stuff to separate class, say a FileService, and then use this service inside controller. Since the need is to support CSV (current), Excel (future) or both at the same time (you haven't mentioned this), we can think of strategy pattern along with factory method pattern.

public interface IFileProcessor {
 // add proper constraint for T
 List < T > GetRecords < T > (...);
}

public class CsvFileProcessor: IFileProcessor {
 List < T > GetRecords < T > (...) {}
}

public class ExcelFileProcessor: IFileProcessor {
 List < T > GetRecords < T > (...) {}
}

public class FileService {
 public List < T > ReadFile < T > (HttpPostedFileBase file) {
  // do whatever with `file` here
  IFileProcessor fileProcessor = GetFileProcessor( < fileExt > );
  return fileProcessor.GetRecords < T > (...);
 }

 // This is not exact implementation of factory but it serves the same purpose as factory method.
 private static IFileProcessor GetFileProcessor(string fileExt) {
  if ( < csv > ) {
   return ServiceLocator.Current.GetInstance < IFileProcessor > ("csv");
  } 
  else if ( < excel > ) {
   return ServiceLocator.Current.GetInstance < IFileProcessor > ("excel");
  }
 }
}

TIP: CsvFileProcessor and ExcelFileProcessor can readily use CsvHelper and CsvHelper.Excel libs. You just have to specify the model/entity type and libs will return you list of records deserialised to that type.

Now inject this FileService into your controller.