Storing Temporal Message Lists in ASP.NET MVC TempData

Question

I've been using the following the code to store temporal error messages in TempData, to be displayed from my master page on production sites without any problems, but is this a good way of doing it?

The controller helper code:

namespace System.Web.Mvc
{
 public static class Controllers
 {
 public static void SetMessage(this ControllerBase controller, string message)
 {
 List<string> messages = controller.TempData["Messages"] as List<string> ?? new List<string>();
 messages.Add(message);
 controller.TempData["Messages"] = messages;
 }
 public static void SetErrorMessage(this ControllerBase controller, string errorMessage)
 {
 List<string> messages = controller.TempData["ErrorMessages"] as List<string> ?? new List<string>();
 messages.Add(errorMessage);
 controller.TempData["ErrorMessages"] = messages;
 }
 }
}

Example Use:

public ActionResult Edit()
{
 // .. do stuff
 this.SetMessage("You edited X.");
 return View();
}

Here is the rendering code:

<%
 if (TempData["ErrorMessages"] != null)
 {
 %>
 Error Messages
 <%
 foreach (string s in TempData["ErrorMessages"] as List<string>)
 {
 %><%=s%><%
 }
 }
 if (TempData["Messages"] != null)
 {
 %>
 Messages
 <%
 foreach (string s in TempData["Messages"] as List<string>)
 {
 %><%=s%><%
 }
 }
%>

Can you spot any issues?

Answer 1

On the whole the approach is fine, but do not use TempData unless you're looking to populate this information across redirects. TempData comes with a host of issues, and its behaviour isn't quite what you think.

ViewData, on the other hand, is designed exactly for this purpose but comes with the caveat that it doesn't persist across redirects.

Therefore, if you're looking to put something in place which allows developers to push messages into your views easily, you need to expose the ability to put data in either ViewData or TempData, depending on whether they want to redirect or not.

Another option is to clear the TempData entry manually after the call has finished, but I wouldn't recommend this approach.

As a final point, your function names SetMessage and SetErrorMessage are misleading in that they imply you've got one spot only for the message. To me, as a user of your function, when doing this:

this.SetMessage("Foo");
this.SetMessage("Bar");

I would assume that my message is now set to "Bar", but behind the scenes it's a list with both "Foo" and "Bar". I'd recommend changing your function names to AddMessage and AddErrorMessage instead, which implies that behind the scenes there is a collection of elements.

• Petrus Theron: I agree the function names are misleading. AddMessage would be better. I used ViewData initially, but if you add a message in an action, and then redirect the user - the messages are lost. Without resorting to Session, TempData is meant exactly for passing that type of data around.
• OJ.: That's exactly what I said in my response :) But bear in mind that you can leave data hanging around in TempData if you don't redirect. It has side effects. Leave it in the hands of the caller to decide, provide them with both options.
• Petrus Theron: is TempData not automatically cleared between request cycles?
• OJ.: Yes it is, but you have to have a request cycle. If you add to TempData and you don't direct it will still be there when the user does their next action. This can result in undesirable behaviour.