Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

This question already has an answer here:

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.

share|improve this question

marked as duplicate by MichaelT, Robert Harvey, BЈовић, thorsten müller, Yusubov Jul 10 '13 at 16:54

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

3  
Does the language have static type checking? Refactoring a method name in C# takes seconds. –  Mike Jul 9 '13 at 19:45
3  
While continual improvement is a nice thing to engage in, it may not be budgeted and it may not be consequence free. strategy for refactoring API legacy method names –  JustinC Jul 9 '13 at 19:48
1  
Given the reference to "a bean", it's probably Java. Eclipse has good refactoring support as well. –  alroc Jul 9 '13 at 19:50
    
I assume there's no reflection going on anywhere that would ping the method by that name? ... right? –  MichaelT Jul 9 '13 at 20:10
    
It's not that it's hard to change the method name. It's whether I want more clarity in meaning or maintain clarity that the code is about to take a very sharp nodedive into the Twilight Zone. –  Erik Reppen Jul 9 '13 at 20:27
show 1 more comment

6 Answers

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

share|improve this answer
5  
deprecate the method too (the deprecated annotation will cause compiler warnings and can be used to fail the build). The real danger comes from computed reflections like foo.getClass().getMethod("" + (active?"active":"removed")+"Non" + (price?"Price":"Cost") + "ChangingConfermations") –  MichaelT Jul 9 '13 at 20:34
    
@MichaelT: verily. But if you have code like that, you have bigger problems than the spelling error in word 'confermations'. This is why I propose to add logging to detect if anyone is secretly calling the method by excessively contrived reflection. –  9000 Jul 9 '13 at 20:44
    
Absolutely, and your logging will catch it. I was more thinking of the deprecate it and catch anything that calls it (possibly outside of the immediate project) at compile time. –  MichaelT Jul 9 '13 at 20:45
2  
+1 I wish I could give this +10. Idea #4 is very creative. –  Brandon Jul 10 '13 at 0:22
    
It's not our biggest problem. It's just a dilemma I run into regularly. Add to clarity of what a method does even though sometimes some filter setting off an absurd redirect chain might just pull the rug out from under it (nothing quite like seeing a step-through just stop on whitespace and leap out of the middle of a method to something that wasn't called)? Or just leave it nice and odious until we've ripped the silly 20-30 class beast out and replaced it with something reasonable that's more like one class, maybe two? –  Erik Reppen Jul 10 '13 at 3:54
show 1 more comment

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

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
    
This code's probably been here over ten years. The app is actually run on a triple-stack of Rails, Java, and .NET. Rails got tacked on when somebody decided a prototype was as good as a brand new feature (and not for the first or last time as I understand it). We're trying to get a gradual but continuous clean-up process going but any time we refactor as we go we get slapped on the wrists for not staying on our other goals (which is somewhat understandable given current circumstances, yet ironic given that the circumstances have much to do with the code base being a disaster). –  Erik Reppen Jul 9 '13 at 20:51
add comment

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.

share|improve this answer
add comment

Not the answer you're looking for? Browse other questions tagged or ask your own question.