Just because something works, so you don’t need to refactor?

Question

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

if ( $user_id = wp_validate_auth_cookie( '', apply_filters( 'auth_redirect_scheme', '' ) ) ) { } 

Um. Yes. two method calls and an assignment operator in an if statement.

So, I think no one refactors this.

Is there any reason?

Answer 1

To answer the question in the title:

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

If that were the case, we would never refactor. Because refactoring is done on green . That is, this happens when all the tests pass, when - by definition - the code works. If you "reorganize" on red, you are not refactoring. You play with your code. You risk that you probably should not accept. Refactoring improves the design of existing code without changing its behavior. If your code does not work, if you do not have tests demonstrating that it works, then your changes can change its behavior.

Answer 2

This certainly violates some encoding standards, but does WP violate the encoding standard?

I would not have a job in an IF block condition. People will always be surprised, it should be == instead of = ?.

Refactoring is good to recover part of your technical debt. Unit tests and good code coverage make refactoring easier.

A good rule of thumb is when you touch the code (or have to look at it to decrypt it) that it is time for it to be reorganized, so the next person should not deal with it. In this case, you have reviewed the code, you must reorganize it. Of course, these are recommendations that the group should agree on.

Ideally, you should control your technical debt.

Answer 3

Time is a luxury that not everyone has. I would like to go back and reorganize almost every line of code I have ever written. We always learn (I hope) and, probably, we can always come back and reorganize what we wrote, more concise, more elegant, more perfect, etc.

But the reality of this is that ... if it works, there is usually no priority to go back and “fix” it. Most likely, you are working on something new, and the client does not care if you have 2 method calls in the code block where it will work. They need their product.

If the code in question is so inactive that the system is unusable, they inform you (trust me), and at that moment you will definitely do refactoring.