C# – Refactoring Similar Classes Using Inheritance

cinheritanceobject-orientedrefactoring

I'm currently working on refactoring the code-base for one of our services. I'm been going through reviewing everything, and I feel it's a bit scattered, and could probably adhere to OOP principles better.

I have three classes that are all derivative of another class Cache. All three of these classes perform the exact same operations, the only difference is the type of object they are querying for and what method call they use to query them. What would be the best method of making these classes even more simple?

public static class ZenDeskCache
{
    public static ZendeskApi Api = new ZendeskApi(GlobalVariables.ZendeskUrl, GlobalVariables.ZendeskUser,
        GlobalVariables.ZendeskPass);

    public class Users : Cache<Users, List<User>>
    {
        protected override List<User> GetData()
        {
            var users = Api.Users.GetAllUsers();
            var allUsers = new List<User>(users.Users);

            while (users.NextPage != null)
            {
                users = Api.Users.GetByPageUrl<GroupUserResponse>(users.NextPage);
                allUsers.AddRange(new List<User>(users.Users));
            }

            allUsers = allUsers.OrderBy(n => n.Name).ToList();

            return allUsers;
        }

        protected override TimeSpan GetLifetime()
        {
            return TimeSpan.FromDays(1);
        }
    }

    public class Organizations : Cache<Organizations, List<Organization>>
    {
        protected override List<Organization> GetData()
        {
            var organizations = Api.Organizations.GetOrganizations();
            var allOrgs = new List<Organization>(organizations.Organizations);

            while (organizations.NextPage != null)
            {
                organizations = Api.Users.GetByPageUrl<GroupOrganizationResponse>(organizations.NextPage);
                allOrgs.AddRange(new List<Organization>(organizations.Organizations));
            }

            allOrgs = allOrgs.OrderBy(n => n.Name).ToList();

            return allOrgs;
        }

        protected override TimeSpan GetLifetime()
        {
            return TimeSpan.FromDays(1);
        }
    }

    public class Groups : Cache<Groups, List<Group>>
    {
        protected override List<Group> GetData()
        {
            var groups = Api.Groups.GetGroups();
            var allGroups = new List<Group>(groups.Groups);

            while (groups.NextPage != null)
            {
                groups = Api.Groups.GetByPageUrl<MultipleGroupResponse>(groups.NextPage);
                allGroups.AddRange(new List<Group>(groups.Groups));
            }

            allGroups = allGroups.OrderBy(n => n.Name).ToList();

            return allGroups;
        }

        protected override TimeSpan GetLifetime()
        {
            return TimeSpan.FromSeconds(600);
        }
    }
}

Best Answer

The problem is that the design of the underlying API loves to repeat type information in all property names:

... Api.Groups.GetGroups();
... groups.Groups ...

    ... Api.Groups.GetByPageUrl<MultipleGroupResponse>(...);
    ... groups.Groups ...

This misdesign makes it difficult to abstract over all otherwise identical Api.X objects. In particular, your methods differ in these points:

  • the Api.X object that methods are called on
  • the name of the Api.X.GetX() method to get the inital response
  • the name of the property x.X that accesses the data in one response
  • the type of the response from GetPageByUrl

The solution is to introduce an appropriate layer that levels away these differences. E.g. you could define a common function CommonGetData that could be invoked like this:[1]

[1]: Please note that I'm not fluent in C#, so only take note of the conceptual design rather than the specifics of syntax.

protected override List<User> GetData()
{
    return CommonGetData(
        Api.Users,
        (api) => api.GetAllUsers(),
        (response) => response.Users,
    );
}

With CommonGetData being defined approximately like this:

private static <T, ResponseT, ApiT> List<T> CommonGetData(
        ApiT api,
        Function<ResponseT, ApiT> getInitialResponse,
        Function<Collection<T>, ResponseT> itemsFromResponse,
    )
{
    ResponseT response = getInitialResponse(api);
    var allItems = new List<T>(itemsFromResponse(response);

    while (response.NextPage != null)
    {
        response = api.GetPageByUrl<ResponseT>(response.NextPage);
        allItems.AddRange(new List<T>(itemsFromResponse(response));
    }

    allItems = allItems.OrderBy(n => n.Name).ToList();

    return allItems;
}

If using lambdas is not your thing, you could also note that this looks remarkably like a candidate for either the Strategy Pattern (in CommonGetData, the callback parameters represent strategies), or for the template method pattern. We could then define an abstract Api like this:

abstract class CommonApi<T, ResponseT>
{
    protected abstract ResponseT GetInitialResponse();
    protected abstract Collection<T> ItemsFromResponse(ResponseT response);
    protected abstract GetPageByUrl(Url url);

    private List<T> GetData()
    {
        ResponseT response = GetInitialResponse();
        var allItems = new List<T>(ItemsFromResponse(response);

        while (response.NextPage != null)
        {
            response = GetPageByUrl(response.NextPage);
            allItems.AddRange(new List<T>(ItemsFromResponse(response));
        }

        allItems = allItems.OrderBy(n => n.Name).ToList();

        return allItems;
    }
}

class UsersApi : CommonApi<User, MultipleUserResponse>
{
    protected override MultipleUserResponse GetInitialResponse()
    {
        return Api.Users.GetAllUsers();
    }

    protected override Collection<User> ItemsFromResponse(MultipleUserResponse response)
    {
        return response.Users;
    }

    protected override GetPageByUrl(Url url)
    {
        Api.Users.GetPageByUrl<MultipleUserResponse>(url);
    }
}

which would be used like

private UsersApi usersApi = new UsersApi();

protected override List<User> GetData()
{
    return usersApi.GetData();
}

Ideally, the existing APIs could be refactored to implement common interfaces, so that wrapping them in common interfaces that delegate to the incompatible methods would not be necessary.

Also note that your very high use of static classes probably gets in the way of code reuse, since it makes it difficult to pass these around as values. If only one instance may exist, prefer the Singleton Pattern which also makes your classes easier to test when compared with static classes.

Related Topic