5
\$\begingroup\$

I have a subsystem in a web application that essentially allows users to build views of certain lists of objects. They can specify which columns to display, how to sort and group the list, and what filters they want to assign to the results set. These views of certain objects are fundamentally similar, but due to limitations of NHibernate I can't use generics on my domain view entities, so I have a series of classes that inherit from a ViewBase abstract class.

On the web UI side, I'm using Telerik's MVC grid to display the results, so I have to do some translation from the options present in the domain object and the settings the grid control expects. This process is where I see a lot of repeated code.

Here are two examples:

public GridModel GetOpportunityViewResults(int viewId, GridCommand command)
{
    var activeUser = UserContext.Current.GetActiveUser();

    var viewSvc = DependencyResolver.Current.GetService<IViewService<OpportunityView>>();
    var view = viewSvc.FindBy(viewId, activeUser);

    // Replace view's ordering with the command options
    var propertyTranslator = new OpportunitiesViewModelTranslator();
    view.Orders = TelerikGridHelpers.GenerateViewOrderList(propertyTranslator.TranslateToDomainProperty, command);

    // Get the results of the view
    var itemsService = DependencyResolver.Current.GetService<IOpportunityService>();
    IFutureValue<long> total;
    var results = itemsService.FindByView(view, ((command.Page - 1) * view.PageSize), command.PageSize, activeUser, out total);

    // Map the domain results to view models
    var mapper = new ViewResultsMapper();
    var viewModels = results.Select(o => mapper.MapToViewModel(o, view.VisibleProperties.ToList(), new OpportunityViewResultsRequiredProperties()));

    // Return the grid model
    return command.GroupDescriptors.Any()
               ? new GridModel
                     {
                         Data = TelerikGridHelpers.ApplyDynamicGrouping(viewModels.AsQueryable(), command.GroupDescriptors),
                         Total = Convert.ToInt32(total.Value)
                     }
               : new GridModel
                     {
                         Data = viewModels,
                         Total = Convert.ToInt32(total.Value)
                     };
}

public GridModel GetCustomerViewResults(int viewId, GridCommand command)
{
    var activeUser = UserContext.Current.GetActiveUser();

    var viewSvc = DependencyResolver.Current.GetService<IViewService<CustomerOrganizationView>>();
    var view = viewSvc.FindBy(viewId, activeUser);

    // Replace view's ordering with the command options
    var propertyTranslator = new CustomersViewModelTranslator();
    view.Orders = TelerikGridHelpers.GenerateViewOrderList(propertyTranslator.TranslateToDomainProperty, command);

    // Get the results of the view
    var itemsService = DependencyResolver.Current.GetService<ICustomerOrganizationService>();
    IFutureValue<long> total;
    var results = itemsService.FindByView(view, ((command.Page - 1) * view.PageSize), command.PageSize, activeUser, out total);

    // Map the domain results to view models
    var mapper = new ViewResultsMapper();
    var viewModels = results.Select(o => mapper.MapToViewModel(o, view.VisibleProperties));

    // Return the grid model
    return command.GroupDescriptors.Any()
               ? new GridModel
               {
                   Data = TelerikGridHelpers.ApplyDynamicGrouping(viewModels.AsQueryable(), command.GroupDescriptors),
                   Total = Convert.ToInt32(total.Value)
               }
               : new GridModel
               {
                   Data = viewModels,
                   Total = Convert.ToInt32(total.Value)
               };
}

You can probably ignore the meat of what those methods do, because a lot of it is domain-related stuff. When I went to refactor this code to consolidate these methods, I came up with the following:

public GridModel GetViewResults<TView, TEntity, TRequiredProps, TService, TTranslator>(int viewId, GridCommand command)
    where TEntity: class, new()
    where TView : ViewBase
    where TRequiredProps : ReadOnlyCollection<string>, new()
    where TService : IFindByView<TView, TEntity> 
    where TTranslator : IPropertyNameTranslator, new()
{
    var activeUser = UserContext.Current.GetActiveUser();

    var viewSvc = DependencyResolver.Current.GetService<IViewService<TView>>();
    var view = viewSvc.FindBy(viewId, activeUser);

    // Replace view's ordering with the command options
    var propertyTranslator = new TTranslator();
    view.Orders = TelerikGridHelpers.GenerateViewOrderList(propertyTranslator.TranslateToDomainProperty, command);

    // Get the results of the view
    var itemsService = DependencyResolver.Current.GetService<TService>();
    IFutureValue<long> total;
    var results = itemsService.FindByView(view, ((command.Page - 1) * view.PageSize), command.PageSize, activeUser, out total);

    // Map the domain results to view models
    var mapper = new ViewResultsMapper();
    var viewModels = results.Select(o => mapper.MapToViewModel(o, view.VisibleProperties.ToList(), new TRequiredProps()));

    // Return the grid model
    return command.GroupDescriptors.Any()
               ? new GridModel
               {
                   Data = TelerikGridHelpers.ApplyDynamicGrouping(viewModels.AsQueryable(), command.GroupDescriptors),
                   Total = Convert.ToInt32(total.Value)
               }
               : new GridModel
               {
                   Data = viewModels,
                   Total = Convert.ToInt32(total.Value)
               };
}

The advantage with this generics Frankenstein is that I can simplify the individual calls in the initial examples like so:

public GridModel GetOpportunityViewResults(int viewId, GridCommand command)
{
    return GetViewResults<OpportunityView, Opportunity, OpportunityViewResultsRequiredProperties, IOpportunityService, OpportunitiesViewModelTranslator>(viewId, command);
}

public GridModel GetCustomerViewResults(int viewId, GridCommand command)
{
    return GetViewResults<CustomerView, CustomerOrganization, CustomerViewResultsRequiredProperties, ICustomerOrganizationService, CustomersViewModelTranslator>(viewId, command);
}

The disadvantage is that it's really smelly that way and technically Microsoft warns against too many type parameters on generic methods. It does the job, but holy crap is it ugly with all those generic parameters and constraints. Am I taking generics too far? Should I break up the bits of the method into a series of generic calls that only take one or two types?

\$\endgroup\$
1
\$\begingroup\$

Yes. I would break them up into serial calls. It should help things stay a bit more modular as well and make refactoring/alterations easier down the road. Not to mention it would be easier to read.

\$\endgroup\$
  • \$\begingroup\$ The only thing about breaking them up is that it won't really do too much to cut down on repetition. I'd still have probably four sections where I'd make calls to another class. \$\endgroup\$ – Josh Anderson Dec 29 '11 at 1:56
1
\$\begingroup\$

For a specific closed generic IFindByView<TView, TEntity>, are there multiple implementations? If not you could just retrieve IFindByView<TView, TEntity> from the DI instead of TService : IFindByView<TView, TEntity>. This would help to get rid one type parameter (TService).

You can remove a second type parameter TRequiredProps by adding an argument ReadOnlyCollection<string> requiredProperties to the method. This also means the collection doesn't need to be new()-able (= one less constraint which the rest of your code has to adhere to).


Now if you were to use a property DI container and do inversion of control instead of using the service locator (anti-?!) pattern you could also just ctor-inject the dependencies and instead of specifying which type to use by a generic type constraint, you could just as well configure the DI container to inject the right type given some conditions. How this is done depends on the DI container.

Here's an example of what Ninject can do.

\$\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.