Using the Extract Method pattern to refactor a simple method with a for loop

Question

I am reading specific parts of Martin Fowlers refactoring book again (the areas I was not clear about the first time round). I am looking at the Extract Method chapter at the moment. I can understand why Extract Method is beneficial; for example:

1) Inheritance and overriding

2) Clarity for the user of the class

Say I have some code like the below ( This is a DDD domain Service). Is this a candidate for Extract Method?:

 public IEnumerable<KeyValuePair<int, int>> CalculateDenominationsFor(int cost) 
 {
 var target = cost;
 foreach (var denomination in currency.AvailableDenominations.OrderByDescending(a => a))
 {
 var numberRequired = target / denomination;
 if (numberRequired > 0)
 {
 yield return new KeyValuePair<int, int>(denomination, numberRequired);
 }
 target = target - (numberRequired * denomination); 
 }
 } 

I guess I could extract the following lines of code to methods:

target = target - (numberRequired * denomination); 

and:

yield return new KeyValuePair<int, int>(denomination, numberRequired);

The concerns I have about my two ideas above are:

1) They would be private methods so no benefit to the caller.

2) The class is currently sealed so no Inheritance benefits.

Is there any guidance available stating when to use Extract Method? Am I overthinking this? I am trying to apply this principle of least astonishment and find myself overthinking a lot recently.

• Doc Brown: The biggest mistake I can see in this method is the overuse (IMHO wrong use) of var. I had to read it three times to see all that variables are integers, and the code makes an integer division. Do yourself a favor and write int instead of var, that is not even more typing.
• amon: Btw, this might be clearer if you write target = target % denomination, i.e. the remainder after giving that change. It then becomes clear that every line is integral to the solution, and nothing is worth extracting. (Except maybe fetching and ordering the denominations? Dunno. This method should arguably be called GiveChange(), and be a member of a currency class.)
• w0051977: @amon, I see what you mean about putting the method in the Currency class. However, I am planning to encapsulate Cost in a method (value object), so I think it makes sense to use a Domain Service, which operates on: Cost (value object - which may have methods for calculating discounts etc) and Currency. Do you agree?
• Ewan: the biggest mistake is the potential infinite loop
• w0051977: @Ewan, would you elaborate?

Answer 1

Yes I believe you are overthinking it! You should not look for "candidates for refactorings". You should look for code which require improvement and then look for refactorings as the possible tools to improve the code.

So the question is if the code in question:

1. Has a problem
2. This problem can be alleviated by applying a refactoring

I don't really see the two expressions in question as problematic, and I don't see how they are improved by the refactorings you suggest.

(The method as a whole could probably be clearer though, I cant really figure out what is going on. But that might just be lack of context.)

• w0051977: Thanks. +1 for the two points. Could you explain what you mean by: "The method as a whole could probably be clearer though". Does it need comments or better variable names etc?
• Doc Brown: @w0051977: as I already wrote, don't use var when it is not obvious from the specific line which type the declared variable gets. Especially when it is important for how the code works.
• David Arno: Absolutely do ••not•• change those var’s. I’d change the variable names though. target confused me until I worked out it was the remaining cost. So call it that. And I’d be tempted to change numberRequired to eg denominationQuantity, but that’s less of an issue. One things for sure though: breaking this function up into sub-functions would almost certainly make it more complex. So leave it as it is.
• JacquesB: It is crucial to understand the division is an integer division. This is why I didn't understand the code at first, since it is not obvious. So I agree with the doc that you should be explicit with the type or or otherwise indicate this. Also agree with Arno about the naming.
• Doc Brown: @DavidArno: so you prefer to leave a var instead of int, even when it obfuscates the code? Do you prefer code which is hard to read, or why?