28

I have a Java class that is becoming long. When I run it through code quality tools, I get flagged for number of lines in the class.

This is a lower layer class, used by upper layers with Spring's @Autowired. The class has a lot of private instance methods which are not static. They don't use any instance fields, working only on method parameters.

Can I safely move these methods as public static in some separate utility class? What are the downsides?

Luuklag
  • 3,897
  • 11
  • 38
  • 57
Sabir Khan
  • 9,826
  • 7
  • 45
  • 98
  • You can move them into a *package-private* class, which lets you separate your stateless methods from other code without exposing them outside your package. – dimo414 Jun 28 '17 at 09:41
  • I wonder, how many instance variables you have in the class? – fabfas Jun 28 '17 at 10:12
  • @fabfas : two - one is an `@Autowired` `JdbcTemplate` & other one is a `String` injected with `@Value` – Sabir Khan Jun 28 '17 at 10:29
  • The “JdbcTemplate” instance variable suggest that this class will be executing the JDBC workflow. In my opinion, the database interface layer should responsible for grabbing the data, populating the “data records” and implement the interface in the application layer. – fabfas Jun 28 '17 at 11:05

5 Answers5

67

"Wrong" mindset here.

You do not rework your classes because tools are complaining about this or that.

You want to improve the quality of your source code; and such tools can help with identifying "topics worth thinking think about". You take their feedback as hint; not as "order".

Therefore you don't worry about "lines of code" within a class. Instead, you worry about the responsibilities that this class has. Meaning: the line number count isn't a problem per se - but violations of the Single Responsibility Principle are.

Thus: you step back and check out what exactly your class is doing. And when it is clearly doing more than one thing, you extract such aspects into other classes!

Meaning: if you actually find that all this code "belongs" to the responsibility of that class; then keep it in there. Don't start putting internal implementation details into unrelated helper classes; just because some tool warns you about number of lines.

On the other hand, turning private methods into something that is static/package protected would allow you to unit test those methods. Which could be an advantage. But as said: as long as we are talking about implementation details, they should stay private; and they shouldn't be unit tested anyway.

Long story code: know and understand what "clean code" is about; and try to follow the ideas outlined there.

starball
  • 20,030
  • 7
  • 43
  • 238
GhostCat
  • 137,827
  • 25
  • 176
  • 248
4

The division of methods should be by purpose/application/logic (you name it), not by technical properties.

A long source code can probably be separated into several small size classes, each with its own separate purpose/responsibility.

Sharon Ben Asher
  • 13,849
  • 5
  • 33
  • 47
  • As of now, those private methods are used by only containing class but behavior is similar to static utility methods since no reference to class instance or static fields is made in those methods. I guess, these can be moved but I should try to group in classes with similar purpose. – Sabir Khan Jun 28 '17 at 09:37
2

I constantly use Utility classes that contain static methods and find it very convenient. If the methods are really for use by a single class only you may place your Utility class as static private class within your original class. But I found out that in many cases general static utility methods can be used by several classes, so in this case I d create a separate utility class with set of static methods that could be used by several classes.

Also, I strongly agree with GhostCat's answer in terms of general way of thinking. As for the size of a class - it could be a concern but usually I don't worry about that much. What I really look for is for the size of your methods. I like methods to be short and self-explanotory, starting with its name and the parameters names and order and to the logic. If there is large chunk of internal logic - extract it into separate method. That makes code by far better readable and maintainable.

Michael Gantman
  • 7,315
  • 2
  • 19
  • 36
1

Quality of code cannot be measured by number of lines. If you find like the class file is growing day by day make sure your class follows Single Responsibility Principle.

When your class doesn't follow the principle, separate them into individual classes and make the classes into a package.

Else you could make your base class as an abstract class and make your util methods as abstract. Have an child class which extends the base class providing implementation for abstract methods in the base class.

Here is a good SO answer on when you could make your methods as static

As said by @Zack Jannsen ,

"static" is often valuable when you know something is not going to change across instances. If this is the case, I would really consider the "Single Responsability Principle", which implies a class should have one responsability and thus only one reason to change. I feel one should consider moving the "ConvertMpgToKpl(double mpg)" function, and similar methods, to their own class. The purpose of a car object is to allow instantiation of cars, not provide a comparison between them. Those should be external to the class.

Though static allows you to access the method without object creation, always remember the following when trying to make the method as static

  • The method produces consistent result based on the input arguments
  • You always need that method to be in memory (ie; you need the method quite often) - It could be better to access a huge method which would be accessed only a very few times with the help of object creation rather to access it via static
  • Utility methods which are used quite often could be made as static
Tom Taylor
  • 3,344
  • 2
  • 38
  • 63
0

In my opinion using private methods that are doing some "magic" is a bit dangerous because of the single reason that you cannot really unit test it. Yes of course you can unit test the public method that is calling that private method but think about the asymptotic complexity that you will have. Usually I prefer to have utility class for such methods and write some good amount of unit tests.

Tano
  • 1,285
  • 1
  • 18
  • 38