Just because something works, does it mean you don't have to refactor?

I just saw this line of code in the WP codebase.

if ( $user_id = wp_validate_auth_cookie( '', apply_filter开发者_StackOverflow中文版s( 'auth_redirect_scheme', '' ) ) ) {
}

Um. Yeah. two method calls and an assignment statement in an if statement.

So, my guess is, nobody is refactoring this.

Are there any reasons why?

---

To answer the question in your title:

Just because something works, does it mean you don't have to refactor?

If it did, we'd never refactor. Because refactoring happens on green. That is, it happens when all tests are passing, when - by definition - the code works. If you "refactor" on red, you're not refactoring. You're playing with your code. You're taking risks that you probably shouldn't be taking. Refactoring is improving the design of existing code, without changing its behavior. If your code isn't working, if you don't have tests that demonstrate that it's working, then the changes you make could be changing its behavior.

---

This is certainly breaking some coding standards, but is it breaking the WP coding standard?

I wouldn't have an assignment in the condition of an IF block. People will always wonder, is that supposed to be == instead of =?.

Refactoring is good to earn back some of your technical debt. Unit tests and good code coverage makes refactoring easier.

A good rule of thumb is any time you touch the code (or have to stare at it to decipher it), that's the time to refactor it, so the next person doesn't have to deal with it. In this case, you looked at the code, you should refactor it. Of course, these are guidelines that the group has to agree on.

Ideally, you should keep your technical debt under control.

---

Time is a luxury that not everybody has. I'd -love- to go back and refactor just about every line of code I've ever written. We're always learning (hopefully), and could probably always go back and refactor things we've written to be more concise, more elegant, more performant, etc.

But the reality of it is... if it works, there's not usually a high priority to go back and "fix" it. Chances are you're working on something new, and the client doesn't really care if you've got 2 method calls in a block of code where one will do. They want their product.

If the code in question is so non-performant that the system is unusable, they'll let you know (trust me), at which point you'll definitely be doing the refactoring.