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

Shorthand for setting private readonly fields in constructor #12820

Closed
Richiban opened this issue Jul 29, 2016 · 7 comments
Closed

Shorthand for setting private readonly fields in constructor #12820

Richiban opened this issue Jul 29, 2016 · 7 comments

Comments

@Richiban
Copy link

Note: This is similar in functionality to the primary constructors that were considered for C# 6 but were dropped "because we're going to have record types!".

I'm reanimating this similar proposal because I don't understand what this has to do with records?

With C# I'm finding myself writing this same field-setting boilerplate code over and over again; it's incredibly common and I would say that there must be an enourmous number of developers doing the same day after day...

public class MyClass
{
    private readonly IServiceA _serviceA;
    private readonly IServiceB _serviceB;

    public MyClass(IServiceA serviceA, IServiceB serviceB)
    {
        _serviceA = serviceA;
        _serviceB = serviceB;
    }

    public ActionResult Index()
    {
        var something = _serviceA.GetSomething();
        // ???
        // Profit.
    }
}

All I want is to define a type that has a dependency on an IServiceA and an IServiceB, but I've had to write variants of "serviceA" and "serviceB" six times each!

Not only is this a pain to write (although IDEs are now really good at inserting the code for you) but it's error-prone and not helpful to look at once it's been written.

My proposal is that the above would be replaced with:

public class MyClass
{
    public MyClass(
        private IServiceA serviceA,
        private IServiceB serviceB)
    {
    }

    public ActionResult Index()
    {
        var something = serviceA.GetSomething();
        // ???
        // Profit.
    }
}

Note that I've not included the need to add the 'readonly' keyword in this proposal. Readonly should be the default behaviour.

The advantage of this approach is:

  • it's very easy to read
  • there's no more duplication or boilerplate code
  • it's just as easy to add body code to this type of constructor as any other
  • it's less error-prone

I'm pretty sure that this would still be considered a "primary" constructor, meaning that any & all other
constructors defined must call this constructor.

It's less error prone because it prevents the possibility of a private readonly field being left unset or being read before they're set.

Who else has ever written this code before?

public class MyClass
{
    private readonly IServiceA _serviceA;
    private readonly IServiceB _serviceB;

    public MyClass(IServiceA serviceA, IServiceB serviceB)
    {
        if(_serviceA == null)
            throw new ArgumentNullException(nameof(serviceA));

        _serviceA = serviceA;
        _serviceB = serviceB;        
    }
}

Oops! You've just got a guaranteed exception because you checked the value of the field for null, not the constructor argument! Since the field has not yet been assigned it will always be null! Sadly, although this is a typo, it still compiles just fine and can be hard to spot.

With my primary constructors:

public class MyClass
{
    public MyClass(
        private IServiceA serviceA,
        private IServiceB serviceB)
    {
        if(serviceA == null)
            throw new ArgumentNullException(nameof(serviceA));
    }
}

The error simply can't happen, because the constructor argument is hidden from you.

I should explain why this has nothing to do with records:

  • My class (in this case) is almost certainly a service of some kind (not a "value" type) and thus shouldn't have members for equality, ToString() etc
  • My dependencies should not be publicly accessible.
@HaloFour
Copy link

I'd argue that embedding the fields into other constructors obfuscates their existence. Changing their default behavior to readonly would be inconsistent and there is no C# keyword to reverse that behavior. Having multiple constructors would introduce other grammar issues. Would you redeclare these fields or would they just automatically exist from the other constructor?\

At least with records/"primary constructors" there can only be one and it is front and center. As for whether records should be an all-or-none proposition, I believe there was a separate proposal here discussing breaking it up into component features.

@alrz
Copy link
Member

alrz commented Jul 30, 2016

Related: #11909, #10154 (comment)

@Richiban
Copy link
Author

@HaloFour perhaps you're right that readonly-by-default is a break from C#'s history, so that could be dropped.

I do disagree though that embedding the fields into constructors obfuscates their existence though. I was bending over backwards a little in supporting the idea of multiple constructors; maybe the existence of a primary constructor proscribes that there is only one?

