2

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.

Improve this question
1
  • 1
    imho, that's a poor argument, I support refactoring. Commented Mar 7, 2012 at 18:37

5 Answers 5

Reset to default
5

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

This is a good solution but creating one more class hierarchy just for one method does not make sense to me. AbstractAgent is something standard and everybody would be extending this class. Asking them to use AbstractAgentWithFilePath is little difficult. But logically you are right.
I like this, except I think he may be criticized for introducing a new class in the hierarchy entirely based around what is probably an insignificant utility method. Oh well.
3

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.

Improve this answer

1 Comment

getFilePath() method functionality is same in all the classes
3

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.

Improve this answer

9 Comments

Makes sense. This is inline with my thoughts.
Not true in general. Overload resolution can easily be a problem and break compatibility.. "Sorry we added a function and now your code is calling the wrong function" is a great way to make friends with clients of your library..
+1 to this line of thought. Adding an overloaded method should not break backwards compatibility. Worst case scenario you give it a different name and then it definitely does not break backwards compatibility.
@Perception It certainly can break backwards compatibility and there's no way you can guarantee that a function name isn't in use by some arbitrary external subclass..
@Voo - hmmmm, please elaborate how adding a new method can break backwards compatibility with existing code, based on the user case the OP described.
|
0

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,.

Improve this answer

Comments

0

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.

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.