Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Handle injected dependencies by with an additional method for auto-properties; additional contextual keyword for constructor parameters. #14928

Closed
jyarbro opened this issue Nov 3, 2016 · 10 comments

Comments

@jyarbro
Copy link

jyarbro commented Nov 3, 2016

Preface

I suggested this as a response to issue #14853 but was told multiple times this proposal is different enough that it was distracting from that discussion's proposal. In that issue discussion there were also some discussions about the definition of dependency injection as an implementation of the dependency inversion principle, the use of IoC containers to accomplish this same functionality I'm about to propose, and the overarching SOLID acronym. I am assuming for the purpose of this discussion thread here my understanding and definition of these principles to be accurate, and welcome discussion about this solution which would correct any inaccuracies in my understanding.

I am also not well versed in IL or how the CLR would handle certain aspects I've laid out below. This is written from the perspective of an experienced C# coder who appreciates how the .NET framework has alleviated much of the boilerplate minefield code that comes with doing common tasks in unmanaged code.

Example Class

So take a situation where you are injecting a dependency into the a class through the constructor. Here's a contrived example:

public class MailingLabelService<T> where T : ILabelType
{
    IPersonRepository PersonRepository { get; }
    IPrinterService PrinterService { get; }

    bool PrinterReady { get; set; } = false;
    bool Initialized { get; set; } = false;

    public MailingLabelService(IPersonRepository personRepo, IPrinterService printService) {
        PersonRepository = personRepo;
        PrinterService = printerService;
    }

    public void Initialize() {
        // obviously contrived. more robust and complex examples would excel here as this 
        // type of status would normally be checked at print time.

        PrinterReady = PrinterService.StatusCode > 0;

        Initialized = true;
    }

    public Print(int personId) {
        if (!Initialized)
            throw new InvalidOperationException("Initialize() must be executed first.");

        if (!PrinterReady) {
            // handle with friendly response to user, maybe with data-bound status updating
        }

        var address = GetFormattedAddress(personId);

        // ... further implementation
    }

    string GetFormattedAddress(int personId) {
        // ... implementation
    }

    // ... further implementation
}

In this example, the two properties are dependencies that MailingLabelService has which are injected by the client or unit test. To be clear, these properties are private and readonly.

The purpose of this constructor in this class is simply to accept the dependencies and set the properties to them. In a more realistic example the constructor would have potentially 5 or 6 properties like this depending on how strictly the developer is adhering to the single responsibility principle, and several other lines of perhaps very rudimentary initialization code.

As the object is not yet fully initialized, it is probably not yet the right time to look into the injected properties to see if they have been initialized, if the printer is ready, etc. The class would therefore include some initialization method which would handle certain use cases, such as if the printer was current in error status. The printer service is potentially a sealed class that has some obscure language that would not be very clear later in the code if we stuck with the default implementation.

Proposed Change

What I am proposing is to add an injection method to auto-properties of reference types only. The compiler would not allow this for value types. This inject method, with no body, would simply reduce boilerplate constructor code. What it is doing behind the scenes would be to set the properties based on parameters passed in on instantiation labelled with the inj contextual keyword (similar to out and ref)

The exact words / letters used in these two terms inject and inj are less important to me. If this does not fit within dependency injection as a concept for others, I am fine to accept some other relevant pattern name instead.

The example class would become less bogged down with initialization code and moderately more maintainable as you now only really see post-instantiation behavior.

The getter is implied for the underlying field, and the compiler would not allow a custom getter to be defined when an injector is defined.

public class MailingLabelService<T> where T : ILabelType
{
    IPersonRepository PersonRepository { inject; }
    IPrinterService PrinterService { inject; }

    bool PrinterReady { get; set; } = false;
    bool Initialized { get; set; } = false;

    public void Initialize() {
        // obviously contrived. more robust and complex examples would excel here as this 
        // type of status would normally be checked at print time.

        PrinterReady = PrinterService.StatusCode > 0;

        Initialized = true;
    }

    public Print(int personId) {
        if (!Initialized)
            throw new InvalidOperationException("Initialize() must be executed first.");

        if (!PrinterReady) {
            // handle with friendly response to user, maybe with data-bound status updating
        }

        var address = GetFormattedAddress(personId);

        // ... further implementation
    }

    string GetFormattedAddress(int personId) {
        // ... implementation
    }

    // ... further implementation
}

The implied constructor would conceptually look like this, though would not be implemented in the written code:

public MailingLabelService(inj IPersonRepository personRepo, inj IPrinterService printService) { 
    this.PersonRepository = PersonRepository;
    this.PrinterService = PrinterService;
}

