Effects of returning a self reference from an instance method in C#

Question

Let's say I have an interface called IConvertableModel and it helps me to convert some MVC models to/from DTO objects as shown below:

public class DisplayEditModel : IConvertableModel<Display>
{
    [HiddenInput(DisplayValue = false)]
    public int ObjectId { get; set; }

    [StringLength(255)]
    public string Description { get; set; }

    public Display ToDto()
    {
        return new Display
        {   
            Description = Description,
            ObjectId = ObjectId,
        };
    }

    public void SetFromDto(Display dto)
    {
        Description = dto.Description;
        ObjectId = dto.ObjectId;
    }
}

But there is one problem with this approach and that is it doesn't allow me do this :

var dto = _dtoRepository.GetFirstDto();
return new DisplayEditModel().SetFromDto(dto);

Instead I should do the following:

var dto = _dtoRepository.GetFirstDto();
var model = new DisplayEditModel();
model.SetFromDto(dto);
return model;

and this is adding extra two lines of code and little bit complexity in the long run.

What I am thinking is to convert SetFromDto method to something like this:

public DisplayEditModel SetFromDto(Display dto)
{
   Description = dto.Description;
   ObjectId = dto.ObjectId;
   return this;
}

I think the benefit of this code is obvious but I also like to learn whether this harms code readability and leads to unexpected results for developers in the long run and if you think anything else, what would you recommend.

Note: Because of the interfaces reasons, I am not thinking to implement a constructor method.

Answer 1

A few thoughts, to begin with:

1. Adding lines of code is not the same as adding complexity. Having three statements, where each does a simple operation, is not necessarily harder to maintain or understand than a single statement with three operations inside of it.

2. When a method that begins with Set..., programmers will automatically assume that some stateful values of the target object are going to be changed by this method. It is rare for Set methods to have a return value. Property setters in C# actually "return" the original value passed into them, so you can chain setters:

   int i = foo.A = 2;
   

   So the precedent is generally against returning "this" from a set method specifically.

3. Chaining in general is most useful/desired when you're expecting several operations to be performed, one after the other. For example, C# provides nice initialization syntax so you can "chain" a bunch of different property setters on the same object:

   var foo = new Foo { A = 1, B = 2 };
   

   You can see how chaining is fulfilling the need to perform similar, grouped, repetitive operations that typically get performed all together. That is not the problem that you are trying to solve.

If your main gripe is that you don't like having three lines of code, why not just use a helper whose name indicates what you're trying to do?

TModel MapToModel<TModel, TDto>(TDto dto, TModel model)
    where TModel : IConvertableModel<TDto>
{
    model.SetFromDto(dto);
    return model;
}

// usage:

var dto = _dtoRepository.GetFirstDto();
return MapToModel(dto, new DisplayEditModel());

... or even:

TModel CreateModel<TModel, TDto>(TDto dto)
    where TModel : IConvertableModel<TDto>, new()
{
    var model = new TModel();
    return MapToModel(dto, model);
}

// usage:

var dto = _dtoRepository.GetFirstDto();
return CreateModel<DisplayEditModel>(dto);

This is simple, readable, and feasible, whereas the approach you're suggesting would break the IConvertableModel<Display> interface:

public interface IConvertableModel<TDto>
{
    public TDto ToDto();
    public ??? SetFromDto(TDto dto);
}

What would SetFromDto return? You would have to define another generic type on IConvertableModel.

public interface IConvertableModel<TDto, TModel> {
    public TDto ToDto();
    public TModel SetFromDto(TDto dto);
}

But this wouldn't really indicate that the SetFromDto method is necessarily returning itself, because it allows for a class that is not a TModel to implement IConvertableModel to convert between two other types.

Now, you could go out of your way to push the generics even farther:

public interface IConvertableModel<TDto, TModel>
    where TModel : IConvertableModel<TDto, TModel>
{...}

But this still allows for some fudging, and the interface cannot guarantee that you are really returning "this" object. All in all, I'm not a big fan of that approach.

Answer 2

Rather than having DisplayEditModel have a get/set method for a Display object to get/set the values, just use a property that doesn't actually have a separate backing store:

public Display Display
{
    get
    {
        return new Display
        {
            Description = Description,
            ObjectId = ObjectId,
        };
    }
    set
    {
        Description = value.Description;
        ObjectId = value.ObjectId;
    }
}

Now you can use an object initializer with this property when creating a model:

return new DisplayEditModel() { Display = dto };

Answer 3

This is a very javascript way of approaching this problem, though it has it's benefits. In the context of C#, it is a little bit strange though libraries such as LINQ do this to allow chaining together function calls.

My only worry about this, is that this has to be a class that does this consistently. Implementing a chaining function return pattern is not really a convenience as much as it is a design choice. The rule to follow in this case, would be to return this every time you mutate the object.

Chaining also may not be worth it performance wise. Something that can be done by wrapping all those operations into a single function is much faster. For instance:

   MyVector.setX(1).SetY(1).SetZ(1).SetW(0)

is a lot slower than simply

   MyVector.set(1, 1, 1, 0)

because now you are now doing excessive stack operations to do something fairly simple. It only becomes worth it on very large operations that take up the bulk of the computing time and make sense to chain together. For this reason, LINQ allows you to chain things together.

I wouldn't say that it necessary "harms" or is dangerous. We are in the world of a managed language, so we don't have direct access to that memory location (unlike C/C++). So I would just call it a design choice which can be fairly powerful in some cases and not so much in others.

Answer 4

As noted, chainable methods work fine but are not as common in C# as in some other languages. If the extra lines of code only happen in one place, I'd just leave it alone. If it's really bugging you or you do it a lot, then consider implementing a special constructor for it:

public void DisplayEditModel(Display dto)
{
    this.SetFrom(dto);
}

or a static factory method:

public static DisplayEditModel CreateFrom(Display dto)
{
    var model = new DisplayEditModel();
    model.SetFrom(dto);
    return model;
}

Either option has a clear intent, lets you create and return the object in a single line, and is idiomatic. It does require a few extra lines of code in DisplayEditModel, but I doubt it will be a serious problem.