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

Generated constructor doesn't contain guard clauses #7

Closed
NightOwl888 opened this issue Mar 28, 2018 · 4 comments
Closed

Generated constructor doesn't contain guard clauses #7

NightOwl888 opened this issue Mar 28, 2018 · 4 comments
Assignees

Comments

@NightOwl888
Copy link

In the book Dependency Injection in .NET it is pointed out how to use guard clauses in conjunction with readonly fields to make it impossible to set dependency fields to null. I have talked about the advantage of this in this StackOverflow question and there is more about it here.

// VS 2017 syntax
public class MyClass 
{
    private readonly IFoo _foo;
    private readonly IBar _bar;
    private readonly IBaz _baz;

    public MyClass(IFoo foo, IBar bar, IBaz baz) 
    {
        _foo = foo ?? throw new ArgumentNullException(nameof(foo));
        _bar = bar ?? throw new ArgumentNullException(nameof(bar));
        _baz = baz ?? throw new ArgumentNullException(nameof(baz));
    }
}

// VS 2015 syntax
public class MyClass 
{
    private readonly IFoo _foo;
    private readonly IBar _bar;
    private readonly IBaz _baz;

    public MyClass(IFoo foo, IBar bar, IBaz baz) 
    {
        if (foo == null)
            throw new ArgumentNullException(nameof(foo));
        if (bar == null)
            throw new ArgumentNullException(nameof(bar));
        if (baz == null)
            throw new ArgumentNullException(nameof(baz));
        _foo = foo;
        _bar = bar;
        _baz = baz;
    }
}

While not everyone agrees that the guard clause is necessary and AFAIK there is no way to configure any DI container to inject a null dependency, the DI pattern can technically be used with or without a DI container. So constructors should have a guard clause to prevent the latter case (e.g. unit tests) from being able to instantiate the service with a null dependency.

Perhaps the best option would be to add a second Generate constructor option that includes guard clauses, so those who prefer them can add them if they want and those that don't want them can exclude them. That said, I think the default should be to include them since they come highly recommended by the experts.

tjrpw

@conwid
Copy link
Owner

conwid commented Mar 29, 2018

I have read the book myself and I totally agree.

@conwid conwid self-assigned this May 9, 2018
@conwid
Copy link
Owner

conwid commented May 9, 2018

I started on this feature, but as far as I can tell, currently there's no way in a Roslyn CodeRefactoringProvider to tell the selected language version of the project. I tried going to SO for answer, but without any luck (at least for now):

https://stackoverflow.com/questions/50176715/roslyn-net-compiler-platform-get-current-project-c-sharp-language-version

So I'm thinking that I will implement the "old version" (no nameof and no throw expression) to support every language version properly, and if someone can figure out a way to properly determine the selected language version, the fix can be adjusted as well. Until then, it's up to everybody to use the built-in VS refactorings to change the snippet inserted by this refactoring.

@NightOwl888
Copy link
Author

Thanks. Using the older syntax is still preferable to having to add them all manually.

Could a workaround be to make a different installer for VS2015 and VS2017? I realize that you can still select the language version regardless of which version of VS you are using, but separate installers might be good enough for most people.

@conwid
Copy link
Owner

conwid commented May 21, 2018

I committed a fix and published the new version to the marketplace.
Also made a small blog post about it: http://dotnetfalcon.com/dependency-injection-toolset-upgrade/

I do know that the issue of using the new language features still exists, but I'm closing this issue for now. I have to say, I don't really like the workaround of having different installers (I'm not even sure that the markeplace supports it, but even if it does, it feels way more hacky than I'd like). I am creating a new issue for this though, so if someone has an idea, they can join the conversation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants