CODEWITHSUNDEEP
javainheritancestatic
user977263
user977263

Reputation: 57

Java Inheritence or static method

There is an abstract class called AbstractAgent and currently there are 27 classes who are extending this class.

I have started developing some agents and all my 5 agents are extending AbstractAgent class.

Now I observed that there is a getFilePath() which I am copy-pasting in all my 5 agents. Then I realized that out of existing 27 classes also there were many classes who had this method. I thought it would be a good idea to put this method in the base class AbstractAgent and let everybody use this method. But I dont want all existing classes to change their code I have changed the method name so that going forward anyone can use it.

The person who did my code review suggested me not to touch AbstractAgent class because it is already being used by existing clients and put this method is some utility class.

I am not convinced by his argument. Anybody want to pitch in their thoughts.

Upvotes: 2

Views: 201

Answers (5)

Mike Clark
Mike Clark

Reputation: 10136

There's no right answer here. Only you, your coworkers, and your company know how and where AbstractAgent is used, and how changing it impacts various different teams, departments, or customers. Decisions about where to locate code are often more political and social than driven by "clean room" design choices.

From a purely code perspective, I support radical refactoring: get rid of ALL the duplicate code in ALL subclasses of AbstractAgent, and move everyone to the new getFilePath() in AbstractAgent.

But you may not have the authority to do this, and you may get pushback. Or maybe getFilePath() varies slightly sometimes (you were not clear about this.) In that case, whatever choice you make seems like a compromise. Your code reviewer may be pushing back against 'getFilePath2()' in AbstractAgent because it sheds light on the problem of imperfect code reuse and feels like it is "polluting" the superclass with a temporary workaround to what should really be solved by bringing everyone up to date with truly common shared code in the superclass. Who knows.

Perhaps a static utility method is a better choice until such time as a complete refactoring can be done.

Upvotes: 0

Óscar López
Óscar López

Reputation: 236140

If the getFilePath() method is exactly the same for all of AbstractAgent's subclasses, then by all means go ahead and pull it up to the abstract class (use your IDE's refactoring tools), delete it from the subclasses then run unit tests (you do have unit tests, right?) and check that everything is working fine.

If there are differences between some implementations of getFilePath(), but most of them are identical, it's still a good idea to pull the most common implementation up to the abstract class, delete it from the subclasses that use it and override the method in the subclasses in those cases where the implementation differs.

Now, if there's just too much variability between the different implementations of the method, leave the code base untouched.

Defining a static method in a utility class (say, FileUtil) makes sense only if the method is not directly related to the functionality of the Agent classes, or if the method could be of use in some other part of the object hierarchy, not directly related to agents.

Upvotes: 3

simaremare
simaremare

Reputation: 417

if the method is tightly related to the Agent classes, put it inside the abstract is a good option, but if it is not, put it in another utility class which handles all File related actions would be acceptable,.

Upvotes: 0

Oscar Gomez
Oscar Gomez

Reputation: 18488

In this case because the AbstractAgent class is already being used by other classes, and as you mentioned yourself NOT all the classes implement this method then you should indeed NOT change the AbstractAgent. You can however extend AbstractAgent in another abstract subclass called say AbstractAgentWithFilePath, which declares that additional method you want, and then have those classes extend the AbstractAgentWithFilePath class instead.

Upvotes: 5

Eugene Kuleshov
Eugene Kuleshov

Reputation: 31795

Adding common logic to the parent class is one of the strong sides of abstract classes and such addition is a forward compatible change. Unless, of course, you don't know about some uses of this class and your newly added method has the same name. But even then, if subclass overwrite such method, its own method will be invoked, so it is still a forward compatible change.

Upvotes: 3

Related Questions