Painfully Stupid Method Names in Legacy Code: Fix or Leave as Warning? [duplicate]

Question

This question already has answers here:

Best Practices for Renaming, Refactoring, and Breaking Changes with Teams (3 answers)

Closed 7 years ago.

For this case let's assume something like... "removedNonPriceChangingConfermations" that is in no way relating to things that happened in the past tense, nor does it return a list of removed items (which you would never need in this context).

If you can't spot the other thing wrong with it, I challenge you to a high-stakes spelling contest.

The Dilemma:

Is it better to rename the method to more accurately and less painfully describe what it actually does?

Or given that it sits within a 20-class roller coaster of fail (mapping a db queried table to a bean) is it better to leave it as a sort of a warning buoy of the unique minds that crafted the code with comments to explain what it really does at the definition? At least until we can properly refactor the silly thing.

If relevant, assume a high turnover rate but always good intentions.

Please Note: It's not hard to change the method name. This question is more generally about whether it isn't better to leave a really dumb method name intact so people can see that they're walking into "one of those parts" of the code base or if I should in fact change it to better reflect what it actually does.

Answer 1

Is it useful as a warning sign? Not really. It will just confuse people and be hard to remember.

A well named function that tells someone unfamiliar what hairy code actually does is very useful. Bad or confusing code, which you imply is in the function, can be horrible to work out. If you've done that and can give a good description for the function in the name then you've made the next programmer's job much easier.

Commenting what it does is also useful, but comments are not always read, but the function name is.

Broken windows should always be fixed. Not fixing the name will leave a broken window, and encourage the next programmer not to start re-factoring.

Answer 2

• Grep the entire code base for the name of the method. Find out how public it is, how many configs mention it, etc.

• Rename the method mercilessly (a good IDE can do this), grep again, manually update any references that the IDE has missed. Add a comment explaining method's peculiarities, if any.

• If the method is not public (only has package visibility or narrower), you're done!

• If the method is somehow public (exposed to 3rd parties or just has public visibility for no apparent reason), create a method with the old name that calls the newly renamed method and logs the fact. This way you will find out what other clients of this methods are. If these are under your control, eventually make them use the new name.

• Run tests. If you don't have a test for that method, or for some of its clients, it's a good moment to add them.

Answer 3

Refactor it, but make a hardprint first. Frame it. And put it on a fame hall of shame.

Answer 4

How I would approach this:

If refactoring is actually going to happen at some point then I would leave it as is with a comment.

If it's only called in a few spots and has good test coverage then I would change it.

If neither of those situations apply but it keeps you up at night I would change it.

Else, move on to something more important.

Answer 5

I'd probably fix it, especially if I had some work to do in that area... No need to leave bad code around. Examples of good names trump examples of bad names.

But, some things that might affect that decision are whether I can test it for unintended consequences. Especially if it's public and from an interface in legacy code.

I ran into that today, in fact, a method that was public and in an interface, which didn't seem to be referenced from anywhere, until I ran the application and found out a legacy framework in our app that is configured with method names in XML used it.

If I couldn't confirm that things still worked correctly after the change, I'd probably just leave it to ferment.

Answer 6

I would fix it. The question infers Java, but I code in C# for a living and own a copy of ReSharper, and so changing the name of a code member, from a private field all the way up to the namespace root, is pretty simple; right-click->Refactor->Rename. So, I just do it, because it really is just that simple (99% of the time; if I'm working in libraries shared between distinct codebases, I might be more cautious as quite a bit of our legacy code has no automated build in TeamCity).

I do not know what kinds of tools along these lines are available for Eclipse, but JetBrains also writes IntelliJ IDEA which has many of these types of refactor operations built right in.