1

I'm writing a PCL that uses an HttpClient to go visit a few sites and extract data from them. My initial code looked like this:

public static class Download
{
    public async static Task<byte[]> FromYouTubeAsync(string videoUri)
    {
        using (var client = new HttpClient())
        {
            string source = await client
                .GetStringAsync(videoUri);

            // get links...

            return await client
                .GetByteArrayAsync(links.First());
        }
    }
}

I realized, however, that this would be a waste of resources if the user of my library already had a HttpClient in hand. For example:

// Caller code
using (var client = new HttpClient())
{
    // Do some work with client

    byte[] bytes = await Download.FromYouTubeAsync("some://uri"); // A second HttpClient is created here, wasting resources and time...

    // Do some work with client and bytes
}

One solution could be to add an overload taking an HttpClient as a parameter, but what if the user wanted to use a WebClient instead? Or an HttpWebRequest? Or some other kind of client that wasn't availsble in the PCL?

My solution

I decided to take delegate parameters to get the page source and to download the file based on the URI. My code now looks like this:

public static class Download
{
    public async static Task<byte[]> FromYouTubeAsync(string videoUri)
    {
        using (var client = new HttpClient())
        {
            return await FromYouTubeAsync(
                () => client
                .GetStringAsync(videoUri),
                uri => client
                .GetByteArrayAsync(uri));
        }
    }

    public async static Task<byte[]> FromYouTubeAsync(
        Func<Task<string>> sourceFactory, Func<string, Task<byte[]>> downloadFactory)
    {
        string source = await sourceFactory();

        // get links...

        return await downloadFactory(links.First());
    }
}

The problem

Hm. Wait, what if the caller is using a client that doesn't support async, like HttpWebRequest? Better add another overload taking a Func<string> and a Func<string, byte[]>. What if s/he wants to make the first call synchronous, but the second call async? Or vice versa? OK, just add 2 more overloads. Wait, what if the caller wants the whole operation to be synchronous? Let's go add 4 Download.FromYouTube overloads wrapping each of the async methods. Whoops, it turns out we need to visit another page of YouTube in case the video's signature is encrypted! Sure, let's go add 8 mor-


I think you can see where this is going. How do I maintain this flexibility without scaring the user away when Visual Studio says that there are 17 different overloads for this method?

1

Why not make FromYouTubeAsync be a pure function (e.g. which takes a String representing the HTML of the youtube video page, and which returns the URL of the mp4 video file it finds within), and then have the caller worry about how exactly to download bytes over the internet, if you think they want to have as much control over the process as you're implying?

You could always provide one default implementation using HttpClient for those clients who don't really care how it gets downloaded and just want as low-effort an API as possible.

  • I think it's a little far to ask the user to download the source of the page, but returning a URI seems reasonable and will help me get rid of the Func<string, byte[]>. – James Ko Jul 3 '15 at 14:56
  • 1
    Given that we're assuming the user knows how to, given the URL to an mp4 file, get its bytes, I figured the user would also know how to, given the URL to an HTML page, get its bytes. I guess I'm handwaving those extra steps of "given the bytes that describe an HTML page, return a String representation of the HTML page". – Nebu Pookins Jul 3 '15 at 18:19
2

Your first argument regarding reuse of the HttpClient makes perfect sense.
As for the rest of of the requests, I think you're trying to optimize prematurely. That can lead to unnecessary complexity.
Also, I'd like to warn you of using the static classes. This is usually a bad idea. It makes your code harder to test and extend.
Anyways, below is a design that allows the flexibility you want.

Declare an interface.

public interface IDownloadable
{
    Task<byte[]> FromUriAsync(string videoUri);
}

Implement it for HttpClient and allow passing the instance of HttpClient as a constructor parameter.

public class HttClientpDownloader : IDisposable, IDownloadable
{
    private readonly HttpClient _client;

    public HttClientpDownloader() : this(new HttpClient())
    {

    }
    public HttClientpDownloader(HttpClient client)
    {
        _client = client;
    }

    public void Dispose()
    {
        if (_client == null)
            return;

        _client.Dispose();
    }

    public Task<byte[]> FromUriAsync(string videoUri)
    {
        throw new NotImplementedException();
    }
}

As for the rest of your requests, Factory Method design pattern should be able to resolve them. Just implement WebClientDownloader, HttpWebRequestDownloader and whatever else you desire in dedicated classes.
Same approach can also work for your partially async request. Just implement that logic in a dedicated class. Something like PartiallyAsyncHttpClientDownloader.

public class DownloaderFactory
{
    public IDownloadable Create(/*pass whatever parameters to make teh decision*/){
        switch (param)
        {
            case '1':
                return new HttClientpDownloader();
            case '2':
                return new WebClientDownloader();
            ...
        }
   }
}
  • I think interfaces would be helpful for hiding the growing number of Funcs I seem to be accepting as a parameter, but 1) I don't wish to implement a factory class and 2) a downloader for each new type of client (what if the user wanted to use a custom client)? – James Ko Jul 3 '15 at 14:29
  • 1
    I don't think static classes are nescessarily harder to test that non-static classes. Consider a static class Math that contains functions like sin, cos, tan, etc. This sort of class is the absolute easiest to test, since they are all pure functions with no state. In contrast, non-static classes tend to have at least some state (or else why would they need reference to the this pointer?) – Nebu Pookins Jul 4 '15 at 19:34
  • 1
    @NebuPookins the problem is not testing the static classes. It's testing classes that use static classes internally. Because you have no way of mocking or modifying the static class behavior – DanielS Jul 4 '15 at 20:33
  • @DanielS I decided to go with inheritance after the overloads grew worse; now I only have to override one method in a derived class to expand to a new website. Thanks for the suggestions! – James Ko Jul 4 '15 at 23:17
  • @DanielS thanks for the clarification. I'm more familiar with Scala than C# and I had incorrectly assumed that a C# static class could implement an interface in the same way a Scala object can implement an interface. If such a thing were possible, then you could provide the (sole) instance static class via dependency injection, but without it, you're right that it's difficult to mock the static class behavior. – Nebu Pookins Jul 5 '15 at 0:16
1

After thinking about it overnight and looking at Nebu's answer, here are the solutions I came up with:

  • Return only the URI, and accept one a/sync Func. While I do think it's reasonable for an API to return the URI to a video (in case the user doesn't want to download it just yet), I think it's a bit technical to ask him/her to download the source of the page and would tempt me to make more overloads (i.e. SourceToBytes, SourceToUri).
  • Make the HTML source downloader accept a string as a URI. In other words, instead of () => client.GetStringAsync("some://uri"), it would look like uri => client.GetStringAsync(uri). The reason I say this is because doing so would prevent me from having to provide 8 more overloads asking how to download the string from that other page on YouTube (in case the video's signature is encrypted), when s/he has already said it's client.GetStringAsync. TL;DR: This would prevent me from having to create 8 more overloads.
  • Move all of the Func methods into an AdvancedDownload class. This will help not scare the user away when he clicks on the overloads for the method.

EDIT: After some more complications I also decided to wipe all of the static methods and inherit all except one of them from a base class. Despite having ~15 different boilerplate methods in the base, now I only have to override one method each time I want to expand to a new website (i.e. Vimeo).

Thanks @DanielS and @NebuPookins for the suggestions!

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.