Not feeling 100% about my Controller design.

Basically, I'm uploading an excel file and parsing the information then displaying what was parsed in a view.

```
using System.Data;
using System.Data.OleDb;
using System.Web;
using System.Web.Mvc;
using QuimizaReportes.Models;
using System.Collections.Generic;
using System;

namespace QuimizaReportes.Controllers
{
public class UploadController : Controller
{
public ActionResult Index()
{
return View();
}

[HttpPost]
public ActionResult Index(HttpPostedFileBase excelFile)
{
if (excelFile != null)
{
//Save the uploaded file to the disc.
string savedFileName = "~/UploadedExcelDocuments/" + excelFile.FileName;
excelFile.SaveAs(Server.MapPath(savedFileName));

//Create a connection string to access the Excel file using the ACE provider.
//This is for Excel 2007. 2003 uses an older driver.
var connectionString = string.Format("Provider=Microsoft.ACE.OLEDB.12.0;Data Source={0};Extended Properties=Excel 12.0;", Server.MapPath(savedFileName));

//Fill the dataset with information from the Hoja1 worksheet.
var adapter = new OleDbDataAdapter("SELECT * FROM [Hoja1$]", connectionString);
var ds = new DataSet();
adapter.Fill(ds, "results");
DataTable data = ds.Tables["results"];

var people = new List();

for (int i = 0; i ("Id");
newPerson.Name = data.Rows[i].Field("Name");
newPerson.LastName = data.Rows[i].Field("LastName");
newPerson.DateOfBirth = data.Rows[i].Field("DateOfBirth");

people.Add(newPerson);
}

return View("UploadComplete", people);
}

return RedirectToAction("Error", "Upload");
}

public ActionR

The logic for reading the Excel file (i.e. everything from var connectionString until before the return) belongs in a model method, not the controller.

You might also want to handle the case when the uploaded file isn't an Excel file or it doesn't have the columns you expect it to. Depending on how you use this or who uses it, this might not be necessary, but if normal users are allowed to upload files, they might very well upload the wrong file even if you tell them not to. And in that case a meaningful error message leads to a better user experience.

Regarding the question in your comment: As I said, I don't know how this is going to be used, but I can't think of a situation where it would be helpful for the user to be able to re-upload the file by hitting the refresh button. So yes, redirecting to an UploadComplete action makes sense.

StackExchange Code Review Q#981, answer score: 10