If I'm constantly rebuilding my viewmodel after posts, is that indicative of a major design flaw?

Question

I've been building a new ASP.NET MVC 5 application for quite a while now, and now that I'm on the final leg of the build of my application, something is very apparent to me. I have several post calls in my controllers and after each one, after validation is done, I follow PRG architecture which then allows me to pass some parameters to the URL for a GET request to take place that will then rebuild my ViewModel with the info I retrieved from the post as well as any info saved in the database, info hiding in construtors, etc. and I was curious, is this really the way things should be done?

My ViewModel is kind of complex, utilizing lists of Models to populate dropdownlists in my view, and I also have other models as variables in my viewmodel which I then populate to use appropriately in my application.

Of course, I have about 5 DropDownLists on one of my main views, and every time I do a post, I have to follow PRG and then call the repository layer to repopulate all of the dropdownlists again, even though they are the same info every time. I just figure there has to be a better way, but maybe I'm wrong.

Code samples:

public ViewResult RgaByRgaNumber(string strRgaNumber)
{
    var vm = new ReturnGoodsAuthorizationViewModel()
    {
        RGA = _returnGoodsAuthorizationRepository.GetSpecificByRGANumber(strRgaNumber),
        RGAItems = _returnGoodsAuthorizationItemRepository.GetByRgaNumber(strRgaNumber),
        ReturnGoodsAuthorizations = _returnGoodsAuthorizationRepository.GetAll(),
        ReasonCodes = _reasonCodeRepository.GetAll(),
        RestockFeeOptions = _restockFeeOptionRepository.GetAll(),
        Customers = _customerRepository.GetAll(),
        IsUserAllowedToClickOpenCloseRGAButton = _returnGoodsAuthorizationRepository.GetUserAllowedToClickOpenCloseRGAButtonStatus(User.Identity.NameWithoutDomain())
    };
    vm.SubmitButtonText = vm.GetSubmitButtonText();
    vm.RGAClosedOpenStatusText = vm.GetRgaClosedOpenStatusText();
    vm.Customer = vm.Customers.First(x => x.CustomerId == vm.RGA.CustomerNumber);
    vm.Items = _itemRepository.GetAll(vm.RGA.CustomerNumber);
    return View("Index", vm);
}

public ViewResult Index()
{
    var vm = new ReturnGoodsAuthorizationViewModel()
    {
        ReturnGoodsAuthorizations = _returnGoodsAuthorizationRepository.GetAll(),
        ReasonCodes = _reasonCodeRepository.GetAll(),
        RestockFeeOptions = _restockFeeOptionRepository.GetAll(),
        Customers = _customerRepository.GetAll(),
        RGA = new ReturnGoodsAuthorization()
        {
            PreparedByUser = User.Identity.NameWithoutDomain().ToUpper(),
            AuthorizedByUser = User.Identity.NameWithoutDomain().ToUpper(),
            CreateUser = User.Identity.NameWithoutDomain().ToUpper()
        }
    };
    return View("Index", vm);
}

public ViewResult GetCustomerItemList(string strCustomerNumber)
{
    var vm = new ReturnGoodsAuthorizationViewModel()
    {
        ReturnGoodsAuthorizations = _returnGoodsAuthorizationRepository.GetAll(),
        ReasonCodes = _reasonCodeRepository.GetAll(),
        RestockFeeOptions = _restockFeeOptionRepository.GetAll(),
        Customers = _customerRepository.GetAll(),
        RGA = new ReturnGoodsAuthorization()
        {
            PreparedByUser = User.Identity.NameWithoutDomain().ToUpper(),
            AuthorizedByUser = User.Identity.NameWithoutDomain().ToUpper(),
            CreateUser = User.Identity.NameWithoutDomain().ToUpper()
        }
    };
    vm.RGA.CustomerNumber = strCustomerNumber;
    vm.Customer = vm.Customers.First(x => x.CustomerId == strCustomerNumber);
    vm.Items = _itemRepository.GetAll(strCustomerNumber);
    Session["ReturnGoodsAuthorizationViewModel"] = vm;
    return View("Index", vm);
}

 public ViewResult GetItemPrice(string strCustomerNumber, string strItemNumber)
        {
            if (Session != null && Session["ReturnGoodsAuthorizationViewModel"] != null)
            {
                var vm = (ReturnGoodsAuthorizationViewModel)Session["ReturnGoodsAuthorizationViewModel"];
                vm.ReturnGoodsAuthorizations = _returnGoodsAuthorizationRepository.GetAll();
                vm.ReasonCodes = _reasonCodeRepository.GetAll();
                vm.RestockFeeOptions = _restockFeeOptionRepository.GetAll();
                vm.Customers = _customerRepository.GetAll();
                vm.RGA.CustomerNumber = strCustomerNumber;
                vm.Items = _itemRepository.GetAll(strCustomerNumber);
                vm.Item = vm.Items.First(m => m.Number == strItemNumber);
                vm.SubmitButtonText = vm.GetSubmitButtonText();
                vm.RGAClosedOpenStatusText = vm.GetRgaClosedOpenStatusText();
                vm.IsUserAllowedToClickOpenCloseRGAButton = _returnGoodsAuthorizationRepository.GetUserAllowedToClickOpenCloseRGAButtonStatus(User.Identity.NameWithoutDomain());
                Session["ReturnGoodsAuthorizationViewModel"] = vm;
                return View("Index", vm);
            }
            else
            {
                var vm = new ReturnGoodsAuthorizationViewModel()
                {
                    ReturnGoodsAuthorizations = _returnGoodsAuthorizationRepository.GetAll(),
                    ReasonCodes = _reasonCodeRepository.GetAll(),
                    RestockFeeOptions = _restockFeeOptionRepository.GetAll(),
                    Customers = _customerRepository.GetAll(),
                    RGA = new ReturnGoodsAuthorization()
                    {
                        PreparedByUser = User.Identity.NameWithoutDomain().ToUpper(),
                        AuthorizedByUser = User.Identity.NameWithoutDomain().ToUpper(),
                        CreateUser = User.Identity.NameWithoutDomain().ToUpper()
                    }

                };
                vm.IsUserAllowedToClickOpenCloseRGAButton = _returnGoodsAuthorizationRepository.GetUserAllowedToClickOpenCloseRGAButtonStatus(User.Identity.NameWithoutDomain().ToUpper());
                vm.RGA.CustomerNumber = strCustomerNumber;
                vm.Item = _itemRepository.GetItem(strCustomerNumber, strItemNumber);
                vm.Items = _itemRepository.GetAll(strCustomerNumber);
                Session["ReturnGoodsAuthorizationViewModel"] = vm;
                return View("Index", vm);
            }
        }