My other idea was to allow one to completely omit the constructor via the introduction of a new keyword (tentatively called "dependency") but, after thinking about it, that idea has too many limitations:

public class MyClass
{
    private dependency IServiceA _serviceA;
    private dependency IServiceB _serviceB;
}

It suffers from the same limitations as the original proposal for primary constructors, i.e. you can't write a constructor body and as soon as you want to add a custom constructor you have to rewrite a large chunk of the class.

Just for reference, F# has primary constructors and records:

type MyClass(serviceA : IServiceA, serviceB : IServiceB) =
    member this.MyMethod () =
        serviceA.GetSomething()

@alrz
Copy link
Member

alrz commented Jul 30, 2016

@Richiban Good point. F# primary constructors do not introduce properties like what has been proposed for C# records. Instead, it conflates both in a single feature that beside of being at the class-level scope, allows you to specify different names to preserve code convention and/or define matching members to substitute fields, etc. I think there should be at least two separate features to satisfy specific requirements instead of introducing a single hairy feature which does not accommodate none of them quite well.

@Richiban
Copy link
Author

At first I thought that the chosen syntax for records:

public class Point(int x, int y);

would have precluded the original primary constructor idea, but maybe this is still possible:

public class Service(IServiceB serviceB)
{
    public object GetSomething()
    {
        return serviceB.GetSomething();
    }
}

I don't think it would conflict with records too much; the rule would be "if you end your class signature with a semicolon then your constructor, properties, and equality members get generated for you. If you declare a class body then you still get the auto-generated constructor & fields but nothing else". Makes sense.

@alrz
Copy link
Member

alrz commented Jul 30, 2016

@Richiban AFAIK it is possible -- you can use primary constructor's parameters in class-level initializers and method bodies. But in the current proposal the presence of the class body does not affect code generation. It does not seem to be a good idea though, i.e. lack of presence of the class body is orthogonal to the type being a record. F#, for example, allows you to define members for a record and it is quite useful sometime.

@gafter
Copy link
Member

gafter commented Sep 11, 2017

We are now taking language feature discussion in other repositories:

Features that are under active design or development, or which are "championed" by someone on the language design team, have already been moved either as issues or as checked-in design documents. For example, the proposal in this repo "Proposal: Partial interface implementation a.k.a. Traits" (issue 16139 and a few other issues that request the same thing) are now tracked by the language team at issue 52 in https://github.com/dotnet/csharplang/issues, and there is a draft spec at https://github.com/dotnet/csharplang/blob/master/proposals/default-interface-methods.md and further discussion at issue 288 in https://github.com/dotnet/csharplang/issues. Prototyping of the compiler portion of language features is still tracked here; see, for example, https://github.com/dotnet/roslyn/tree/features/DefaultInterfaceImplementation and issue 17952.

In order to facilitate that transition, we have started closing language design discussions from the roslyn repo with a note briefly explaining why. When we are aware of an existing discussion for the feature already in the new repo, we are adding a link to that. But we're not adding new issues to the new repos for existing discussions in this repo that the language design team does not currently envision taking on. Our intent is to eventually close the language design issues in the Roslyn repo and encourage discussion in one of the new repos instead.

Our intent is not to shut down discussion on language design - you can still continue discussion on the closed issues if you want - but rather we would like to encourage people to move discussion to where we are more likely to be paying attention (the new repo), or to abandon discussions that are no longer of interest to you.

If you happen to notice that one of the closed issues has a relevant issue in the new repo, and we have not added a link to the new issue, we would appreciate you providing a link from the old to the new discussion. That way people who are still interested in the discussion can start paying attention to the new issue.

Also, we'd welcome any ideas you might have on how we could better manage the transition. Comments and discussion about closing and/or moving issues should be directed to #18002. Comments and discussion about this issue can take place here or on an issue in the relevant repo.


I have not moved this feature request to the csharplang repo because it is a discussion of some alternatives to features under consideration, and the discussion seems to have died down. However, you are welcome to move discussion to the new repo if this still interests you.

@gafter gafter closed this as completed Sep 11, 2017
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