Using type object as returning type - bad practice ?

Question

I have a method

    private object SetGrid(IGrid grid)
    {
        grid.PagerHelper.SetPage(1, 10);
        grid.SortHelper.SetSort(SortOperator.Ascending);
        grid.PagerHelper.RecordsPerPage = 10;

        return grid;
    }

which returns an object of type object.

Then I cast the object back to the previous type.

    var projectModel = new ProjectModel();

    projektyModel = (ProjectModel)SetGrid(projectModel);

The gain of this is, the method SetGrid can be reused across the app.

Is this a common practice or should I avoid doing this ?

Answer 1

You could use a generic method instead, and constrain the type argument to your IGrid interface:

private T SetGrid<T>(T grid) where T : IGrid
{
    grid.PagerHelper.SetPage(1, 10);
    grid.SortHelper.SetSort(SortOperator.Ascending);
    grid.PagerHelper.RecordsPerPage = 10;

    return grid;
}

You should still be able to call the method in exactly the same way, just without the cast. Type inferencing should be capable of automagically figuring out the required generic type argument for you:

var projectModel = new ProjectModel();
projektyModel = SetGrid(projectModel);

EDIT...

As other answers have mentioned, if your IGrid objects are reference types then you don't actually need to return anything at all from your method. If you pass a reference type then your method will update the original object, not a copy of it:

var projectModel = new ProjectModel();  // assume that ProjectModel is a ref type
projektyModel = SetGrid(projectModel);
bool sameObject = object.ReferenceEquals(projectModel, projektyModel);  // true

Answer 2

This is better accomplished with generics. You can use a constraint on the generic typeparam to preserve your type safety!

private T SetGrid<T>(T grid) where T : IGrid
{
    grid.PagerHelper.SetPage(1, 10);
    grid.SortHelper.SetSort(SortOperator.Ascending);
    grid.PagerHelper.RecordsPerPage = 10;

    return grid;
}

and then

var projectModel = new ProjectModel();
projectModel = SetGrid(projectModel);

Here, the generic typeparam "T" is actually inferred by the compiler by the way you call the method.

It's worth noting that in the particular use-case you've demonstrated, returning grid is probably unnecessary, as your original variable reference will be appropriately modified after the method call.

Answer 3

Since you are passing in an object of a class that implements IGrid you could just as well change the return type to IGrid.

Also, since it's a reference type you don't even need to return the grid again. You could just as well use this:

var projectModel = new ProjectModel();
SetGrid(projectModel);

Answer 4

In the case you illustrate above there is no need to return grid. The IGrid instance is passed by reference, so your projectModel reference will be updated with the changes you've made in the SetGrid method.

If you still want to return the argument, at least return IGrid, since it is already known that the argument is an IGrid.

In general, provide as much type information as you can when programming in a statically typed language/manner.

Answer 5

Your grid is an IGrid, why not return IGrid?

Answer 6

"Is this a common practice or should I avoid doing this ?"

This is not common practice. You should avoid doing this.

1. Functions that only modify the parameter passed in should not have return types. If causes a bit of confusion. In the current C# you could make the modifying function an extention method for better read-ability.

2. It causes an unnecisary cast of the return type. It's a performance decrease, which may not be noticable... but its still needless since you are casting from an interface, return that interface even if the object is different from the parameter passed in.

3. Returning object is confusing to users of the function. Lets say the function created a copy and returned a copy... you would still want to return the interface passed in so that people using the function know "hey i'm getting an IGrid back." instead of having to figure out what type is being returned on thier own. The less you make your team mates think about stuff like this the better, for you and them.

Answer 7

This is a very weird example because SetGrid doesn't seem to do a lot of things other than setting some defaults. You are also letting the code perform manipulation on the object that could very well do that by itself. Meaning IGrid and ProjectModel could be refactored to this:

public interface IGrid {
    // ...
    public void setDefaults();
    // ...
}

public class ProjectModel : IGrid {
    // ...
    public void setDefaults() {
        PagerHelper.SetPage(1, 10);
        SortHelper.SetSort(SortOperator.Ascending);
        PagerHelper.RecordsPerPage = 10;            
    }    
    // ...
}

Using this refactoring you only need perform the same with this:

myProjectModel.setDefaults();

You could also create an abstract base class that implements IGrid that implements the setDefaults() method and let ProjectModel extend the abstract class.

---

what about the SOLID principles ? Concretely the Single Responsibility Principle. The class is in the first place something like a DTO. – user137348

I'm exercising the Interface Segregation Principle out of the SOLID principles here, to hide the implementation from the client of the class. I.e. so the client doesn't have to access the internals of the class it is using or else it is a violation of Principle of Least Knowledge.

Single Responsibility Principle (SRP) only tells that a class should only have one reason to change which is a very vague restriction since a change can be as narrow and broad as you want it to be.

I believe it is okay to put some configuration logic in a parameter class if it is small enough. Otherwise I'd put it all in a factory class. The reason I suggest this solution is because IGrid seems to have reference to PagerHelper and SortHelper that seem to be mutators for IGrid.

So I find it odd that you mention the class being a DTO. A DTO from a purist sense shouldn't have logic in it other than accessors (i.e. getter methods) which makes it strange that ProjectModel itself has references to PagerHelper and SortHelper which I assume can mutate it (i.e. they're setters). If you really want SRP the "helpers" should be in a factory class that creates the IGrid/ProjectModel instance.