1

Having a service that eg. downloads data, whos responsability is ProgressReporter? Should it be caller responsability to pass a valid one, as in CodeSample1, or it would be better if it's object's creator responsability like in CodeSample2? First option gives more flexibility regarding possible reuse of this function object with different implementation of IProgressReporter, while second one abstracts away the need to repeat and pass around the IProgressReporter object.

CodeSample1:

public class WebClientDownloader : IDownloader
{
    private IProgressReporter ProgressReporter { get; set; }

    private void ProgressChanged(object sender, DownloadProgressChangedEventArgs e)
    {
        // ...
    }

    private void DownloadCompleted(object sender, DownloadDataCompletedEventArgs e)
    {
        ProgressReporter?.Stop();
    }

    public Task<byte[]> GetBytesAsync(Uri uniqueResourceIdentifier, 
IProgressReporter progress)
    {
        ProgressReporter = progress;
        using (var client = new WebClient())
        {
            client.DownloadProgressChanged += ProgressChanged;
            client.DownloadDataCompleted += DownloadCompleted;
            return client.DownloadDataTaskAsync(uniqueResourceIdentifier);
        }
    }

CodeSample2:

public class WebClientDownloader : IDownloader
{
    public IProgressReporter ProgressReporter { get; }
    public WebClientDownloader(IProgressReporter progress)
    {
        ProgressReporter = progress;
    }

    private void ProgressChanged(object sender, DownloadProgressChangedEventArgs e)
    {
        //...
    }

    private void DownloadCompleted(object sender, DownloadDataCompletedEventArgs e)
    {
        ProgressReporter?.Stop();
    }

    public Task<byte[]> GetBytesAsync(Uri uniqueResourceIdentifier)
    {
        ProgressReporter.Restart();
        using (var client = new WebClient())
        {
            client.DownloadProgressChanged += ProgressChanged;
            client.DownloadDataCompleted += DownloadCompleted;
            return client.DownloadDataTaskAsync(uniqueResourceIdentifier);
        }
    }
}
1
  • I think you should consider, how are you going to test your code. It will help you to decide which way to go. (method that is accepting an object is usually harder to test compared to a method that accepts parameters)
    – Urban48
    Commented Nov 14, 2016 at 11:17

2 Answers 2

2

I would favor the first, with some changes.

Since ProgressReporter is responsible for the progress of a single GetBytesAsync call, it's right to make it an argument to the function. But since it's responsible for the progress of a single GetBytesAsync call, it is improper to assign it to a class level variable. You'll quickly run into concurrency issues if you use this downloader twice since the second call will stomp on that variable, killing the progress of the first download.

Instead, you should just leave it in local scope and use lambdas for the event callbacks.

2
  • The original sample had locks all over, which I removed for brevity. However, your reasoning about responsbilitis is flawless and convinced me. Thanks Commented Dec 15, 2016 at 13:40
  • Alternatively you can have an explicit inner class that holds ProgressReporter and has the event methods, which is what a pair of lambda's closing over it is.
    – Caleth
    Commented Dec 15, 2016 at 14:47
1

I think it largely depends on whether ProgressReporter represents the state of the WebClientDownloader or whether it represents the state of a given download in progress. I'm not clear on your code sample if you intend for a single WebClientDownloader to be instantiated per download or not, as well. If there's a one to one correlation between downloads and instances of your object, then the progress is measured as a change in the instance state, and so it makes sense for the ProgressReporter to be passed in through instantiation and immutable after that. You'd then just destroy this instance and create a new one next download.

I think it also matters what you believe will be the conditions under with a ProgressReporter will change. It's clear from your code that you don't think it'll change in the middle of a download, but you should think about when it will change in order to get a better sense of when to allow said change.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.