After doing things to the model in the `POST`, [save it to `TempData["whatever"]`](https://stackoverflow.com/a/1500485/11683) and redirect, then read it from the `GET` after the redirect. — GSerg, Mar 07 '19 at 14:40
I think this question is slightly opinion based and may be better suited for the code review stack exchange but anyway, the first thing that jumps out at me is you hit the database 8 times for your `RgaByRgaNumber` action. this is going to be pretty bad performance-wise and I'd try to find ways around this. one option of many would be to store data in hidden fields when passing data between actions and posting it back to the controller so you don't need to re-retrieve it — GregH, Mar 07 '19 at 14:43
also you mention your dropdown options are the same every time, so you don't need to re-retrieve them from the db every time. again, there are many options to avoid this but a couple would be utilizing cacheing or session/temp data — GregH, Mar 07 '19 at 14:45

score 0 · Answer 1 · answered Mar 07 '19 at 15:00

The first thing I would suggest is to start making use of application level cache. Start by reading here : https://learn.microsoft.com/en-us/dotnet/framework/performance/caching-in-net-framework-applications

Any data that is generic, not specific to any user can be cached at that level and then you simply return it from the cache, minimizing the numbwr of calls to the database and improving the application's execution speed. So basically in Application_Start, this is where you load static data for your dropdowns, statuses, things like that. You cache that data as soon as the application starts and you only load it from cache when you need to.

These 5 drop downs you are talking about fit into this category. I don't really understand why you think a simple pattern is forcing you to reload data. We, as developers need some common sense. If it doesn't feel right, then don't do it, regardless of what a pattern says. Patterns are there to help, not force you to do the wrong thing, repeatedly.

Next start looking at your code, do not call the same method over and over. An example would be this code:

public ViewResult Index()
{
    var vm = new ReturnGoodsAuthorizationViewModel()
    {
        ReturnGoodsAuthorizations = _returnGoodsAuthorizationRepository.GetAll(),
        ReasonCodes = _reasonCodeRepository.GetAll(),
        RestockFeeOptions = _restockFeeOptionRepository.GetAll(),
        Customers = _customerRepository.GetAll(),
        RGA = new ReturnGoodsAuthorization()
        {
            PreparedByUser = User.Identity.NameWithoutDomain().ToUpper(),
            AuthorizedByUser = User.Identity.NameWithoutDomain().ToUpper(),
            CreateUser = User.Identity.NameWithoutDomain().ToUpper()
        }
    };
    return View("Index", vm);
}

with a a very simple change it can look like this:

public ViewResult Index()
{
    string username = User.Identity.NameWithoutDomain().ToUpper();

    var vm = new ReturnGoodsAuthorizationViewModel()
    {
        ReturnGoodsAuthorizations = _returnGoodsAuthorizationRepository.GetAll(),
        ReasonCodes = _reasonCodeRepository.GetAll(),
        RestockFeeOptions = _restockFeeOptionRepository.GetAll(),
        Customers = _customerRepository.GetAll(),
        RGA = new ReturnGoodsAuthorization()
        {
            PreparedByUser = username,
            AuthorizedByUser = username,
            CreateUser = username
        }
    };
    return View("Index", vm);
}

then look at the code a bit more and start asking questions like :

do you really need to load all customers?
does a GetItemPrice method need to call the database 10 times to load so much data?

If I'm constantly rebuilding my viewmodel after posts, is that indicative of a major design flaw?

1 Answers1