7

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.

8
  • 3
    Does the language have static type checking? Refactoring a method name in C# takes seconds.
    – Mike
    Commented Jul 9, 2013 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
    Commented Jul 9, 2013 at 19:48
  • 1
    Given the reference to "a bean", it's probably Java. Eclipse has good refactoring support as well.
    – alroc
    Commented Jul 9, 2013 at 19:50
  • I assume there's no reflection going on anywhere that would ping the method by that name? ... right?
    – user40980
    Commented Jul 9, 2013 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
    Commented Jul 9, 2013 at 20:27

6 Answers 6

Reset to default
4

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.

2
  • Sorry for the delay. I just assumed they were going to lock in it in five seconds at the time I imagine and you actually stayed on the question.
    – Erik Reppen
    Commented Jul 23, 2014 at 4:43
  • Note: while broken windows should always be fixed, not all broken windows should be fixed immediately when you see them. There may be some broken windows that are worth leaving around. One case in particular: V&V'd code (or worse VV&A'd code) can be very expensive to change if your business model requires re-verification or re-accreditation. In those cases, it is better to wait for an opportune moment before striking.
    – Cort Ammon
    Commented Mar 16, 2015 at 6:13
17

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

8
  • 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")
    – user40980
    Commented Jul 9, 2013 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
    Commented Jul 9, 2013 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.
    – user40980
    Commented Jul 9, 2013 at 20:45
  • 2
    +1 I wish I could give this +10. Idea #4 is very creative.
    – Brandon
    Commented Jul 10, 2013 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
    Commented Jul 10, 2013 at 3:54
3

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

1

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.

0

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.

1
  • 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
    Commented Jul 9, 2013 at 20:51
0

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.

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