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.

Answer 2

None of your examples are bad. They are just design choices. Dropping the accessors to a and b in the second example seems a step backwards to me. As to putting implementation dependent save code in the class definition, that would be bad if there were multiple types that all needed to define the save. There you would be better to define a parent class with the save function and then inheriting from that class. However, if it’s just you writing code and there is just that one class it doesn’t matter.

Good that you are thinking about what makes good code. As a general rule, good code is code that works and which you can return to in 6 months and modify easily in the future. If you have a group of developers then of course provide accessors.

Another aspect of good code is having unit tests. If you change something and the unit tests pass you’ve done your job. If someone is using internals they should write a unit test that will signal a change that would break their code.