-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Discussion: implicit return of this
for builder pattern with this
type
#311
Comments
Why is code like Or rather, why is that a pattern that should be promoted? |
@svick Method chaining can help to implement immutable design (see Roslyn), beside this, you can create with method chaning workflows how an implementation should be used. new Person()
.SetName("Petr")
.Build(); The workflow here is for example: Set first Name then you can call Build. The advantage is, you do not need to check in Build if name was set or not, because you can be sure that it was. Also for the consumer of the code library it is much more intuitive what options are there. I'm referring here only to method chaining not the special case of implicit return of |
@svick But not only builders would profit from this addition. LINQ like constructs, e.g. the extension methods to It would serve (at least) 3 goals:
|
I don't like this. There are cases such as cloning where you would want |
@jnm2 Then use the class name as return type, as usual. The |
@biqas This proposal is specifically about returning |
That's no good: abstract class Foo
{
public abstract Foo Clone();
}
class Bar : Foo
{
// Having a this-type restriction would be incredibly helpful.
// I should be forced to return a Bar here, but I'm not.
public override Foo Clone() => new Bar();
} Better: abstract class Foo
{
public abstract this Clone();
}
class Bar : Foo
{
public override Bar Clone() => new Bar();
} I'm not coming up with anything new here, I've seen this ask on dotnet/roslyn. |
// satisfy override signature
public override Foo Clone() => CloneThis();
// and redirect to self constraining signature method
private Bar CloneThis() => new Bar(this); I think the signature of the base class should stay intact. |
@jnm2 There are probably CLR restrictions that forbid that use, because you cannot tell the type system to use |
You might be able to use a constructor with many optional parameters (which is what I tried to imply by using named parameters).
Yes, properties that only have a setter are a code smell, but they could be a solution here. I think that using an existing feature in an unusual way is preferable to adding a new feature to the language. Also, I don't see much reason why builder properties shouldn't be readable. For example,
They wouldn't, LINQ methods don't return
If you really care about this kind of safety (I'm not sure why, "this method has to return So this proposal wouldn't improve identity safety checking by that much.
Why is this something that is important to document in the method signature? I think the main reason why knowing if a chainable method returns For example, But I'm not sure whether Also, you might be able to take advantage of the To sum up, I think your points 2. and 3. are fairly weak. Point 1. is good, but it only applies to types that follow a fairly rare pattern (and I don't see a reason to encourage it). Which to me does not make this feature worth it. |
whilst I can see a point this proposal, it's definitely niche and so unlikely to happen. However, if the ideas around the enum PersonSex { male, female, undetermined }
class Person
{
public string Name { get; private set; }
public DateTime DateOfBirth { get; private set; }
public int PersonSex Sex { get; private set; }
public class PersonBuilder
{
Person _person = new Person();
public this SetName(string Name) => ({_person.Name = Name; this });
public this SetDateOfBirth(DateTime DateOfBirth) =>
({_person.DateOfBirth= DateOfBirth; this });
public this SetAge(int Age) =>
({OtherModule.CalculateVirtualDateOfBirth(Age, ref _person.DateOfBirth); this });
// this function returns the offset in days of the virtually created date
public this SetSex(PersonSex Sex) => ({_person.Sex= Sex; this });
public Person Build() => _person;
}
} |
@jnm2 While #252 would collide with this proposal, #169 would not. As they seem to serve the same purpose, I would vote for #169 as it seems more "mature". So, that seems to be no problem. This proposal would also allow to easily convert some classes, like the Logo turtle, to use method chaining. Imagine... class Turtle
{
public float Speed { get; set; }
public void TurnLeft(float Degrees) { }
public void TurnRight(float Degrees) { }
public void PenUp() { }
public void PenDown() { }
public void Move(float Seconds) { }
public void GoHome() { }
}
//...//
public static int main() {
var turtle = new Turtle();
turtle.Speed = 20;
turtle.Move(3);
turtle.TurnLeft(90);
turtle.PenDown();
turtle.Move(5);
turtle.PenUp();
turtle.GoHome();
} This could so easily converted to the common method chaining, without any further ado in the existing method bodies. class Turtle
{
public float Speed { get; set; }
public this SetSpeed(float Speed) => this.Speed = Speed;
public this TurnLeft(float Degrees) { }
public this TurnRight(float Degrees) { }
public this PenUp() { }
public this PenDown() { }
public this Move(float Seconds) { }
public this GoHome() { }
}
//...//
public static int main() {
new Turtle().SetSpeed(20)
.Move(3).TurnLeft(90)
.PenDown().Move(5).PenUp()
.GoHome();
} |
@DavidArno So you avoid |
Not really. It is just a case of re-using an existing proposal to achieve this idea too. |
The one thing I don't care for about this proposal is that it pushes mutable instances and builder patterns rather than immutable wither/decorator patterns. It also appears to promote setter methods over writable properties which, in my opinion, don't fit in well in the .NET ecosystem. |
Slightly left-field idea that I'm not sure if I like or not: What about some bit of syntax that allowed the chaining of any instance method? I'm picking the class SomeObject
{
void Foo() { ... }
int Bar() { ... }
}
var someObject = new SomeObject();
someObject.Foo()^.Bar()^.Foo(); // Allow this to end with ^; so it all returns someObject? What this is doing is ignoring the return value of the method and effectively returning the object on which the method was called. This would compile out to something like: someObject.Foo();
someObject.Bar();
someObject.Foo(); We already have precedent for this style of syntax with I haven't yet really thought through the implications of this, it might be a nice shortcut syntax or it might encourage really unreadable code. EDIT: I don't personally really feel the feature is necessary for the language though, and agree with @HaloFour and others that this is fairly niche and encourages some not so nice patterns. I'm also the person who posted #169 and a fan of #252 and would like to see |
I really like #252 on a number of levels, especially the part where you can have |
@TheOtherSamP
How does it push that? Whether Set methods are inferior to setters and counter the initial language design is just a matter of taste, IMO. There shouldn't be a right or wrong. For the use in builders e.g. they do a good job. But if you want to push immutables together with setters, instead of WithXxx methods, maybe another syntax should be invented. Just as a quick, out of the guts example: imObject.WithA("a").WithB("b").Execute();
// following is equivalent
imObject.{ A="a", B="b" }.Execute();
class ImObject {
public string A {
set new { return new ImObject(this) {A=value} ;}
get; private set;
}
/*... */
|
@lachbaer If you particularly want to chain these things with a single expression, how about a method (or extension method) like this? public static T Chain<T>(this T obj, Action<T> action)
{
action(obj);
return obj;
} Then you could create chaining methods like this: public Turtle MoveLeft() => this.Chain(t => t.X--);
public Turtle MoveRight() => this.Chain(t => t.X++); For now though, having to have expression bodies and |
Alternatively I was having an idea quite sometimes that. Maybe we could make scope to everything not just constructor Instead of chaining method we could call method directly Something like this public static int main() {
var turtle = new Turtle();
turtle {
// assume all here is called by turtle
Speed = 20;
Move(3);
TurnLeft(90);
PenDown();
Move(5);
PenUp();
GoHome();
}
} By the way, I don't like builder pattern |
@Thaina Dim turtle As New Turtle();
With turtle
.Speed = 20;
.Move(3);
.TurnLeft(90);
.PenDown();
.Move(5);
.PenUp();
.GoHome();
End With I used to use the But you could also go with var turtle = new Turtle();
// ... //
{ref var t = ref turtle;
t.SetSpeed(10);
t.TurnRight(90);
t.GoHome();
} That is now allowed with C# 7, should have no impact on the emmited MSIL and encapsulate the (About the braces: I'm personally not a friend of too strict brace/indent style rules. As long as they serve the purpose and don't lead to accidential mistakes I mix the styles within the same codebase. Well, there is one rule: the used style for the corresponding code construct must be consistent throughout the whole code.) |
this
for builder pattern with this
typethis
for builder pattern with this
type
This pattern does not seem to be so favored as that it justifies the use of 'this' for this purpose. Ultimately it only is syntactic sugar to save 'return' statements in cases the method could be written in one line. |
When using builder patterns it seems to be common to chain method invokes.
With two additions this could be heavily abbrevated.
(void)
casting for methods (also see Allow return void function with void function #135)Following rules apply:
this
as return type impies that the function will return itself.static
functions cannot return itself, so thethis
type is an error.return;
orreturn this;
return
of anything else (e.g.new PersonBuilder();
) is prohibitedSetAge
above a(void)
cast is necessary, to force something like a simplereturn;
, otherwise the functions value would be returned, what is an error if it is notthis
.I think this nicely fits in the language and promotes creating chaining patterns.
The text was updated successfully, but these errors were encountered: