3
\$\begingroup\$

It's really a nitty-gritty issue for me, and I have been here many times before. How could this be improved?

I have a basic viewmodel for a view:

public class LoginViewModel : BaseViewModel
{
    public User _User { get; set; }
}

My base class looks like this:

public class BaseViewModel
{
    public List<string> StateMessages { get; set; }
}

The view takes a LoginViewModel, but if I do not pass it in from the controller, then I need to check the StateMessages for null before I access it. So I pass in an empty ViewModel, which is okay, but if I could get around it, it would just look better.

[Route("login")]
[Route( "~/", Name="default" )]
public ActionResult Login()
{
    if (User.Identity.IsAuthenticated)
        return RedirectToAction("Index", "Dashboard");

    return View(new LoginViewModel());
}

Then in the view I don't have check for null, which I prefer:

@foreach (var err in Model.StateMessages)
{
    <span>@err</span><br />
}

I know this is very cosmetic, but is this just they way it should be, or can I somehow avoid to new up a new and empty ViewModel in the controller?

\$\endgroup\$
  • \$\begingroup\$ Please add more code to get a better view of your problem. Where do you do the null check ? Where and how do you pass an empty ViewModel ? I have deleted my answer so it is legit to add more code. \$\endgroup\$ – Heslacher Nov 20 '14 at 9:26
  • \$\begingroup\$ At the moment I don't do a null check anywhere. And I want to keep it than way. But I believe passing in an empty viewmodel, just to come around it, is equally a bad design. But there might not be a way around it. Ideally I would like just "return View();" and in the view, still do the above. But since the model would be null, I need to do a null check in the view, which is just ugly. \$\endgroup\$ – user3633222 Nov 20 '14 at 9:33
  • 1
    \$\begingroup\$ I don't see a problem with the empty viewmodel. It seems to better represent the situation. You have no state messages, that corresponds to an empty collection of state messages, not to a null collection. \$\endgroup\$ – Ben Aaronson Nov 20 '14 at 11:50
  • \$\begingroup\$ Thx Ben, that was actually all I needed to hear :) \$\endgroup\$ – user3633222 Nov 20 '14 at 13:17
1
\$\begingroup\$

I would suggest changing the type of StateMessages from List<string> to IEnumerable<string>.

It hides implementation details, allowing you flexibility in your choice of data structure in the future.

It also gives you more control over how client code can access/modify the state messages.

Make the backing field readonly to ensure that it's not null.

public class BaseViewModel
{
    private readonly List<string> stateMessages = new List<string>();

    public IEnumerable<string> StateMessages
    {
        get { return this.stateMessage; }
    }

    // Other methods to access/modify stateMessages.
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.