1
\$\begingroup\$

I rewrote a ActionResult method that will output list of users depending if it is a SuperAdmin, OrganisationAdmin or GroupAdmin. I know there is a lot of things in the codes I still can improve so I would ask the community here the things I should learn and keep in mind when writing codes.

You might ask why I have parameters on the method below is because I have a filtering selection in the view for this method which I have to resubmit whenever I have to get a different list from a specific organisation.

Also I changed some names in the codes which is different from the real code that we have.

public ActionResult Index(UserManagementViewModel model, int page = 1)
{
    model.CurrentAccountId = CurrentAccount.Id;
    model.IsSuperAdmin = CurrentAccount.HasRole(RoleNames.SuperAdmin);
    model.IsGroupAdmin = CurrentAccount.HasRole(RoleNames.GroupAdmin);
    model.IsOrganisationalAdmin = CurrentAccount.HasRole(RoleNames.OrganisationalAdmin);

    //permission
    if (CurrentOrganisation != null && (model.IsSuperAdmin || model.IsOrganisationalAdmin))
    {
        if(CurrentOrganisation.Permission1)
        {
            model.Permission1 = true;
            //get list of bulk link
            var bulks = _bulkRepository.GetBulkForOrganisation(CurrentOrganisation.Id);
            model.BulkList = Mapper.Map<List<BulkViewModel>>(bulks);
        }

        if (CurrentOrganisation.Permission2)
        {
            model.Permission2 = true;
        }
    }

    if (model.IsSuperAdmin)
    {
        var organisations = _organisationRepository.GetAll().OrderBy(i => i.Name);
        var users = AccountManager.Users.Where(a => !a.Deleted).OrderBy(a => a.LastName);

        var activeUsers = users.Where(u => u.Organisation.IsClient).Paged(page, 20);
        var trialUsers = users.Where(u => !u.Organisation.IsClient).Paged(page, 20);

        var organisationId = model.CurrentOrganisation;
        if (organisationId != 0)
        {

            activeUsers = users.Where(u => u.Organisation.Id == organisationId && u.Organisation.IsClient).Paged(page, 20);
            trialUsers = users.Where(u => u.Organisation.Id == organisationId && !u.Organisation.IsClient).Paged(page, 20);
        }

        var activeUserList = Mapper.Map<List<UserViewModel>>(activeUsers);
        var trialUserList = Mapper.Map<List<UserViewModel>>(trialUsers);

        model.ActiveUserPageList = new PagedList<UserViewModel>(activeUserList, page, 20, c => activeUsers.TotalItemCount);
        model.TrialUserPageList = new PagedList<UserViewModel>(trialUserList, page, 20, c => activeUsers.TotalItemCount);

        model.ClientOrganisationList = organisations.Where(i => i.IsClient).Select(i => 
            new SelectListItem
            {
                Text = i.Name,
                Value = i.Id.ToString(),
                Selected = i.Id.ToString() == organisationId.ToString()
            }
        ).ToList();

        model.NonClientOrganisationList = organisations.Where(i => !i.IsClient).Select(i =>
            new SelectListItem
            {
                Text = i.Name,
                Value = i.Id.ToString(),
                Selected = i.Id.ToString() == organisationId.ToString()
            }
        ).ToList();

        return View(model); // immediately return the result
    }

    var organisationUsers = AccountManager.Users.Where(u => u.Organisation.Id == CurrentOrganisation.Id 
                                                            && !u.Deleted).OrderBy(a => a.LastName);

    if (model.IsOrganisationalAdmin)
    {
        var users = organisationUsers.Where(u => u.Status == Status.Approved).Paged(page, 20);
        var userrequests = organisationUsers.Where(u => u.Status == Status.Pending).Paged(page, 20);

        //initialize all neccessary properties for IA
        model.OrganisationName = CurrentOrganisation.Name;
        model.UsedAccounts = users.TotalItemCount;
        model.AllowedAccounts = CurrentOrganisation.Licence.AllowedAccounts;

        var userList = Mapper.Map<List<UserViewModel>>(users);
        var userRequestList = Mapper.Map<List<UserViewModel>>(userrequests);

        model.ActiveUserPageList = new PagedList<UserViewModel>(userList, page, 20, c => users.TotalItemCount);
        model.UserRequestPageList = new PagedList<UserViewModel>(userRequestList, page, 20, c => userrequests.TotalItemCount);

        return View(model);
    }

    model.GroupAdmin = new GroupAdmin();
    model.GroupAdmin.CanCreateNewUser = CurrentAccount.CanCreateNewUser;
    model.GroupAdmin.CanViewAllUsers = CurrentAccount.CanviewAllUsers;

    if (model.IsGroupAdmin && CurrentAccount.CanViewAllUsers)
    {
        var users = organisationUsers.Where(u => u.Status == Status.Approved).Paged(page, 20);
        var userList = Mapper.Map<List<UserViewModel>>(users);

        model.ActiveUserPageList = new PagedList<UserViewModel>(userList, page, 20, c => users.TotalItemCount);
    }

    if (model.IsGroupAdmin && !CurrentAccount.CanViewAllUsers)
    {
        var currentAccount = organisationUsers.FirstOrDefault(u => u.Id == CurrentAccount.Id);
        var groupAccountIds = currentAccount.Groups.Where(g => !g.IsSystem)
                                                    .SelectMany(g => g.Accounts)
                                                    .Select(u => u.Id)
                                                    .ToList();

        if(groupAccountIds.Any() && !groupAccountIds.Contains(CurrentAccount.Id))
        {
            groupAccountIds.Add(CurrentAccount.Id);
        }

        var users = organisationUsers.Where(u => groupAccountIds.Contains(u.Id)).Paged(page, 20);
        var userList = Mapper.Map<List<UserViewModel>>(users);

        model.ActiveUserPageList = new PagedList<UserViewModel>(userList, page, 20, c => users.TotalItemCount);
    }
    return View(model);
}
\$\endgroup\$
2
\$\begingroup\$

This method is huge and it does a lot. It is responsible for each type of view. This is too much.

You should refactor it so that each if that returns a view is a separate method returning either a null or a view so that you can chain them like this:

// build the model
model.CurrentAccountId = CurrentAccount.Id;
model.IsSuperAdmin = CurrentAccount.HasRole(RoleNames.SuperAdmin);
model.IsGroupAdmin = CurrentAccount.HasRole(RoleNames.GroupAdmin);
model.IsOrganisationalAdmin = CurrentAccount.HasRole(RoleNames.OrganisationalAdmin);

// prepare other suff...


return
    GetSuperAdminView(model) ??
    GetOrganisationalAdminView(model) ??
    ... other views
    return View(model); // default view

The first one that returns a view will be used or otherwise the last default view will be returned.

\$\endgroup\$
  • 2
    \$\begingroup\$ You might want to point questions of this nature to the Wikipedia definition of the Single Responsibility Principal en.wikipedia.org/wiki/Single_responsibility_principle. \$\endgroup\$ – pacmaninbw Aug 28 '16 at 16:37
  • 2
    \$\begingroup\$ @pacmaninbw this is a good idea - next time ;-) I really need to create a notebook with such links so that I don't have to google them each time. \$\endgroup\$ – t3chb0t Aug 28 '16 at 16:40

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.