74

One of the methods that I commonly use in our codebase is misspelled (and it predated me).

This really irritates me not simply because it is mispelled but more importantly it makes me ALWAYS get the name wrong the first time I type it (and then I have to remember "Oh, right, it should be mispelled to this...")

I'm making a few changes around the original method. Should I take the opportunity to just rename the freaking method?

One Two Three
  • 1,019

6 Answers6

138

Should I take the opportunity to just rename the freaking method?

Absolutely.

That said, if your code has been released as an API, you should also generally leave the misspelled method and have it forward to the correctly named method (marking it Obsolete if your language supports such things).

Telastyn
  • 109,398
  • 33
    The idea of "forward to the correctly named method" is simple and brilliant. Interesting link on the broken windows theory, as well. – dev_feed Jun 11 '14 at 21:03
  • 2
    Note that if this is part of a public API, this may not be a backwards compatible change (depending on language). This is not as unlikely as it may sound.. if I had to inherit from an existing API with a misspelt name I'd definitely fix that. – Voo Jun 11 '14 at 22:58
  • 10
    @voo - eh? What language would make adding a new method (and changing the implementation of one to do the same exact behavior) be not backwards compatible? – Telastyn Jun 11 '14 at 23:29
  • +1 because you're right, but I disagree with the broken windows theory. You're going to have a little bit of clutter, no matter what, and it isn't going to lead to the total decay of the project. Just clean it up as you go along and don't panic. – Kevin Jun 11 '14 at 23:32
  • @kevin - well, isn't that what the OP is asking? If they should clean it up as they're going along? – Telastyn Jun 11 '14 at 23:35
  • @Telastyn Voo said "if I had to inherit from an existing API with a misspelt name I'd definitely fix that". I intercept that as creating you own, correctly spelled method that delegates to the original misspelled method. Depending on the language's namespacing mechanism, on how it solves with name conflicts, and on how it deals with method overriding, adding a correctly spelled method to the library(which will have the same name as the method created in the project that uses the library) may cause compilation errors or infinite mutual recursion. – Idan Arye Jun 12 '14 at 00:14
  • 3
    @Telastyn it can be tricky sometimes to add methods to say web services. Some clients balk at changing WSDLs for example, suddenly refuse to talk to the server. That's an implementation problem in the client, but if the client is an important one you don't want to upset it can very well prevent you from changing your interface. – jwenting Jun 12 '14 at 07:43
  • @Telastyn just what jwenting said (and there are tons of languages that demand that you declare when you override a method - c#, scala, ..) - backwards compatibility is not a simple thing to determine. Taking c# as an example it gets even worse: thanks to extension methods it's impossible (in most situations) to add any kind of public method to an existing class (even if it's sealed) that may not break existing code. – Voo Jun 12 '14 at 09:22
  • 1
    @Kevin " it isn't going to lead to the total decay of the project" I dunno, once the developers get into the habit of turning off the spell checker in their documentation… :) – Joshua Taylor Jun 12 '14 at 11:58
  • 17
    @Telastyn If the misspelt method name is on an interface (as in interface type used in Delphi/Java/C#), then adding a correctly spelt version of the name will probably break all existing implementations of said interface. – Disillusioned Jun 12 '14 at 12:53
  • @IdanArye I reckon you got it backwards. – JF it Jun 13 '14 at 16:10
  • 1
    @CraigYoung Java 8 fixes that problem by allowing a default implementation to be specified by an interface. C# extension methods can be used to achieve a similar result (http://joeduffyblog.com/2010/02/09/extension-methods-as-default-interface-method-implementations/). Not sure about Delphi, though. – Jules Jun 15 '14 at 01:45
  • @CraigYoung Hence why Telastyn proposes forwarding the incorrect to the correct. – OJFord Jun 15 '14 at 13:47
  • @OllieFord - eh, adding the correct method to the interface means that implementers of the interface will probably break, even if the users don't. Technically a breaking change, but far less impactful. – Telastyn Jun 15 '14 at 14:36
52

There are cases where you should avoid doing such refactorings:

  1. If the method is used in a public interface. A canonical example is the misspelling of referrer in HTTP referer, the wrong spelling being kept, because changing the spelling now would have too many repercussions.

  2. If the code base is not covered by any tests. Any refactoring should be done on tested code in order to be able to do regression testing. Refactoring the code base which is not under tests is particularly risky. If you do have a lot of time, start by adding tests; if you work under time pressure, taking a risk of introducing subtle bugs is not the best thing to do if you want to ship on time.

  3. If the method could be used in an unusual way, which makes its uses practically impossible to find (through Ctrl+F or by an automated refactoring tool). For example, in C#, a method can be called through Reflection, making Rename dialog of Visual Studio ineffective. In JavaScript, the function called inside eval() is difficult to find as well. In PHP, variable variables could cause issues.

  4. If the size of the project is huge and the method could be used by other teams. This is similar to the first point, i.e. the interface you provide to other teams can be considered a public interface.

  5. If you deal with a life-critical project. Chances are, the misspelling is not too important to justify a few months of paperwork in order to change the name of the method and ensure it won't cause any patient to receive ten times the authorized radiation or any shuttle to miscalculate its speed.

In any other situation, feel free to rename the method.

Arseni Mourzenko
  • 135,365
  • 1
    +1 for the comment mentioning Reflection, it's bitten me more than once. – DaveShaw Jun 11 '14 at 21:37
  • 34
    If your life-critical project is fragile enough that the slightest refactoring is likely to kill someone, it's fragile enough that no one should trust it with their lives. How are you ever going to be confident that your new feature or streamlined user interface didn't introduce life-threatening bugs if you can't even rename a method? – user2357112 Jun 12 '14 at 06:30
  • 3
    Similarly, if one changed method name causes several extra months of paperwork (rather than, say, mostly getting taken care of by the paperwork for all the other stuff that's changing at the same time), then the programming-to-paperwork balance is skewed by several orders of magnitude, and it's impossible to get any major improvement done without changing the system. – user2357112 Jun 12 '14 at 07:16
  • 8
    @user2357112: I never said that the slightest refactoring is likely to kill someone. It's not about killing someone, but about making everything possible to mitigate the remaining 0.001% risk of having a bug. This requires formal proff. This requires several layers of testing. This requires formalism. This forbids "I wanna quickly rename this method, hopefully it would work!" behavior. Life-critical projects use techniques which would be considered a complete waste of time and money for any business app. That's why they are so reliable (and expensive). – Arseni Mourzenko Jun 12 '14 at 07:41
  • If that takes months specifically for one trivial change to one method, how are you ever going to get through the paperwork for the thousands upon thousands of changes you'll need to do before the next version? I'm not saying to just change it, run the unit tests, and ship it like that. I'm saying your thorough, formal, people's-lives-depend-on-it quality assurance process has to be able to comfortably accommodate this change and the many others like it. Testing may take months, but it can't take more months with this renaming than without if you're to get anything done. – user2357112 Jun 12 '14 at 07:54
  • 5
    @user2357112: as MainMa pointed it out, this is not about casual business apps. It's about special kind of software that are extensively tested/verified. What if the method is called by reflection somewhere? What if some pre/post compiling tool does something with it? What about the documentation? What about other teams using it? Do they use reflection? What if... Real life can be quite complex sometimes. And sometimes it is better to leave a method name untouched rather than verifying if there is any consequences in a bullet-proof manner. – dagnelies Jun 12 '14 at 08:55
  • If you find this in code intended for safety-critical uses, the software is of insufficient quality for its intended use. Safety-critical development processes are meant to find and fix problems in the code. If something so obvious as a misspelling has gotten through, the quality system is sorely deficient. Either reviews haven't taken place, or the reviewers are inadequately skilled. – Ben Voigt Jun 13 '14 at 21:06
  • @BenVoigt: we have misspellings in HTTP protocol. This should have been extensively reviewed too, don't you think? Actually, reviews of live-critical software are more concentrated on mathematical exactness and unambiguity of the program logic. Naming a variable radiatonIntensity instead of radiationIntensity might be a source of a bug, but much less than, say, ratio = 10 / 3.0 (let's guess, is it 3, 3.3333333333333333 or maybe 3.3333333333333335? JavaScript suggests the last one. C# thinks it's the first one. My math teacher would consider the second one to be the least wrong answer). – Arseni Mourzenko Jun 13 '14 at 21:43
  • @MainMa: It's not so much that the misspelling is hiding bugs, as that it is correlated with not being reviewed. People who are familiar with the subject matter will know the correct spellings. At a minimum, they will be a severe distraction from the main goal of inspecting the logic. (Aside: Did you mean 10 / 3, which is 3 in C#? The expression you showed definitely will calculate a non-integer part, although that could be discarded by type coercion) – Ben Voigt Jun 13 '14 at 21:55
  • @BenVoigt: let's talk about reviews. I'm not a native English speaker, and I don't consider I know English very well. However, even I've found misspellings in every book published by Wiley, Microsoft Press, and other publishers which are expected to have well-paid expert reviewers focused on misspellings. I don't even mention magazines and ordinary press which contain a ton of misspellings and even grammar mistakes. So no, for me, a misspelling in a source code written for NASA is not indicative of anything. – Arseni Mourzenko Jun 13 '14 at 22:05
  • @MainMa: I wouldn't consider the code in those books to be production-quality either. – Ben Voigt Jun 13 '14 at 22:07
  • @BenVoigt: I'm not talking about code, but about the content of the books. Also, I'm not talking about Learn in five days-type books which become useless when, two months later, is released, but rather about books such as Code Complete. – Arseni Mourzenko Jun 13 '14 at 22:12
  • Also Consider if there are other code branches that will get merge issues from your rename.... – Ian Jun 13 '14 at 22:35
30

I've done this a few months ago (for different reasons). The steps I took (the language was Perl):

  1. Rename the method. Alias the old name to the new name (this should not break any code, as the method can be called by either name).

  2. Inform the rest of the developers about the name change and why, telling them to use the new name from now on.

  3. Grep the code base for the old name, fix any occurrences.

  4. Log any uses of the old name (using the old name should still work at this time). Fix those cases.

  5. Wait (while doing 4.), until no more entries appear in the log.

  6. Break the alias. Make a method using the old name that throws a fatal exception with a message about the name change.

  7. After some time, remove the method with the old name.

    Of course, your mileage will vary.

Community
  • 1
Abigail
  • 301
6

A good way to not break any existing code would be to chain the new method name to the old in a such as 

private void MyNewMethodName()
{
    TheOldMethodName();
}

and then mark the old method as obsolete (if your language support this). This way any existing code will still work and you can gradually take out all the old spelling mistake out of your code base. Eventually you could even copy/paste the method body into the new method and delete the old one.

/Edit As ivo said in the comment : An even better thing to do would be to move the code from TheOldMethodName into the MyNewMethodName and call the new method from the old one. This one would also have the advantage to help the dev to get their head around where the code belong to.

Rémi
  • 211
  • 2
    I agree with your method; I would do the same. It's the most sane, clear and secure method of refactoring. What also might be a nice way is to move the code from the old method into the new method and make the old one use the new method. When the codebase is a few iterations down the timeline you only need to remove the old deprecated method. – Ivo Limmen Jun 13 '14 at 09:07
  • 1
    @IvoLimmen That's a really good suggestion. This way people get even more their head around using the new method and this would remove a warning from the obsolete call to the old method from within the new one. I will add this to my answer. – Rémi Jun 13 '14 at 11:36
1

Renaming the method:

  • Do it through refactoring lest you have more work to do than you want
  • If your IDE supports auto-completion, use it when referencing that method

Those are two options you could go for. I would prefer auto-completion (ex. Eclipse IDE) and not need to type out the method name. Going for the rename; just make sure you find out what calls that method and change direct references in each place. Refactoring will be your friend for that but be most careful when doing so.

Mushy
  • 345
0

I would generally recommend yes, rename it.

Other answers here have listed good reasons why you might not want to renamed it though, so if you find yourself in one such situation, you can create a new method with the proper name and implementation, and change the old method to call the new method. Then mark the old one as deprecated if your language supports it.

Brian
  • 121