3
\$\begingroup\$

I would appreciate some feedback on my design of a few classes to query the Trakt.tv API.

public abstract class TraktQuery
{
    private const string baseUrl = "http://api.trakt.tv/";
    private const string apiKey = "[API key here]";

    protected abstract string QueryUrl { get; }
    protected abstract List<string> QueryParameters { get; }
    protected abstract TraktResponse ParseResponse(string response);

    public TraktResponse PerformQuery()
    {
        //exception handling removed for brevity
        WebClient webClient = new WebClient();
        string url = baseUrl + QueryUrl + "/" + apiKey + "/" + string.Join("/", QueryParameters);
        string response = webClient.DownloadString(url);
        return ParseResponse(response);
    }
}

public class TraktQueryEpisodeSummary : TraktQuery
{
    private string _showName;
    private int _season;
    private int _episodeNumber;

    public TraktQueryEpisodeSummary(string showName, int season, int episodeNumber)
    {
        _showName = showName;
        _season = season;
        _episodeNumber = episodeNumber;
    }

    protected override string QueryUrl
    {
        get { return "show/episode/summary.json"; }
    }

    protected override List<string> QueryParameters
    {
        get
        {
            List<string> paramters = new List<string>();
            paramters.Add(_showName);
            paramters.Add("" + _season);
            paramters.Add("" + _episodeNumber);
            return paramters;
        }
    }

    protected override TraktResponse ParseResponse(string response)
    {
        return JsonConvert.DeserializeObject<TraktResponseEpisodeSummary>(response);
    }

public class TraktResponseEpisodeSummary : TraktResponse
{
    public TraktShowInformation show { get; set; }
    public TraktEpisode episode { get; set; }
}

Currently TraktResponse is an empty abstract class but I'll probably add to it soon.

This code is then used as follows:

class Program
{
    static void Main(string[] args)
    {

        TraktQueryEpisodeSummary query = new TraktQueryEpisodeSummary("House", 1, 1);
        TraktResponseEpisodeSummary summary = (TraktResponseEpisodeSummary)query.PerformQuery();

        Console.WriteLine("Title: " + summary.show.title);
        Console.WriteLine("Poster image: " + summary.show.images.poster);
        Console.WriteLine("Genres: " + string.Join(", ", summary.show.genres));
        Console.WriteLine("Episode name: " + summary.episode.title);
        Console.WriteLine("Overview: " + summary.episode.overview);
    }
}

What makes me think my design isn't quite right is that I have to cast from TraktResponse to the more specific TraktResponseEpisodeSummary in order to access the information I need. What I would like to do is make TraktQueryEpisodeSummary.ParseResponse() return a TraktResponseEpisodeSummary instead of a TraktResponse however I'm aware that C# doesn't support return type covariance which I think is what this is.

Are my concerns well-founded or not? Is there a better design that avoids this casting or is it perfectly normal and acceptable?

\$\endgroup\$
1
  • \$\begingroup\$ Could you use generics to turn PerformQuery into PerformQuery<T>() where T: TraktResponse \$\endgroup\$ – dreza Nov 3 '12 at 20:32
3
\$\begingroup\$

Dreza has outlined the correct approach (the same used in IComparable<T>). You can make your class generic, with a type constraint:

public abstract class TraktQuery<TResponse> where TResponse : TraktResponse

And change every occurence of TraktResponse to TResponse:

protected abstract TResponse ParseResponse(string response);

and

public TResponse PerformQuery()
{
    //...
}

Then, your deriving class can be defined as

public class TraktQueryEpisodeSummary : TraktQuery<TraktResponseEpisodeSummary>
{
    //...
    protected override TraktResponseEpisodeSummary ParseResponse(string response)
    {
        //...
    }
    //...
}

Voila, no cast necessary:

TraktResponseEpisodeSummary summary = query.PerformQuery();

Further suggestions:

  • Use collection and object initializers, for instance in the overridden QueryParameters:

     return new List<string> {_showName, _season.ToString(), _episodeNumber.ToString()};
    
  • Or turn them into an IEnumerable<string> and use an iterator block:

    protected override IEnumerable<string> QueryParameters
    {
        get
        {
            yield return _showName;
            yield return _season.ToString();
            yield return _episodeNumber.ToString();
        }
    }
    
  • Be careful when concatenating strings to form a URL like this:

    string url = baseUrl + QueryUrl + "/" + apiKey + "/" + string.Join("/",QueryParameters);
    

    It might be better to

    1. Use the Uri and/or UriBuilder class
    2. Put the first three parts into an sequence, then call IEnumerable<T>.Concat and finally string.Join
    3. There's probably some even better options.
\$\endgroup\$
1
  • \$\begingroup\$ That's a great way to solve the problem. Extra bonus points for the other points of advice as well. Thanks very much. \$\endgroup\$ – Stuart Leyland-Cole Nov 3 '12 at 22:35

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.