In situations where constructor logic is appropriate, the written constructor could have custom parameter titles, and the values could be set as per usual. The class could inherit from a base class, and the parameters could be passed in using the inj keyword as well for required parameters.

public MailingLabelService(int someInt, string someString, inj IPersonRepository personRepo, inj IPrinterService printService) : base (inj printService) { 
    PersonRepository = personRepo;
    PrinterService = printService;

    // ... logic to handle someInt and someString
}

Potential ways of instantiation follow. These are not mutually exclusive options.

  1. Constructor parameters in the implied order the properties are defined.
var labelService = new MailingLabelService<USLabel>(inj personRepo, inj printService);
  1. Named constructor parameters
var labelService = new MailingLabelService<USLabel>(PersonRepository: inj personRepo, PrinterRepository: inj printService);

Object initialization syntax would not work for these properties as they are still readonly.

In situations where auto-properties are not appropriate, further action may be taken on a dependency, for example checking if the printer device is reporting errors at instantiation, a method body could be defined for inject which would be executed after the object is initialized. To be clear, the property reference would be set in the constructor still, but the injected code body would not be executed until after the constructor.

The purpose of this code block is to have a fully constructed object before this logic is executed. This reduces the potential for partially constructed object exception handling, and reduces the need for boilerplate initialization methods on classes that need to check the state of dependencies on instantiation. It also keeps simple property-specific logic with the properties, which is generally helpful in keeping code maintainable.

The final product would look like this:

public class MailingLabelService<T> where T : ILabelType
{
    IPersonRepository PersonRepository { inject; }

    IPrinterService PrinterService {
        inject {
            // obviously contrived. more robust and complex examples would excel here as this 
            // type of status would normally be checked at print time.

            PrinterReady = value.StatusCode > 0;
        }
    }

    bool PrinterReady { get; set; } = false;

    public Print(int personId) {
        if (!PrinterReady) {
            // handle with friendly response to user, maybe with data-bound status updating
        }

        var address = GetFormattedAddress(personId);

        // ... further implementation
    }

    string GetFormattedAddress(int personId) {
        // ... implementation
    }

    // ... further implementation
}
@dsaf
Copy link

dsaf commented Nov 3, 2016

I doubt this will happen before some version of primary constructor syntax arrives. Also it might be possible with poor man's hacky code generation that is currently in works #5292.

@HaloFour
Copy link

HaloFour commented Nov 3, 2016

What makes this an improvement (or frankly any different) from using standard property setters and object initializer syntax?

public class MailingLabelService<T> where T : ILabelType
{
    IPersonRepository PersonRepository { set; }

    IPrinterService PrinterService {
        set {
            // obviously contrived. more robust and complex examples would excel here as this 
            // type of status would normally be checked at print time.

            PrinterReady = value.StatusCode > 0;
        }
    }

    bool PrinterReady { get; set; } = false;

    public Print(int personId) {
        if (!PrinterReady) {
            // handle with friendly response to user, maybe with data-bound status updating
        }

        var address = GetFormattedAddress(personId);

        // ... further implementation
    }

    string GetFormattedAddress(int personId) {
        // ... implementation
    }

    // ... further implementation
}
var labelService = new MailingLabelService<USLabel> {
    PersonRepository: personRepo,
    PrinterRepository: printService
};

@sirgru
Copy link

sirgru commented Nov 3, 2016

@HaloFour With set these properties are not read-only. I think the idea is to preserve read-only status while allowing the property-style setting.

@HaloFour
Copy link

HaloFour commented Nov 3, 2016

@sirgru

So my original complaint remains, this does nothing to alleviate the complexity of the constructor, nor does it fix issues that will arise with inheritance, specifically that the constructor of the base class must execute before the constructor of the derived class, still leaving you in a partially initialized state. All this does is take the constructor body and scatter it all over the class definition. It also makes it difficult to see what the constructor parameters actually are. Reordering properties would break the contract. How are explicitly-defined constructors handled?

I still also object to intertwining the language with dependency injection, particularly through these keywords inject, inj or anything like them. They assume the consumer which the language cannot do. C# provides no container, that remains a library concern. And those libraries often support many options to designing your classes in order to avoid the boilerplate mentioned above.

@jyarbro
Copy link
Author

jyarbro commented Nov 3, 2016

Constructor complexity reduction is not the goal. Perhaps you are bringing things we discussed from the previous discussion over, when I've already considered things you said and removed or modified things in order to factor in your issues.

Explicit constructors are already laid out in my suggestion above. Perhaps you skimmed that part with the reasonable assumption that it was the same as the previous thread.

