Remove functionality from ASP.NET MVC controller

Question

I have a controller which is used to save data in database. The controller looks like below:

[Authorize]
[HttpPost]
public ActionResult Create(EmployeeFormViewModel viewModel)
{
    var _employee = new Employee
    {
        Employee = User.Identity.GetUserId(),
        DateTime = DateTime.Parse(string.Format("{0} {1}", viewModel.Date, viewModel.Time))
    };

    _context.Employees.Add(_employee);
    _context.SaveChanges();

    return RedirectToAction("Index", "Home");
}

I want to remove this line of code

DateTime.Parse(string.Format("{0} {1}", viewModel.Date, viewModel.Time))

and make this calculations somewhere else in order to keep the controller clean. Which is the best way to archive this?

Have you considered moving that code into the Employee's constructor and just saying var_employee = new Employee(User.Identity.GetUserId, viewModel.Date, viewModel.Time) and parsing it there? — Batman
– Batman, Commented Jun 29, 2016 at 20:56
Create a data mapper class that maps the view model data to an entity instance. — Dmitry S.
– Dmitry S., Commented Jun 29, 2016 at 21:25

Ervis Trupja · Accepted Answer · 2016-06-29 20:44:18Z

From the data given I see that you have used a ViewModel called EmployeeFormViewModel to saperate the logic from the model. I would guess that your ViewModel looks something like below:

public class EmployeeFormViewModel
    {
        public string Venue { get; set; }
        public string Date { get; set; }
        public string Time { get; set; }
    }

Now, in order to make the changes in controller, i would suggest you make it look like below:

        [Authorize]
        [HttpPost]
        public ActionResult Create(EmployeeFormViewModel viewModel)
        {
            var _employee = new Employee
            {
                Employee = User.Identity.GetUserId(),
                DateTime = viewModel.DateTime
            };

            _context.Employees.Add(_employee);
            _context.SaveChanges();

            return RedirectToAction("Index", "Home");
        }

and after that go to your ViewModel and add the new property that you added in the Controller (DateTime). Now your ViewModel should look something like below:

public class EmployeeormViewModel
    {
        public string Venue { get; set; }
        public string Date { get; set; }
        public string Time { get; set; }
        public DateTime DateTime
        {
            get
            {
                return DateTime.Parse(string.Format("{0} {1}", Date, Time));
            }
        }
    }

I hope this solves your problem.

Community · Accepted Answer · 2017-05-23 11:44:24Z

To offer a different perspective, I'd suggest you could put it in an extension method. The concept of combining date and time strings doesn't really feel like it should belong to your domain model, it feels like a generic thing that you might want to use across your application (or even in other applications). I would do this...

public static class DateTimeExtensions
{
  public static DateTime ParseToDateTime(this string date, string time = null)
  {
     return string.IsNullOrEmpty(withTime) ? DateTime.Parse(date) : DateTime.Parse($"{date} {time}");
  }
}

And in the controller...

[Authorize]
[HttpPost]
public ActionResult Create(EmployeeFormViewModel viewModel)
{
    var _employee = new Employee
    {
        Employee = User.Identity.GetUserId(),
        DateTime = viewModel.Date.ParseToDateTime(viewModel.Time)
    };

EDIT: Additionally...to incorporate etr's answer, which is also a good approach, you could combine the two...

public class EmployeeormViewModel
    {
        public string Venue { get; set; }
        public string Date { get; set; }
        public string Time { get; set; }
        public DateTime DateTime
        {
            get
            {
                return Date.ParseToDateTime(Time);
            }
        }
    }

gnllucena · Accepted Answer · 2016-06-29 20:41:41Z

0

Rich domain is the way.

public class Employee
{
    public Employee(int id, object date, object time)
    {
        Id = id;
        DateTime = DateTime.Parse(string.Format("{0} {1}", date, time))
    }

    public int Id { get; protected set; }
    public DateTime DateTime  { get; protected set; }
}

And them:

[Authorize]
[HttpPost]
public ActionResult Create(EmployeeFormViewModel viewModel)
{
    _context.Employees.Add(new Employee(User.Identity.GetUserId(), viewModel.Date, viewModel.Time));
    _context.SaveChanges();

    return RedirectToAction("Index", "Home");
}

answered Jun 29, 2016 at 20:41

gnllucena

8247 silver badges6 bronze badges

2 Comments

Kenneth K. Over a year ago

object for date and time? What is this, .NET 1.0?

gnllucena Over a year ago

I didn't know what types you used in viewModel.Date and viewModel.Time, so I used object.

JWP · Accepted Answer · 2016-06-29 21:20:34Z

I like strong type binding and a Post method as follows:

public ActionResult Create(EmployeeFormViewModel viewModel)
{

    viewModel.Post(User.Identity.GetUserId());
    _context.Employees.Add(_employee);
    _context.SaveChanges();

    return RedirectToAction("Index", "Home");
}

The view model looking like this:

public class EmployeeFormViewModel
{
     Employee Employee { get; set; }
     DateTime Date { get; set; }
     DateTime Time { get; set; }
    public void Post(int empid)
    {
        Employee= new Employee
        {
            EmployeeID = empid,
            DateTime = DateTime.Parse(string.Format("{0} {1}", Date, Time))
        };
        return;

    }
}

This is all possible because of the nice MVC Binding engine which generates the EmployeeFormViewModel based on query strings, prior to calling the action method.

I put a "Post" method in all of my ViewModels and let MVC do the work.

Collectives™ on Stack Overflow

Remove functionality from ASP.NET MVC controller

4 Answers 4

Comments

Comments

2 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

Comments

Comments

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related