7

Before I ask my question, I want to note that this is not my own code; it comes from the Refactoring Guru website:


enter image description here


I often find myself using the code on the left, and I have one primary motivation for doing so: in the code on the right, you are unnecessarily pushing and popping a stack frame for the same operation twice in a row. For example, suppose that the base price is in fact greater than 1000. In testing this conditional, you already computed what the base price is... So by the time you get to the body of that conditional, you already know the value, and there's really no point in re-computing it with a second call to basePrice(). In either case—when the base price exceeds 1000 or is less than or equal to 1000—you end up with an extra redundant call to basePrice().

Question: Does the code on the right offer any real advantages over the one on the left? The site mentions that this allows for greater reusability of the code in case other methods want to compute the base price as well, which I agree with.

  • 4
    I wouldn't worry about the cost of the stack push/pop unless you have profiling that makes it very apparent that is your bottleneck. Depending on the language and compiler, that call might even get inlined. I'd worry more about what the function is actually doing and how costly that is. – cbojar May 26 at 15:03
  • 7
    The refactoring the Code Guru site is describing is micro-optimization. In any case, if you really want to eliminate the second function call, just call basePrice() once, and store it in a variable, just as you're doing in the first code example. Problum solved. – Robert Harvey May 26 at 15:05
  • @RobertHarvey Gotcha, that makes sense. Thanks! – AleksandrH May 26 at 15:16
  • 2
    The example is surely designed to highlight a refactoring principle that you may wish to use in certain circumstances. It cannot be an argument that every intermediate variable should be hived off into a separate method - and from what is apparent in the particular example, I would actually say the refactoring here is a bad practice. – Steve May 26 at 17:28
  • 1
    The code on the left and the right are not equivalent. On the right you are reading quantity and itemPrice twice and in the left one just once. In this trivial example it probably won't matter, but you can get blindsided. Just reading something can have side-effects, which you now have twice. Also, between the comparison and assignment, the quantity and itemPrice may change and you end up with different results. – Pieter B May 29 at 8:26
19

A refactoring is not a best practice.

A refactoring is a way to change code without changing externally visible behavior. A single well written unit test should be able to pass both the left code and the right code without being touched.

You're reading this post as if the code on the left is always a problem. It's not. Sometimes the code on the right is the problem. This is telling you something you can do if you see the code on the left as a problem. That does not guarantee that the code on the right won't also have problems.

in the code on the right, you are unnecessarily pushing and popping a stack frame for the same operation twice in a row.

You don't actually know that.

It looks like it should call it twice but depending on how it's written and the compiler/interpreter used, it might be optimized away.

Even if it's not, unless the calculation is truly significant, the performance improvement that caching would give you is not worth worrying about.

Some may argue that the code on the left is clearer. Some may argue that the code on the right is doing fewer things and so is better focused on doing one thing.

I'll argue that both of them could be improved by externalizing the dependency on base price and making it an explicit parameter to pass in. Which is yet another refactoring.

Doing that is objectively a refactoring. The idea that it's better is subjective opinion. When making subjective choices ask your team. They're the ones that have to live with it.

6

To me, using the local variable to capture a query result (query of the object state) says something significant: that I'm only doing one query of these field values (to compute one value), and, my logic expects to work with that one value.  This I find to be clear and simple.

Whereas repeating the field accesses — whether directly or by method call — says to me these values might change, and we want to make sure we always use the latest.

Unfortunately in this case, if the fields change between the if-condition and either the then-part or else-part, this code won't really handle it.  For this reason, I find such code confusing — why did the author feel the need to repeat the query, if the logic here cannot handle changes in the underlying fields?

All in all, as @Robert suggests you can still use a local variable with the query computation refactored into a method.  (And I would b/c of the above.)

Doing this refactoring then begs the question as to why that one multiplication is made into its own method yet chosen percentage multiplications based on some magnitude are not.  It is an improvement in some sense, that now that method computes discounted total from the base total (computed elsewhere), i.e. it is a rule encoded in code.

A next logical upgrade, due to the fact that business logic is subject to change by various campaigns and promotions, is that such a rule really should be externalized (runtime replaceable) in some way instead of hard coded with if's and constants in code.

  • "Unfortunately in this case, if the fields change between the if-condition and either the then-part or else-part, this code won't really handle it. For this reason, I find such code confusing — why did the author feel the need to repeat the query if the logic here cannot handle changes in the underlying fields?" That's a great point! I didn't even think of that. – AleksandrH May 29 at 20:32
3

I concur with candied_orange's answer but I would caution that the specific refactoring mentioned here could cause subtle hard-to-detect bugs and/or instabilities.

The given refactoring can only be known to be equivalent if and only if it's guaranteed that the values of quantity and itemPrice will not change during the execution of the method. If there's any chance that they might, using the local variable is not just a 'best practice' it's required for correctness.

Note that creating the basePrice() method and removing the local variable are two separate changes. Introducing that method doesn't indicate removal of the local variable or vice/versa.

Why might the value change? Assume that it's already been implemented as a method. Do you know whether repeated calls to the method produce the same result? Perhaps the basePrice() method contains a lookup call on price. You could end up with the basePrice being over 1000 for the if statement but less than 1000 when calculating the discount (or the other way around.) Oops! Will unit testing catch this problem? Unless you set up a specific test for this scenario: probably not.

Or consider the case of a multi-threaded environment. If these values are references to values that could be changed by other threads, you can end up with a similar problem. Again, hard to find with unit testing (or any other testing.)

There are ways to address this e.g. make these values final/unmodifiable (what I would highly recommend) The point here is not making the calls more than once is always wrong but that you need to realize that such a change is not inherently equivalent. You need to consider such changes in the context of the entire program.

2

The main reason I'd use the right-side code is that it allows for unit-testing basePrice().

Is that something worth unit-testing? Maybe; depends a lot on the specific details.

  • Oh, that's another good point! – AleksandrH May 28 at 17:42
  • 1
    Good reason to introduce a method but as I note in my answer, you can do this without eliminating the local variable. – JimmyJames May 29 at 15:47
1

This is about refactoring, which means modifying code without changing its meaning. You can refactor from the left to the right, or from the right to the left, whichever you prefer.

Actually we have two refactorings here: replacing an expression with a function, and replacing a variable storing the expression with multiple evaluations (of the expression or the function). Both are sometimes good, sometimes bad. The more complex and non-obvious the expression is, and the more places it is used, the better it becomes to use a function. And the more time-intensive it is to evaluate, the better it is to store the result in a variable. It also answers the question whether function calls are idempotent or may return different values - if your function result is stored in a local variable, it’s not going to change. But if you expect it to change like time() or random() then you may want multiple calls. Or not.

  • I like this take but when you say "if your function result is stored in a local variable, it’s not going to change." it makes me cringe. Local scope doesn't make variables constant. I know what you mean but please find a better way to say it. We live in a world of setters after all. – candied_orange May 30 at 14:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.