1

I make extensive use of DI, but I wonder, where is the 'granurality' limit, when some set of functionality should be separated to a class - lets take an example:

public class DownloadManager : IDownloadManager
{
    public Uri ResourceIdentifier { get; }
    public DownloadManager(Uri uniqueResourceIdentifier)
    {
        if (!uniqueResourceIdentifier.IsWellFormedOriginalString()) throw new ArgumentException(nameof(uniqueResourceIdentifier));
        ResourceIdentifier = uniqueResourceIdentifier;
    }
    public DownloadManager(string uniqueResourceIdentifier) : this(new Uri(uniqueResourceIdentifier)) { }

    public async Task<byte[]> GetBytesAsync(CancellationToken token = new CancellationToken())
    {
        using (var client = new HttpClient())
        using (var response = await client.GetAsync(ResourceIdentifier, token).ConfigureAwait(false))
        {
            var data = await response.Content.ReadAsByteArrayAsync().ConfigureAwait(false);
            return data;
        }
    }
}

It looks for me that this class brings almost no additional benefit, and even hinders some operations:

  • what if we need a stream (ReadAsStreamAsync())?
  • it's not a great idea to litter the code with async, as it spreads like GPL (also see Task etiqutte).
  • may lead (but in this case it rather promotes composition) to yo-yo problem

and could be replaced with a static method like this (or even the extension to Uri class):

public static async Task<byte[]> GetBytesAsync(/*this */Uri uniqueResourceIdentifier, CancellationToken token = new CancellationToken())
{
    if (!uniqueResourceIdentifier.IsWellFormedOriginalString()) throw new ArgumentException(nameof(uniqueResourceIdentifier));
    using (var client = new HttpClient())
    using (var response = await client.GetAsync(uniqueResourceIdentifier, token).ConfigureAwait(false))
    {
        var data = await response.Content.ReadAsByteArrayAsync().ConfigureAwait(false);
        return data;
    }
}

But one cannot simply DI a static method, so this will be wrapped in thin class implementing an interface.

Option 3: The third option would be, the code could be written just where it's needed (the API is great, almost no redundant configuration), risking, that if needed, the code will need to be refactored (or copied) in the future. Also, it could be tempting than to make a god-class like FileManager, which will download, unzip CRUD & all, but sacrificing freedom and 'S' from SOLID.

  1. Which one is preferred in this specific case (almost the same goes for file unzipping, using ZipArchive and couple more of great MS APIs)?
  2. Based on what, should I make a decision which one of this three approaches to take?
  • Just seeing "Manager" in the title makes me immediately suspicious. – whatsisname Nov 11 '15 at 19:14
  • @whatsisname: Why & how would you improve the name? – Piotr Falkowski Nov 11 '15 at 19:51
  • Re: can't DI a static method. Func<Uri, CancellationToken, Task<byte[]>>. I concede that it's awkward in C#. – Kasey Speakman Nov 11 '15 at 20:10
  • Using simply a static method instead of a class does neither solve your problem with "async", nor is it a solution to your question "what if we need a stream". There is no deep inheritance hierarchy, thus no yo-yo problem when you have simply one interface plus one implementation. And "injecting a (static or non-static) method is perfectly possible, that is I guess what Kasey Speakman wants to tell you with his comment. So that makes at least four wrong assumptions in one question. – Doc Brown Nov 11 '15 at 22:07
  • @PiotrFalkowski: How does the class actively "manage" the download? What does "management" even mean? – whatsisname Nov 12 '15 at 18:34
2

I'm not sure I follow all the code details, example and options you put in the text of your question. But I'll address the title of the question specifically, that is "The granularity level to repeat code":

There's not a fixed rule about granularity; design is not about fixed rules (otherwise we'd just write a computer program to do designs and a lot of developers would be... working for Über?). Design has heuristics, which are like rules of thumb that seem to have worked well in the past for certain types of problems.

The General Responsibility Assignment Software Patterns (GRASP) are pretty useful heuristics for design (no so different from SOLID). In your case, it sounds a lot like the problem solved by the Pure Fabrication principle (emphasis mine):

Who is responsible when you are desperate, and do not want to violate high cohesion and low coupling?

Assign a highly cohesive set of responsibilities to an artificial or convenience "behavior" class that does not represent a problem domain concept—something made up, in order to support high cohesion, low coupling, and reuse.

Your DownloadManager class is definitely reusable and it looks pretty cohesive. I'm not sure about the DI necessity, unless it's really something that varies in implementations (but that's another question).

I highlighted the word desperate above, because the granularity notion you seek is there. As you assign responsibilities to classes in your design, at some point you realize that giving the downloading (or unzipping) responsibility to other classes violates the strong cohesion principle, and so you're not satisfied by that (Larman's choice of the word "desperate" is probably hyperbole, as a way to get it to stick in your brain).

The solution is to essentially refactor that responsibility into a separate class that is very cohesive. Pure Fabrication is a lot like the Extract Class refactoring, except pure fabrications don't seem like problem-domain classes. The example for Extract Class has a new class called TelephoneNumber, which sounds like a problem-domain class. But your DownloadManager class sounds like a solution-domain class (especially the Manager part).

The Visitor classes in GoF pattern by the same name are also great examples of Pure Fabrications. They are highly cohesive, but came about only after developers got frustrated with adding non-cohesive responsibilities to classes in a data structure. It helps to think of the design before they existed.

  • The class is only "definitely reusable" because it doesn't do anything of any significance. – whatsisname Nov 12 '15 at 17:07
  • @whatsisname Can you say (without using sarcasm) why it's not significant? If the code appeared a three different places (say in three other classes) wouldn't it be useful to make a pure fabrication? – Fuhrmanator Nov 12 '15 at 18:11
  • Because it's basically 20 lines of boilerplate that wrap what could reasonably be accomplished in 3 with no loss of clarity. The class doesn't even "manage" anything by virtue of controlling data through it, all it does is blindly pass-through. – whatsisname Nov 12 '15 at 18:33
  • 1
    @whatsisname I agree with your conclusion that it's only three lines. But I could see it as good reuse if they're repeated (DRY) and/or dilute the cohesion of the classes where they're placed. C# is not my strength, but If you wanted to start counting bytes transferred, attempt re-try on connection failures, or throttle download requests, wouldn't this be a good encapsulation? – Fuhrmanator Nov 12 '15 at 20:51
  • @Fuhrmanator: If we "wanted to start counting bytes transferred, attempt re-try on connection failures (...)" this boilerplate would, in my opinion, be a burden - because, having an interface not prepared for it, we could not resonably expect that functionality, and should either break it, or expect functionality outside the contract. That would be an argument for static function (and its overloads when needed). – Piotr Falkowski Nov 12 '15 at 21:21
3

May I remind you to assess conformance with the SOLID principles.

Also, bear in mind the idea that "Good enough really does mean good enough". If extra abstractions aren't necessary, then don't do it. It's unnecessary complication.

But downloading, unzipping etc should be in separate classes if they may be used independently, or if alternative methods of downloading and/or unzipping might be desirable.

One major pro for separate classes is testability; a huge benefit of the single responsibility principle.

It's all a matter of balance. If you're finding the code is becoming unmanagable, then do something about it by being stricter on good OOP heuristics such as SOLID.

Lastly, remember to code to an interface. Design the interface first. Write a snippet of code that would use the module in question, as if it already exists, in the most natural way. Then write that code according to that interface. Then finally decide on the implementation, and keep that separate from the interface.

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.