7

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);
        }
    }
}
  • Do you have a typo here? organizations = Api.Users.GetByPageUrl(... – Mike Nakis Nov 20 '15 at 12:05
  • If you're answering your question in that edit, please put it in a separate answer instead. Your question is no longer a question; it's a blog post. – Robert Harvey Nov 20 '15 at 14:27
5

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.

  • The purpose of the static classes is primarily persistence. This cache is global and is to be shared against all web requests that come into our service. – JD Davis Nov 19 '15 at 19:38
  • I updated the OP with some of the changes you suggested. I think it's a bit more coherent now. – JD Davis Nov 19 '15 at 21:40
2

What you need is a non-member (static utility or base class) generic helper function for concatenating the NextPage results from different IEnumerable objects.

Once that is done, approximately five lines of code (in each class) can be condensed down to one line. This will clear up the clutter and IMHO no further refactoring is needed.

It would be best if NextPage is an interface method on the response object. Likewise, it would be best if the response object implement IEnumerable<T> for the respective return type T.

If that is not the case, you may consider:

  • Fix the upstream library or code
  • Use type dynamic
    • But this is a very slippery path.
  • Unfortunately the API I'm using is a third party library for interfacing with Zendesk, so it is a bit more limited. I guess I'm just unclear on how to accomplish this without seeing an example as I've never really attempted something like this. – JD Davis Nov 19 '15 at 19:06
1

If you really want to refactor these classes using inheritance, then fine, go ahead, amon's answer is right on the money.

However, be advised that eliminating a little code duplication is one of the least legitimate reasons for complicating a design by using inheritance. If the resulting code is more verbose than the original, then you have a net loss in readability, understandability, and maintainability.

This might actually be worse than a bit of code duplication.

  • 1
    It's actually because I wanted to expand my caching code quite a bit and needed to come up with a good jumping off point as several other things will be added to the cache soon. – JD Davis Nov 20 '15 at 15:29
0

Could you create a generic class with a type variable for the varying data class?

public class WhateverCache<T> : Cache<T, List<T>> {
  protected override List<T> GetData() {
    // whatever logic you have
  }

The specific classes would only override the time span-defining method.

  • The main thing I'm trying to abstract out is the logic in the GetData classes. They all perform the same series of steps, the only difference is what API call they make and the data type. – JD Davis Nov 19 '15 at 17:47
  • I don't know enough C# to speak precisely; either C# allows to call new List<T> and then the problem is solved, or you can make an abstract method specifically for that and override it in every class returning the specific type of list. With C# having reified generics, new List<T> or a variation on the theme should be possible without losing type safety. – 9000 Nov 19 '15 at 18:14
0

After reviewing some of the information posted by @Amon, I think I've got something a little neater. It's more verbose, but it seems to be cleaner.

CommonApi

public abstract class CommonApi<T, TResponse>
    where TResponse : GroupResponseBase
{
    protected abstract TResponse GetInitialResponse();
    protected abstract List<T> ItemsFromResponse(TResponse response);
    protected abstract TResponse GetPageByUrl(string url);

    public ZendeskApi Api = new ZendeskApi(
        GlobalVariables.ZendeskUrl,
        GlobalVariables.ZendeskUser,
        GlobalVariables.ZendeskPass);

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

        while (response.NextPage != null)
        {
            response = GetPageByUrl(response.NextPage);
            allItems.AddRange(ItemsFromResponse(response));
        }

        allItems = SortData(allItems);

        return allItems;
    }

    private List<T> SortData(List<T> list)
    {
        return list;
    }

    public List<User> SortData(List<User> list)
    {
        return list.OrderBy(n=>n.Name).ToList();
    }

    public List<Group> SortData(List<Group> list)
    {
        return list.OrderBy(n => n.Name).ToList();
    }

    public List<Organization> SortData(List<Organization> list)
    {
        return list.OrderBy(n => n.Name).ToList();
    }
}

UserListApi

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

    protected override List<User> ItemsFromResponse(GroupUserResponse response)
    {
        return response.Users.ToList();
    }

    protected override GroupUserResponse GetPageByUrl(string url)
    {
        return Api.Users.GetByPageUrl<GroupUserResponse>(url);
    }
}

OrganizationListApi

public class OrganizationListApi : CommonApi<Organization, GroupOrganizationResponse>
{
    protected override GroupOrganizationResponse GetInitialResponse()
    {
        return Api.Organizations.GetOrganizations();
    }

    protected override List<Organization> ItemsFromResponse(GroupOrganizationResponse response)
    {
        return response.Organizations.ToList();
    }

    protected override GroupOrganizationResponse GetPageByUrl(string url)
    {
        return Api.Organizations.GetByPageUrl<GroupOrganizationResponse>(url);
    }
}

GroupListApi

public class GroupListApi : CommonApi<Group, MultipleGroupResponse>
{
    protected override MultipleGroupResponse GetInitialResponse()
    {
        return Api.Groups.GetGroups();
    }

    protected override List<Group> ItemsFromResponse(MultipleGroupResponse response)
    {
        return response.Groups.ToList();
    }

    protected override MultipleGroupResponse GetPageByUrl(string url)
    {
        return Api.Groups.GetByPageUrl<MultipleGroupResponse>(url);
    }
}

ZendeskCache

public class ZendeskCache
{
    public class Users : Cache<Users, List<User>>
    {
        protected override List<User> GetData()
        {
            var users = new UserListApi();

            return users.GetData();
        }

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

    public class Organizations : Cache<Organizations, List<Organization>>
    {
        protected override List<Organization> GetData()
        {
            var orgs = new OrganizationListApi();

            return orgs.GetData();
        }

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

    public class Groups : Cache<Groups, List<Group>>
    {
        protected override List<Group> GetData()
        {
            var groups = new GroupListApi();
            return groups.GetData();
        }

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

I think it may be possible to reduce the ZendeskCache even a bit further, but I'll keep tinkering. I'm trying to figure out methods to make my future coding practices cleaner and more maintainable.

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.