Refactoring Code – Replacing Temporary Variables with Queries

clean codedesignrefactoring

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

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.

Best Answer

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.

Related Solutions

Refactoring – Replace Type Code with Class (From Fowler)

I think you've almost answered your own question there.

The second piece of code is preferred over the first because it offers type-safety. With the first piece, if you have a similar enumeration of Fish then you can say something like

MostNotableGrievance grievance = Fish.Haddock;

and the compiler won't care. If Fish.Haddock = 2 then the above will be exactly equivalent to

MostNotableGrievance grievance = MostNotableGrievance.FlipFlopping;

but obviously not instantly readable as such.

The reason enumerations are often no better is because the implicit conversion to and from int leave you with the exact same problem.

But, in C#, there is no such implicit conversion, so an enum is a better structure to use. You'll rarely see either of the first two approaches used.

Clean Code – Is This Way of Calling a Function a Bad Practice?

You are using your languages method overloading feature to offer the caller alternate ways of resolving the methods dependency on positional information. You are then delegating to another method to resolve the remaining work of updating the camera.

The code smell here would be if you keep just extending the chain of methods calling methods. The Location taking method calls the double taking method which calls the latLng taking method which finally calls something that knows how to update the camera.

Long chains are only as strong as their weakest link. Each extension of the chain increases the footprint of code that has to work or this thing breaks.

It is a far better design if each method takes the shortest possible path to solving the problem that has been presented to it. That doesn't mean each must know how to update the camera. Each should translate their position parameter type to one uniform type that can be passed to something that does know how to update the camera when presented this one type.

Do it that way and you can remove one without breaking half of everything else.

Consider:

public void moveCameraTo(Location location){
    moveCameraTo( new LatLng(location) );
}

This makes dealing with latitude and longitude LatLngs problem. The cost is it spreads knowledge of LatLng around. That might seem expensive but in my experience it's a preferable alternative to primitive obsession which is what avoiding establishing a parameter object leaves you stuck with.

If Location is refactorable but LatLng is not, consider solving this by adding a factory to Location:

moveCameraTo( location.ToLatLng() );

This also nicely avoids primitive obsession.

Best Answer

Related Solutions

Refactoring – Replace Type Code with Class (From Fowler)

Clean Code – Is This Way of Calling a Function a Bad Practice?

Related Topic