How do you know when to NOT refactor?

What Sparked the Question

I recently leafed through The Power of TED*," a book-length parable about destructive cycles of human psychology in relationships.

At first I was annoyed by the abstractions. The "Dreaded Drama Triangle" made up of Victim, Persecutor, and Rescuer roles just seemed too neat and tidy to actually describe reality. It seemed the author was under the delusion that they'd pushed aside some curtain and glimpsed Reality Proper.

But then I realized the abstractions are just abstractions - useful heuristics to help people ask and answer the right questions. "Am I thinking of myself as a victim, and how might that impact my relationships? Do I recognize this kind of cycle in my life?"

(Aside: recognition of victim-hood can be useful. From what I can tell, the book thoughtfully avoids judgment of people who recognize "persecution" and instead focus on how to break harmful cycles of behavior. If people choose to wield the book's ideas judgmentally, that's obviously bad.)

The Question

Someone recently remarked to me, "A developer's first reaction to anyone else's code is 'This is rubbage, let's redo it all.'"

Though hyperbolic, the remark hit home. I refactor by default, and I don't have enough tools to recognize when I should leave less-than-perfect code alone.

So I wonder: How do you determine when to not refactor? What questions do you ask yourself or others? What kind of "gut feelings" do you encounter?

I'm also interested in hard-and-fast rules, but I suspect many readers will have more approximate tools at hand.

Comments

[Austin S. Hemmelgarn]

I usually run thorough this quick checklist:

1. The code is in production (that is, it's either past beta, or it's in active use on production infrastructure).
2. There are no existing bugs that would be solved by refactoring the code.
3. There are no new features that would be significantly simpler to implement following a refactor.
4. The code meets all required coding standards for the project.
5. There are no security-related anti-patterns present in the code.
6. You are not the primary maintainer of the code in question.
7. The code cannot be refactored without changing the API exposed to other components of the software.

General logic based on this:

• If points 1-5 are all true, don't refactor.
• If any of points 2, 3, or 5 are false for multiple reasons (for example, multiple bugs would be fixed or multiple features would be easier to implement), count them as false once for each reason they are false.
• If at least one of points 2-5 are false, and both points 6 and 7 are false, refactor.
• If point 4 is overwhelmingly false for the whole project, consider re-writing the coding standards before you think about refactoring.
• If point 1 is false, ignore point 7.
• If three or more of points 2-5 are false, consider ignoring point 7.
• If point 6 is true and you would refactor, propose the refactor to the primary maintainer of the code in question.

[Corey Alexander]

This is an amazing checklist! I'm gonna save this for later!

[Michael Crenshaw]

This is great, thank you!

[MarvelousWololo]

when you look into a piece of code and don't think about the word 'refactor'.

[Josh]

tl;dr – I guess the inverse of all of this is, "When you can read through a block of code, and understand what it does on your first read-through of it, then you don't need to refactor."

Refactors are some of the most important commits you will make to an application as it grows to serve more features to more users. They serve to make code more readable and DRY, less prone to errors, more fault tolerant, and they simplify the process of building out new features by reducing the number of other sections of the app new code will need to touch.

Refactors are synonymous with revisions for clarity in prose and essays. You want the ideas to be easier to understand, but fundamentally unaltered when you're done making changes; if your changes cause the block of code to either be invoked differently, or to return a different value than before, you aren't refactoring anymore. Your test suite should let you know if your refactors have broken anything (and you obviously have a test suite, because you're a good engineer, working with a team of competent people)

To that end, the main way to tell if some code could use refactoring is the simple question, "Can I read this alright?" If you have to pause and take a moment, or go back and reread the code, to grok the actual effect of it, there's probably a refactor or two waiting to be made.

My rubric for "not readable" breaks down into a few questions about the code structure:

1. Does the most nested line of code have 3 or more levels of indentation than the block's base indent?
2. Does the control flow contain an else if or a switch/case statement?
3. Are there any conditionals that directly check multiple Boolean expressions (i.e., do &&/|| appear more than once in any logic branch?)
4. Do the variable names accurately represent the variables' intents?

1 and 2 are typically signs that you can move the nested logic of each case into methods, within classes where the conditions you're checking are inherently true. Then you can rely on polymorphic dispatch to initiate the appropriate behavior from one central method call. Instances of number 3 are typically best to move into standalone methods that return the Boolean as their result; whereas number 4 is a matter of looking at the variables that are declared, considering the system they are a part of, and figuring out what to rename them to, accordingly.

The other thing I consider when asking if I should refactor some code, is whether or not doing so will enable future development to go more rapidly. More often than not, the answer will be "yes", but it's always possible to decide to refactor too early, and lock yourself in to a more complicated abstraction.

I hope that helps!

[Michael Crenshaw]

First off, wow, the depth here is fantastic. Thanks so much for taking the time.

I'm struggling to follow your recommended refactor for #1 and #2. Maybe I'm hung on the term "polymorphic dispatch" (no matter how many times I read descriptions of polymorphism, the meaning doesn't stick to the word for me).

Here's an example of what I think you mean by #2:

function SubmitContactRequest(ContactRequest request, RequestType type) {
    DBConnection dbConn = GetDBConnection("contactdb");

    if (type == RequestType.Phone) {
        dbConn.run("NewPhoneContactRequest", request);
    } else if (type == RequestType.Email) {
        dbConn.run("NewEmailContactRequest", request);
    }
}

If that is an accurate example, how would you refactor with polymorphic dispatch?

Again, thanks so much. I'm honestly daunted trying to learn when to avoid refactoring, and this is super helpful.

[Josh]

I'm glad I could help! Refactors used to be my least favorite aspect of writing code, but once a mentor demonstrated how important they are to having maintainable, updateable code, keeping my own codebases tidy became a matter of respect to him, and pride to myself 😅

To your question: Yes, that's exactly the sort of scenario I was thinking of! I haven't used TypeScript before, so someone else will have to let you know if this compiles, but I essentially mean that you would give two methods on different classes the same signature, like so:

class PhoneRequest extends ContactRequest {
  // PhoneRequest wouldn't necessarily have to extend 
  // ContactRequest in JavaScript, but idk how it works
  // in this Type-Scripted world 😬

  // ...
  handleSubmit(DBConnection conn) { 
    /* process submission in a phoney manner */ 
  }  
}

class EmailRequest extends ContactRequest {
  // ...
  handleSubmit(DBConnection conn) { 
    /*  process submission in an emailey way */
  }
}

That will allow you to remove the entire if-else if-else tree that pertains to handling the check against the passed-in type

function SubmitContactRequest(ContactRequest contactReq) {
  const DBConnection conn = GetDBConnection("contactdb");
  contactReq.handleSubmit(conn);
}

Now, no matter how many types of contact request you submit through this function, adding support for a new type is an O(1) developer problem, because you can guarantee that adding e.g. FaxRequest#handleSubmit will not ever interfere with the behavior of either PhoneRequest or EmailRequest's own #handleSubmit method.

Let me know if you have any other questions!

[Michael Crenshaw]

No worries about the syntax. I kinda invented a C#-TypeScript hybrid there. :-)

That's really helpful! I like your description of developer-complexity as O(1). I'll probably return to this example when looking at future refactors. Thanks again!