4
\$\begingroup\$

I have a MVVM application that fetches data from active directory and displays it in a DataGrid. The exact data displayed is determined by which query a user runs, and there are multiple available queries. Each query requires slightly different logic, but they all have the same general idea. These methods are all in my ViewModel. Data is a property that is bound to the DataGrid in the View.

If you would like to see any additional code for this review, please let me know. I am attempting to divide this project (several thousand lines long) into reasonably sized chunks for review. These methods are contributing greatly to the length, especially considering I intend on adding even more query options in the future. As of now, the whole class is 1000 lines long, with these methods being close to 400 lines.

private async void GetComputerGroupsCommandExecute()
{
    StartTask();
    QueryType = QueryType.ContextualComputerGroups;
    await Task.Run(() =>
    {
        var computerPrincipal = ComputerPrincipal.FindByIdentity(
            _principalContext, GetSelectedComputerDistinguishedName());
        _dataPreparer = new DataPreparer
        {
            Data = new[]
            {
                ActiveDirectorySearcher.GetComputerGroups(
                    computerPrincipal)
            },
            Attributes = DefaultComputerGroupsAttributes.ToList()
        };
        try
        {
            Data = new List<ExpandoObject>(_dataPreparer.GetResults())
                .ToDataTable().AsDataView();
        }
        catch (ArgumentNullException)
        {
            ShowMessage(
                "No groups found for the selected computer.");
        }
    });
    FinishTask();
}

private async void GetDirectReportDirectReportsCommandExecute()
{
    StartTask();
    QueryType = QueryType.ContextualDirectReportDirectReports;
    await Task.Run(() =>
    {
        var directReportUserPrincipal = UserPrincipal.FindByIdentity(
            _principalContext,
            GetSelectedDirectReportDistinguishedName());
        _dataPreparer = new DataPreparer
        {
            Data = new[]
            {
                ActiveDirectorySearcher.GetUserDirectReports(
                    directReportUserPrincipal)
            },
            Attributes = DefaultUserDirectReportsAttributes.ToList()
        };
        try
        {
            Data = new List<ExpandoObject>(_dataPreparer.GetResults())
                .ToDataTable().AsDataView();
        }
        catch (ArgumentNullException)
        {
            ShowMessage(
                "No direct reports found for the selected user.");
        }
    });
    FinishTask();
}

private async void GetDirectReportGroupsCommandExecute()
{
    StartTask();
    QueryType = QueryType.ContextualDirectReportDirectReports;
    await Task.Run(() =>
    {
        var directReportUserPrincipal = UserPrincipal.FindByIdentity(
            _principalContext,
            GetSelectedDirectReportDistinguishedName());
        _dataPreparer = new DataPreparer
        {
            Data = new[]
            {
                ActiveDirectorySearcher.GetUserGroups(
                    directReportUserPrincipal)
            },
            Attributes = DefaultUserGroupsAttributes.ToList()
        };
        try
        {
            Data = new List<ExpandoObject>(_dataPreparer.GetResults())
                .ToDataTable().AsDataView();
        }
        catch (ArgumentNullException)
        {
            ShowMessage(
                "No groups found for the selected direct report.");
        }
    });
    FinishTask();
}

private async void GetGroupComputersCommandExecute()
{
    StartTask();
    QueryType = QueryType.ContextualGroupComputers;
    await Task.Run(() =>
    {
        var groupPrincipal = GroupPrincipal.FindByIdentity(
            _principalContext,
            GetSelectedGroupDistinguishedName());
        _dataPreparer = new DataPreparer
        {
            Data = new[]
            {
                ActiveDirectorySearcher.GetComputersFromGroup(
                    groupPrincipal)
            },
            Attributes = DefaultGroupComputersAttributes.ToList()
        };
        try
        {
            Data = new List<ExpandoObject>(_dataPreparer.GetResults())
                .ToDataTable().AsDataView();
        }
        catch (ArgumentNullException)
        {
            ShowMessage(
                "No groups found for the selected direct report.");
        }
    });
    FinishTask();
}

private async void GetGroupUsersDirectReportsCommandExecute()
{
    StartTask();
    QueryType = QueryType.ContextualGroupUsersDirectReports;
    await Task.Run(() =>
    {
        var groupPrincipal = GroupPrincipal.FindByIdentity(
            _principalContext,
            GetSelectedGroupDistinguishedName());
        _dataPreparer = new DataPreparer
        {
            Data = new[]
            {
                ActiveDirectorySearcher.GetUsersDirectReports(
                    groupPrincipal)
            },
            Attributes = DefaultGroupUsersDirectReportsAttributes
                .ToList()
        };
        try
        {
            Data = new List<ExpandoObject>(_dataPreparer.GetResults())
                .ToDataTable().AsDataView();
        }
        catch (ArgumentNullException)
        {
            ShowMessage(
                "No users and/or direct reports found in the selected group.");
        }
    });
    FinishTask();
}

private async void GetOuComputersCommandExecute()
{
    StartOuTask();
    QueryType = QueryType.OrganizationalUnitComputers;
    await Task.Run(() =>
    {
        _dataPreparer = new DataPreparer
        {
            Data = _activeDirectorySearcher.GetOuComputers(),
            Attributes = DefaultComputerAttributes.ToList()
        };
        try
        {
            Data = new List<ExpandoObject>(_dataPreparer.GetResults())
                .ToDataTable().AsDataView();
        }
        catch (ArgumentNullException)
        {
            ShowMessage("No computers found in selected OU.");
        }
    });
    FinishTask();
}

