When is a class too big or too small? [closed]

Closed. This question is opinion-based. It is not currently accepting answers.

---

开发者_开发问答

Want to improve this question? Update the question so it can be answered with facts and citations by editing this post.

Closed 4 years ago.

Improve this question

I recently had my code reviewed by a senior developer in my company. He criticized my design for using too many classes. I am interested to hear your reactions.

I was tasked to create a service which produces an xml file as a result of manipulating 3 other xml files. Let's name these aaa.xml, bbb.xml and ccc.xml. The service works in two phases. In phase one it scrubs aaa.xml against bbb.xml. In the second phase, it merges the product of phase one with ccc.xml to produce a final result.

I chose a design with three classes: an XmlService class which used two other classes, a scrubber class and a merger class. I kept the scrubbing and merging classes separate because the both classes were large and featured distinct logic.

I thought my approach was good because it kept my classes small and cohesive. My approach also helped to control the size of my test class.

The senior developer asserted that the scrubbing and merging classes would only be used by the XmlService class, and should therefore be part of it. He felt this would make the XMLService cohesive and this is what being cohesive means according to him. He also feels that breaking up classes this way makes them loose cohesiveness.

The irony is I tried to break these classes to achieve cohesiveness. What do you think? Who is right or wrong? Are we both right? Thank you for your suggestions.

---

If you follow the single responsibility principle (and based on the tone of your question, I think you do follow it), then the answer is clear:

1. A class is too big when it does more than one thing; and
2. A class is too small when it fails to fulfill its purpose.

That's very broad and indeed subjective -- hence the struggle with your colleague. On your side, you can argue:

• There's absolutely no problem in creating additional classes -- It's a non-issue, compile-wise and runtime-wise.
• The current implementation of the service may suggest that these classes "belong" to it, but that may change.
• You can test each functionality separately.
• You can apply dependency injection.
• You ease the cognitive load of understanding the inner working of the service, because its code is smaller and better organized.

Furthermore, I think your boss has a misguided understanding of cohesion. Think of it as focus: the narrower the focus of your program, the higher the cohesion. If the code on your satellite classes is merged within the service class, the latter becomes less focused (less cohesive). It's generally accepted that higher cohesion is preferred over lower cohesion. Try to enlighten his/her view about it.

---

Cohesion is a measure of how strongly related is the functionality within a body of code. That said, if merging and scrubbing aren't strongly related, including them both in the same class reduces cohesion.

The Single Responsibility Principle is also worth mentioning here. Creating a class for the sole purpose of scrubbing and another for the sole purpose of merging follows this principle.

I'd say your approach is the better of the two.

---

What would you name the classes? ScrubXml and MergeXml are nice names. ProcessXML and ScrubAndMergeXml aren't, the first being too general and the second having a conjunction. As long as none of the classes rely at all on the internals of one or the others (i.e., low coupling), you've got a good design there.

Names are very useful in determining cohesion. A cohesive module does one thing, and therefore has a simple specific name. A module that does more than one thing is less cohesive, and needs a more general or more complicated name.

One problem with merging functionality in X into Y if X is only used by Y is the reductio ad absurdam: if your call graph is acyclic, you'll wind up with all your functionality in one class.

---

As someone who is coming back from the GodClass building fest of several years in duration, and now trying very hard to avoid that mistake in the future; the error of making a 6000 to 10000 line single source unit with a single class, with over 250 methods, 200 data fields, and some methods over 100 lines, the single responsibility principle looks like something worth defending against the predilections of your unenlightened supervisor.

If however, your supervisor and you are disagreeing over a matter of whether 2000 lines of code belong in one class or three, then I think you're both closer to sane, than I was. Maybe it's a matter of scale and perspective. Some object oriented programming aficionados like a certain "Coefficient of Standalone" per class, in direct violation of the generally understood ideas about how to improve cohesion and minimize coupling.

A set of 3 classes that work well together is, objectively, a more object-oriented system, than a single class that does the same thing. one could argue that if you write an application using only one class, that the application is not really object oriented at all.

---

If the scrubber and merger are not meaningful outside the context of the main class, then I agree with your reviewer, particularly if you've had to expose any implementation details in order to allow this separation. If you're using a language supporting nested private classes or something similar, that might be a reasonable alternative to maintain the logical separation without exposing implementation details to outside consumers of the main class.

---

This is a very subjective area, and will be different depending on coding and style guidelines, and who approves your code.

That said, if your defense of your design didn't hold up, and your senior team member still insisted on merging your classes, then you have to compromise:

You've already got the logic separated out. Write the one service class and keep the methods separate like other good design, and then write a glue method. Add some comments above each method explaining how they could easily be partitioned to multiple classes if the need arises in the future.