Is it good practice to pass TempData and/or ViewDa

Question

In trying to reduce the size of my MVC controllers, I am refactoring a lot of logic out into services (though this same issue would apply if it was being factored into models). I'm often finding that I was directly setting ViewData and/or TempData with information I wanted to be displayed to the user, like:

var repoUser = new UserRepository();
var foundUser = repoUser.GetUser(userId);
if (foundUser == null) {
    ViewData["ErrorMessage"] = "Could not find user with ID {0}.".FormatWith(userId);
    return View("UsersRegister");
}

Of course, as soon as you move into a service class, you lose direct access to ViewData, TempData, and methods like View() or RedirectToAction(), so I'm trying to figure out the best practice for dealing with it. Two solutions come to mind:

1. Create a class containing various pieces of information about what the controller should do, to be passed back from the services, like:

   public class ResponseInfo {
       public string Name { get; set; }
       public bool DoRedirect { get; set; }
       public object RedirectRouteValues { get; set; }
       public string InfoMessage { get; set; }
       public string ErrorMessage { get; set; }
   }
   

2. Try and allow the service methods to have direct access to things like ViewData and TempData, eg.:

   public ActionResult ToggleUserAttended(int eventId, int userId, HttpRequestBase request, TempDataDictionary tempData, ViewDataDictionary viewData, Func<string, object, ActionResult> redirectAction) {
       //...
       var repoUser = new UserRepository();
       var foundUser = repoUser.GetUser(userId);
       if (foundUser == null) {
           tempData["ErrorMessage"] = "Could not find user with ID {0}.".FormatWith(userId);
           return redirectAction("UsersRegister", new { id = eventId, returnUrl = request.QueryString["returnUrl"] });
       }
       //...
   }
   

   ... and then in the controller:

   return _svcEvent.ToggleUserAttended(123, 234, Request, TempData, ViewData, (name, routeVals) => RedirectToAction(name, routeVals));
   

For number 1, the controller would have more logic to do in looking at the ResponseInfo object and determining whether to redirect, display a view, plugging the error or info message into TempData or ViewData, etc. Number 2 would pretty much allow a one-liner controller, but you're making the service very aware of controller-specific stuff. However, the service is always going to be closely tied in with the controller anyway so is this a problem? Would 1 or 2 be best practice, or something else I haven't listed?

Answer 1

Maybe I misunderstood something, but I find it strange that you want to provide information related to presentation concerns to the services. IMO, it is a very bad idea to do that as services are business logic and should not depend on presentation layer.

I don't really like neither of approaches you suggested as it seems to me that they blur the line between presentation and services.

As an example, a service should throw if it can't find the user, and the controller should then handle this error with appropriate UI mechanism: an error message, HTTP error or whatever fits your app. Similarly, how can the service know where to redirect? It is aware of GUI? How do you plan to use it in other contexts (API, non-web GUI, whatever)?

What I usually do is create a command/dto based on form/parameters and provide that to service. Then any service client can use the same command. If I need to show data, I ask services/repos for whatever I need, and map that to presentation form (if needed) and put that into ViewData/TempData/Model/Whatever.

Here are some examples (treat it as pseudo-code):

[HttpPost]
public ActionResult ChangeUserInfo(UserInfoModel model)
{
  var cmd = new ChangeUserInfoCommand(CurrentUserId, model.FirstName, model.LastName);
  try {
      userSvc.UpdateUser(cmd); 
      return RedirectToAction(...);
  }
  catch(XxxxException e) { // e.g. something wrong (business logic)
      TempData["Error"] = "an error occurred: " + e.FriendlyError();
      return RedirectToAction(...); // error action
  }
  catch(SecurityException e) {
      throw new HttpException(404, "NotFound");
  }
}

public ActionResult ShowUsers()
{
  var userInfo = svc.GetUsers().Select(u => { Id = u.id, FullName = u.First + " " + u.Last);
  // or var userInfo = svc.GetUsers().Select(u => Mapper.To<UserDTO>(u));
  return View(userInfo);

  // or without model
  // ViewData["users"] = userInfo;
  // return View();
}

Please note that this is an illustration - I usually do it quite differently, but I have a lot of other infrastructure in place (e.g. I don't explicitly handle exceptions like this, my services sometimes have only one method Execute(IEnumerable<Command> cmds), I have nice support for anonymous classes as view models, etc)