3

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

Improve this question
4
  • Does the new class define a good abstraction on its own, and a responsibility on its own? Then it is a good idea to use a new class. Or is the new class just introduced to avoid the further increasement of number of number of code lines of the existing class, but it does not provide a good abstraction? Then you can try to extend the existing class (but even then it might be a good idea to look for occasions for at least some improvements by refactoring). Try apply the "boyscout principle": always leave the code in a cleaner state behind than it was before.
    – Doc Brown
    Commented Sep 24, 2019 at 19:11
  • The new class the developer defined is called EmailField, which has only one method isEditable() currently, i.e. should the email field be editable or not, and this new class indeed has dependency on the existing lengthy class (assume the name is A), meaning EmailField has a constructor EmailField(A parent) and the isEditable() method is not of a trivial one line of code, then in the existing class, the EmailField is initialized as new EmailField(this)
    – Rui
    Commented Sep 24, 2019 at 19:52
  • EmailField sounds like it could be a good abstraction. However, the cyclic dependency is something one should consider to get rid off, especially if the existing class "A" offers a broad public interface, whilst EmailField only uses a small part of that interface. Maybe the constructor of EmailField could have a signature with some explicit parameters, maybe some of them just delegates/callbacks? That would it make probably simpler to write a unit test for EmailField, to test it in isolation apart from the class A.
    – Doc Brown
    Commented Sep 24, 2019 at 19:56
  • Thanks a lot for your comment. You can answer it a bit formally as an answer here. Then I will vote for it probably
    – Rui
    Commented Sep 24, 2019 at 20:12

3 Answers 3

Reset to default
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.

Improve this 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.

Improve this answer
4
  • The continuous defence comment on adding code to the same class is: 1. Having long class will not create problem as long as methods are short and perform one single operation (Abstraction). 2. adding more classes as dependency of the existing class might create quite many instances, which might be a burden on the server
    – Rui
    Commented Sep 24, 2019 at 8:08
  • @Rui If you aren't worried about maintenance, then take the path of least resistance. Having all those responsibilities in one class will make it harder to determine which methods belong to which system. This becomes a bigger problem if you have multiple developers touching the code base. But if those aren't worries of yours, then putting the new code in the class is perfectly fine as mentioned in my initial comment (before migration)
    – Dioxin
    Commented Sep 24, 2019 at 13:02
  • The code is actually maintained by multiple developers. your last sentence "putting the new code in the class is perfectly fine as mentioned in my initial comment", do you mean the puuting the new code in the existing class or the new class is fine? According to my understanding, the code reviewer's worry is that if hundreds of new classs instances are added to the existing class, that would take lots of memory. But should this really be a problem when nowadays memory is kinda unlimited?
    – Rui
    Commented Sep 24, 2019 at 13:23
  • @Rui I meant the existing class. As for the memory thing, that's pre-optimization, and you shouldn't let "high memory usage" affect your decisions until you've proven it's a problem through profiling. If you're worried about heap usage, you could look at ways to reuse objects (flyweight pattern). With multiple devs, having multiple responsibilities in a single class can easily create conflict in version control systems, when 2 developers modify 2 separate responsibilities, but are required to modify the same class to do so.
    – Dioxin
    Commented Sep 24, 2019 at 13:40
2

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.

Improve this answer
4
  • Not sure if sunk cost applies here - it can still be recovered for all we know. The question seems more relevant to "all fields and operation of logical entity should be represented by single class" than the cost of previous poorly written code in the past.
    – Dioxin
    Commented Sep 24, 2019 at 2:43
  • @VinceEmigh - but it is using the “this class is already like this” as the only reason given to not do what they know is right...
    – Telastyn
    Commented Sep 24, 2019 at 3:27
  • The continuous defence comment on adding code to the same class is: 1. Having long class will not create problem as long as methods are short and perform one single operation (Abstraction). 2. adding more classes as dependency of the existing class might create quite many instances, which might be a burden on the server
    – Rui
    Commented Sep 24, 2019 at 7:32
  • @Telastyn The OP is using "my co-developer used a new class" as the background for the question. It's not a "it's already that way, no way of fixing it", but a "my co-dev did this, does it make sense?"
    – Dioxin
    Commented Sep 24, 2019 at 13:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.