private async void GetOuGroupsCommandExecute()
{
    StartOuTask();
    QueryType = QueryType.OrganizationalUnitGroups;
    await Task.Run(() =>
    {
        _dataPreparer = new DataPreparer
        {
            Data = _activeDirectorySearcher.GetOuGroups(),
            Attributes = DefaultGroupAttributes.ToList()
        };
        try
        {
            Data = new List<ExpandoObject>(_dataPreparer.GetResults())
                .ToDataTable().AsDataView();
        }
        catch (ArgumentNullException)
        {
            ShowMessage("No groups found in selected OU.");
        }
    });
    FinishTask();
}

private async void GetOuUsersCommandExecute()
{
    StartOuTask();
    QueryType = QueryType.OrganizationalUnitUsers;
    await Task.Run(() =>
    {
        _dataPreparer = new DataPreparer
        {
            Data = _activeDirectorySearcher.GetOuUsers(),
            Attributes = DefaultUserAttributes.ToList()
        };
        try
        {
            Data = new List<ExpandoObject>(_dataPreparer.GetResults())
                .ToDataTable().AsDataView();
        }
        catch (ArgumentNullException)
        {
            ShowMessage("No users found in selected OU.");
        }
    });
    FinishTask();
}

private async void GetOuUsersDirectReportsCommandExecute()
{
    StartOuTask();
    QueryType = QueryType.OrganizationalUnitUsersDirectReports;
    await Task.Run(() =>
    {
        _dataPreparer = new DataPreparer
        {
            Data = _activeDirectorySearcher.GetOuUsersDirectReports(),
            Attributes = DefaultUserDirectReportsAttributes.ToList()
        };
        try
        {
            Data = new List<ExpandoObject>(_dataPreparer.GetResults())
                .ToDataTable().AsDataView();
        }
        catch (ArgumentNullException)
        {
            ShowMessage(
                "No users and/or direct reports found in selected OU.");
        }
    });
    FinishTask();
}

private async void GetOuUsersGroupsCommandExecute()
{
    StartOuTask();
    QueryType = QueryType.OrganizationalUnitUsersGroups;
    await Task.Run(() =>
    {
        _dataPreparer = new DataPreparer
        {
            Data = _activeDirectorySearcher.GetOuUsersGroups(),
            Attributes = DefaultUserGroupsAttributes.ToList()
        };
        try
        {
            Data = new List<ExpandoObject>(_dataPreparer.GetResults())
                .ToDataTable().AsDataView();
        }
        catch (ArgumentNullException)
        {
            ShowMessage(
                "No users and/or groups found in selected OU.");
        }
    });
    FinishTask();
}

private async void GetUserGroupsCommandExecute()
{
    StartTask();
    QueryType = QueryType.ContextualUserGroups;
    await Task.Run(() =>
    {
        var userPrincipal = UserPrincipal.FindByIdentity(
            _principalContext, GetSelectedUserDistinguishedName());
        _dataPreparer = new DataPreparer
        {
            Data = new[]
            {
                ActiveDirectorySearcher.GetUserGroups(
                    userPrincipal)
            },
            Attributes = DefaultUserGroupsAttributes.ToList()
        };
        try
        {
            Data = new List<ExpandoObject>(_dataPreparer.GetResults())
                .ToDataTable().AsDataView();
        }
        catch (ArgumentNullException)
        {
            ShowMessage(
                "No groups found for the selected user.");
        }
    });
    FinishTask();
}

The StartTask, StartOuTask and FinishTask methods are as follows:

private void StartOuTask()
{
    StartTask();
    _activeDirectorySearcher = new ActiveDirectorySearcher(
        CurrentScope);
}

private void StartTask()
{
    HideMessage();
    ViewIsEnabled = false;
    ProgressBarVisibility = Visibility.Visible;
}

private void FinishTask()
{
    ContextMenu = GenerateContextMenu();
    ProgressBarVisibility = Visibility.Hidden;
    ViewIsEnabled = true;
}
\$\endgroup\$
2
  • 4
    \$\begingroup\$ Oh, the copy&paste design pattern ;-) \$\endgroup\$ – t3chb0t Jul 4 '16 at 18:20
  • 2
    \$\begingroup\$ That looks better than reviewing the 1000+ code altogether \$\endgroup\$ – Tolani Jul 5 '16 at 10:09
2
\$\begingroup\$
  1. The methods should return Task instead of void. Otherwise exception are shallowed. Alternative you could encapsulate the whole content of the method in a try catch block.

See also: Asynchrone return types - Void Return Type

  1. Try to follow the DRY principle.

For example the line:

new List<ExpandoObject>(_dataPreparer.GetResults()).ToDataTable().AsDataView();

may be extracted to a method:

private static IList<ExpandoObject> GetData(DataPreparer dataPreparer) { ... }
  1. It seems that the instance variables _dataPreparer, _activeDirectorySearcher class variables are set / used from different methods. That increases the comlexity of the program flow. I would try to avoid such 'global writable states'.

The view model seems to be used as universal container with high coupling. In the medium term, I would try to move the logic of each method to a single class that is independent of the view model.

\$\endgroup\$
2
  • \$\begingroup\$ What do you mean by "exception are shallowed"? The ShowMessage method does what it is supposed to with the message. \$\endgroup\$ – Michael Brandon Morris Jul 5 '16 at 10:08
  • \$\begingroup\$ I mean, that the exception is thrown silently (you are not notified about the exception). Sure, the code within the inner try/catch block is not affected, but there is some logic before the try that may trhow exceptions. See also the link I'll add to my answer. \$\endgroup\$ – JanDotNet Jul 5 '16 at 13:55

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.