Feature envy, encapsulation, active record, separation of concerns? When its bad?

Question

you all say, object oriented programming is about encapsulation, data hiding. Let's given this example:

class Rectangle
{
    private int a,b;

    public function __construct(int a, int b)
    {
        this.a = a;
        this.b = b;
    }

    int public function getA()
    {
        return a;
    }

    int public function getB()
    {
        return b;
    }
}

var r = new Rectangle(3, 4);
var area = r.getA() * r.getB();

this is a bad code then, so let's refaktor:

class Rectangle
{
    private int a,b;

    public function __construct(int a, int b)
    {
        this.a = a;
        this.b = b;
    }

    int public function getArea()
    {
        return a*b;
    }
}

r = new Rectangle(3, 4);
area = r.getArea();

way better, data hiding is done and getArea is brought where it belongs to. Ok then, here comes the Active Records:

class Record
{
    private int ID;
    private string username;

    public function __constructor(int ID, string username)
    {
        this.ID = ID;
        this.username = username;
    }

    int public function getID()
    {
        return ID;
    }

    string public function getUsername()
    {
        return username;
    }
}

r = new Record(1, 'test');
dbEngine.save(r);

this is again bad, since all data is public. (altough Doctrine works this way) But if I do that as Propel did:

class Record
{
    private int ID;
    private string username;

    public function __constructor(int ID, string username)
    {
        this.ID = ID;
        this.username = username;
    }

    public function save()
    {
        dbEngine.save([ID, username]);
    }
}

r = new Record(1, 'test');
r.save();

this is also said bad, because Active Records are antipattern. Then when it's good or bad? When does an "act" (getArea, save) should be brought inside an object - and when does it act outsidely?

Answer 1

You can inject the dbEngine dependency in for your specific case, but this doesn't address your concern.

In general, what makes your code good is how easy it is to understand, and how close changes in intention are tied to changes in implementation.

The problem with revealing private internals are that you're exposing your inner values that programs which interface with your program may rely on (and make difficult to change later on). A record is basically a struct/dataclass - it represents a collection of values that goes together with some well-defined meaning. Without knowing the rest of the code I can't say if this specific class is like that, but if that's the case it would be okay to just make it a struct (all members public, no methods).

There aren't any catch-all rules that makes code 'good'. It's a continuous process of making mistakes or being inefficient, and analysing what code led or made more likely that problem. Code smells are just the result of lots of trial and error by others, and although very robust in most cases may sometimes be outdated and should be applied in the specific situation when they improve your code.