Is the practice of passing parent object as reference for parent object interaction Evil?

Question

In some code reviews of legacy c# code i've been doing recently I've seen a number of examples like this:

class ManagerParentClass
{
    public string CustomProperty{get;set;}
    public void Log(string message);

    void DoABunchOfTasks()
    {
       new SomethingService().DoSomething(this);
    }
}

with the following:

public class SomethingService
{
    ManagerParentClass _manager;

    void DoSomething(ManagerParentClass manager)
    {
        _manager = manager;

        // do something
        _manager.CustomProperty = "hello world";
        _manager.Log("said hello world");
    }
}

While this works fine on the surface, I'm concerned that this is a bit of an anti-pattern that may cause evil things with garbage collection.

Will this break the generational garbage collector in .Net's ability to clean the parent and child objects up properly or anything else negative?

Answer 1

Oh yes, this is a terrible anti-pattern in general. I work with a code base that uses this a lot and it is sheer madness.

The biggest offense? Violation of encapsulation and the tight coupling between the classes that comes with it: SomethingService knows too much about ManagerParentClass, and ManagerParentClass gives up control of itself to SomethingService.

Two better alternatives:

1. Make DoSomething() an instance method of ManagerParentClass, this is in closer keeping with an essential point of object orientation: data structures carry their operators
2. Make SomethingService a pure method that does some calculation and returns a value, the caller may then make the assignment to the ManagerParentClass

Of course, both of these refactorings involve an end-game mutation of ManagerParentClass, and coming from a functional programming angle, I would try to avoid that altogether. But without more information, I can't recommend a course for that.