Should new fields and operation of logical entity be in one single class even the existing class has thousands of existing lines

Question

In a legacy Java project, when adding a new feature - an existing email field can be editable or not editable on base of its parent system, a developer added a new feature by adding a new class, which is added as an instance field to the existing class containing thousands of lines because the logic of deciding the editability of the field is not trivial.

However, one code reviewer commented that all fields and operation of logical entity should be represented by single class.

Question: should all fields and operation of logical entity be represented by single class even though that single class has already thousands of lines

Answer 1

If the new class defines a good abstraction on its own, and a responsibility on its own, then it is a probably good idea to introduce this new class.

If, however, the new class can only be implemented with some ugly cyclic dependency to the existing one, and it would only be an artificial attempt to keep the number of LOC in the existing class at its current size, then the new class makes not much sense.

Even then it might be a good idea to look for occasions for at least some improvements by refactoring. I would always try to apply the "boyscout principle": leave the code in a cleaner state behind than it was before.

In your comment above, you mentioned the new class EmailField with just one (public?) method isEditable. That sounds like a good abstraction at a first glance. However, EmailField should not depend directly on the existing class A (like by getting an object of type A in the constructor). There should either be explicit parameters of simple types, or callbacks, or an interface type for the constructor parameter, which makes it possible to unit test EmailField in isolation, apart from A.

Answer 2

However, one code reviewer commented that all fields and operation of logical entity should be represented by single class.

An entity may consist of many sub-systems, and each sub-system should exist as it's own type - it should not all be handled by one class.

---

A Person may have a name and the ability to change names. But they may also require the ability to search for employment.

Having all the employment properties shoved into the Person class, along with other responsibilities a person may require, would make the Person class hard to maintain.

---

You should think of your code in terms of responsibility. If you need the ability to find employment, create a type for it.

From there, you can decide who composes the responsibility.

Answer 3

Yes.

This is akin to the sunk cost fallacy where people feel like they're already too far down a path to change it. Making a new small class isn't much more work than adding a new field, and helps your code be more flexible and testable and maintainable. In an ideal world, you'd break up the thousand of line class too.

Practical matters might mean you still just add the field to the giant class, but you should at least make that trade-off knowing that you're making things worse.