The only part of the constructor that this proposal touches is the auto assignment of properties. The rest of the inject method body, as I said in my proposal, is meant to execute post-construction.

I could use some explanation on your statement as well:

They assume the consumer which the language cannot do.

@dsaf
Copy link

dsaf commented Nov 3, 2016

I feel like this proposal could be split further. And some stuff/cases might be addressed with #229.

@HaloFour
Copy link

HaloFour commented Nov 3, 2016

@jyarbro

Explicit constructors are already laid out in my suggestion above. Perhaps you skimmed that part with the reasonable assumption that it was the same as the previous thread.

So you'd still be required to explicitly declare the constructor parameters and set the "properties". So aside from adding a new unnecessary keyword there'd be no syntax or behavior benefit over existing syntax today when you need a custom constructor.

Can you explain why the inj keyword is necessary? What does it do other than mark that an argument is supposed to be an injected dependency? Why would it be required when instantiating the class?

For scenarios where an explicit constructor is not necessary I agree with @dsaf that source generators should be sufficient in eliminating the boilerplate. You could design your class like this:

public partial class MailingLabelService<T> where T : ILabelType {
    IPersonRepository PersonRepository { get; }
    IPrinterService PrinterService { get; }

    // methods here
}

and the source generator could produce this:

public partial class MailingLabelService<T> where T : ILabelType {
    public MailingLabelService(IPersonRepository PersonRepository, IPrinterService PrinterService) {
        this.PersonRepository = PersonRepository;
        this.PrinterService = PrinterService;
    }

    // methods here
}

I could use some explanation on your statement as well:

They assume the consumer which the language cannot do.

That these language constructs are for designing classes with the assumption that they will be used in a very specific manner, likely by some IoC library. But the C# language can't assume that's how the class would be used, and nothing at all would actually help to enforce that consumption.

@jyarbro
Copy link
Author

jyarbro commented Nov 4, 2016

Ah, okay I'm going to start with your last point. I am not implying any specific consumer, nor would I expect the language to infer a specific type of consumer from this. In fact this is specifically to write "vanilla" code, as it were, without trying to jump into Nuget and add an IoC container for what is effectively a simple project. I prefer to tackle issues on my own first to fully understand the problem and the depth of the implementation, rather than using a bunch of sledgehammers to drive in some nails I found. If I get to the point where a project has grown beyond a simple (or even moderate) implementation, then I will start hunting around for frameworks that will handle my issues in a more robust manner. I have yet to find a situation where an IoC container has not made my project _more_complex rather than less. They tend to obfuscate the readability of what is going to happen at runtime, where I prefer code to speak for itself without massive amounts of documentation.

As a use case, let's say I'm writing some simple unit tests for a basic app. I could go install Moq and Ninject and try to create a robust testing project, however it is probably a significant increase in man-hours on a project that would normally only take me a week.

I am typically writing a lot of line of business solutions with an iterative methodology. This results in many lean solutions that allow customers to immediately see results and determine if they even want to go further down the "tool" route, or if they want to instead work on business process changes first.

I end up with a lot of repetitive boilerplate code because of this because I follow some relatively standard design patterns in my code based on some industry accepted principles.

I get the feeling your job requires a different flavor of code from you, which results in products that may not benefit much from this concept.

Moving onto your question about the inj keyword. It is intended as a contract between the caller and the constructor that this parameter is intended for injection. As @dsaf's proposal defines this, the immutable properties of the object. What I am intending is the parameter itself should be immutable even within the constructor, where a typical readonly property is still mutable in the constructor.

@HaloFour
Copy link

HaloFour commented Nov 4, 2016

@jyarbro

Removing an IoC container from the equation the only difference is effectively instantiation, which this proposal doesn't appear to address. You're still effectively left with a constructor full of parameters or writable properties.

Moving onto your question about the inj keyword. It is intended as a contract between the caller and the constructor that this parameter is intended for injection.

That doesn't answer what the keyword does or how it affects the behavior of the parameter or the argument being passed to that parameter. The only behavior you mention is "immutability", but it makes little sense to tie that functionality to a keyword that doesn't express "immutability". That's covered by proposal #115 which uses the readonly keyword to mark parameters as readonly in the same way that fields can be readonly.

@jyarbro
Copy link
Author

jyarbro commented Jul 22, 2020

I believe this is now more or less resolved by C# 9 and the init-only properties.
https://devblogs.microsoft.com/dotnet/welcome-to-c-9-0/

@jyarbro jyarbro closed this as completed Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants