Discussion on: How do you know when to NOT 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!