4
\$\begingroup\$

The code below is the Index Action from a User Controllers. Basically this code returns a list of users from Azure Active Directory using Azure Authentication Library (ADAL).

However I need to show 2 custom properties, in Azure AD, they are called Schema Extensions, they are just custom attributes where I saved custom data in a comma delimited format. On the first column I have the IDs of the companies the user works for, and on the second column I have the IDs of the Modules the user has access to.

The index controller action is easy:

public async Task<ActionResult> Index()
        {
            var userList = new List<Microsoft.Azure.ActiveDirectory.GraphClient.User>();
            try
            {
                var client = AuthenticationHelper.GetActiveDirectoryClient();
                var pagedCollection = await client.Users.ExecuteAsync();
                while (pagedCollection != null)
                {
                    var usersList = pagedCollection.CurrentPage.ToList();
                    userList.AddRange(usersList.Cast<Microsoft.Azure.ActiveDirectory.GraphClient.User>());
                    pagedCollection = await pagedCollection.GetNextPageAsync();
                }
            }
            catch (Exception)
            {
                if (Request.QueryString["reauth"] == "True")
                {
                    //
                    // Send an OpenID Connect sign-in request to get a new set of tokens.
                    // If the user still has a valid session with Azure AD, they will not be prompted for their credentials.
                    // The OpenID Connect middleware will return to this controller after the sign-in response has been handled.
                    //
                    HttpContext.GetOwinContext()
                        .Authentication.Challenge(OpenIdConnectAuthenticationDefaults.AuthenticationType);
                }

                //
                // The user needs to re-authorize.  Show them a message to that effect.
                //
                ViewBag.ErrorMessage = "AuthorizationRequired";
                return View(userList);
            }
            return View(userList);
        }

However getting extended properties is a little complex; I know how to get them, and I know how to translated those IDS to the Names I want to show in the grid, however I am making a call to the database (see UnitOFWork and getobjectbyid method), for each ID for each row.

Obviously this won't scale well if I have 1000 users.

How can I rewrite the View to make the calls to the database once for all rows and all ids? And then when the view is rendering each row, it should have the value somewhere in memory instead of doing a DB Call?

@model IEnumerable<Microsoft.Azure.ActiveDirectory.GraphClient.User>

@{
    ViewBag.Title = "Index";
    Layout = "~/Areas/GlobalAdmin/Views/Shared/_LayoutGlobalAdmin.cshtml";
    var extPropLookupNameCompania = string.Format("extension_{0}_{1}", SettingsHelper.ClientId.Replace("-", ""), "Compania");
    var extPropLookupNameModulos = string.Format("extension_{0}_{1}", SettingsHelper.ClientId.Replace("-", ""), "Modulos");

    var unitOfWork = new UnitOfWork();

}
<h2>Usuarios</h2>

<div class="wrapper wrapper-content animated fadeInRight">
    <div class="row">
        <div class="col-lg-12">
            <div class="ibox float-e-margins">
                <div class="ibox-title">
                    <h5>Lista de Usuarios</h5>
                    <div class="ibox-tools">
                        @Html.ActionLink("Create New", "Create", null, new { @class = "btn btn-primary btn-xs" })
                    </div>
                </div>
                <div class="ibox-content">

                    <table id="directoryObjects" class="table table-bordered table-striped">
                        <tr>
                            <th>
                                UserPrincipalName
                            </th>
                            <th>
                                DisplayName
                            </th>
                            <th>
                                JobTitle
                            </th>
                            <th>
                                Empresa
                            </th>
                            <th>
                                Modulos
                            </th>
                            <th />
                        </tr>
                        @foreach (var item in Model)
                        {
                            var user = item as User;
                        <tr>
                            <td>
                                @Html.DisplayFor(modelItem => user.UserPrincipalName)
                            </td>
                            <td>
                                @Html.DisplayFor(modelItem => user.DisplayName)
                            </td>
                            <td>
                                @Html.DisplayFor(modelItem => user.JobTitle)
                            </td>
                            <td>
                                @{
                                    var keyvaluepairCompanias = user.GetExtendedProperties().FirstOrDefault(prop => prop.Key == extPropLookupNameCompania);
                                    if (keyvaluepairCompanias.Value != null)
                                    {
                                        var idsEmpresas = keyvaluepairCompanias.Value.ToString().Split(',');
                                        var empresas = string.Empty;
                                        foreach (var id in idsEmpresas)
                                        {
                                            var empresa = unitOfWork.EmpresaRepository.GetById(Convert.ToInt32(id));
                                            if (empresa != null)
                                            {
                                                @Html.Label(empresa.Nombre)
                                                ;
                                                <br/>
                                            }
                                        }
                                    }
                                    else
                                    {
                                        @Html.Label(string.Empty)
                                        ;
                                    }
                                }
                            </td>
                            <td>
                                @{
                                    var keyvaluepairModulos = user.GetExtendedProperties().FirstOrDefault(prop => prop.Key == extPropLookupNameModulos);
                                    if (keyvaluepairModulos.Value != null)
                                    {
                                        var idsModulos = keyvaluepairModulos.Value.ToString().Split(',');
                                        var modulos = string.Empty;
                                        foreach (var id in idsModulos)
                                        {
                                            var modulo = unitOfWork.ModuloRepository.GetById(Convert.ToInt32(id));
                                            if (modulo != null)
                                            {
                                                @Html.Label(modulo.Nombre);
                                                <br />
                                            }
                                        }
                                    }
                                    else
                                    {
                                        @Html.Label(string.Empty);
                                    }
                                }
                            </td>
                            <td>
                                @Html.ActionLink("Editar", "Edit", new { objectId = item.ObjectId }) <br />
                                @Html.ActionLink("Detalles", "Details", new { objectId = item.ObjectId }) <br />
                                @Html.ActionLink("Eliminar", "Delete", new { objectId = item.ObjectId })  <br />
                            </td>
                        </tr>
    }
                    </table>
                </div>
            </div>
        </div>
    </div>
</div>
\$\endgroup\$
4
\$\begingroup\$

Talking about public async Task<ActionResult> Index()

While looking at this

while (pagedCollection != null)
{
    var usersList = pagedCollection.CurrentPage.ToList();
    userList.AddRange(usersList.Cast<Microsoft.Azure.ActiveDirectory.GraphClient.User>());
    pagedCollection = await pagedCollection.GetNextPageAsync();
}  

I thought what is the sense of getting the userList and then calling AddRange() passing itself as parameter. After looking more closely I could spot the tiny little difference between usersList and userList.

Because it happened to me, it will also happen for Sam the maintainer of this code. Sam would be happy if one of these variables would get a different name so it could be seen at first glance that these variables are different.


Instead of comparing Request.QueryString["reauth"] to the string "True" you should take advantage of the bool.TrueString property like so

if (Request.QueryString["reauth"] == bool.TrueString)  

        return View(userList);
    }
    return View(userList);  

the first return is superfluous and does not add any value, hence it should be removed.


\$\endgroup\$
  • \$\begingroup\$ I disagree regarding the removal of the superfluous return. It does have value in describing program logic and where code should exit. If Sam the maintainer needs to alter the return on an exception condition, he knows where that needs to happen and can make the change without affecting the fall through exit code. Likewise, having the extra return allows code after the fall through to be modified without affecting the exception handling. IMO it is better to have a superfluous return for clarity than the most efficient code that requires more effort on the part of Sam the maintainer. \$\endgroup\$ – Anthony Fawcett Nov 24 '15 at 23:02

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.