My colleague Pedro and I were puzzling over some bizarre behaviour with ReSharper’s Move Instance Method refactoring move. This is a fairly complex move, and it makes various changes to the source and destination classes; however, in many cases, it generates code that does not compile, and seems to include wrongly qualified references.
Here is an example drawn from Martin Fowler’s Refactoring: Improving the Design of Existing Code:
Employee class before refactoring
using System;
namespace RefactoringExamples.ReplaceConditionalWithPolymorphism
{
public class Employee
{
private EmployeeType _employeeType;
private readonly int _monthlySalary;
private readonly int _commission;
private readonly int _bonus;
public Employee(int type)
{
Type = type;
_monthlySalary = 100;
_commission = 10;
_bonus = 20;
}
public int Type
{
get { return _employeeType.TypeCode; }
set { _employeeType = EmployeeType.TypeFrom(value); }
}
public int PayAmount()
{
return Pay();
}
private int Pay()
{
switch (_employeeType.TypeCode)
{
case EmployeeType.Engineer:
return _monthlySalary;
case EmployeeType.Salesperson:
return _monthlySalary + _commission;
case EmployeeType.Manager:
return _monthlySalary + _bonus;
default:
throw new Exception("Incorrect Employee");
}
}
}
}
EmployeeType class before refactoring:
using System;
namespace RefactoringExamples.ReplaceConditionalWithPolymorphism
{
public abstract class EmployeeType
{
public abstract int TypeCode { get; }
public static EmployeeType TypeFrom(int value)
{
switch (value)
{
case Engineer:
return new Engineer();
case Salesperson:
return new Salesperson();
case Manager:
return new Manager();
default:
throw new Exception("Incorrect Employee Code");
}
}
public const int Engineer = 0;
public const int Salesperson = 1;
public const int Manager = 2;
}
class Engineer : EmployeeType
{
public override int TypeCode => Engineer;
}
class Salesperson : EmployeeType
{
public override int TypeCode => Salesperson;
}
class Manager : EmployeeType
{
public override int TypeCode => Manager;
}
}
Employee class after refactoring
using System;
namespace RefactoringExamples.ReplaceConditionalWithPolymorphism
{
public class Employee
{
private EmployeeType _employeeType;
private readonly int _monthlySalary;
private readonly int _commission;
private readonly int _bonus;
public Employee(int type)
{
Type = type;
_monthlySalary = 100;
_commission = 10;
_bonus = 20;
}
public int Type
{
get { return _employeeType.TypeCode; }
set { _employeeType = EmployeeType.TypeFrom(value); }
}
public int MonthlySalary
{
get { return _monthlySalary; }
}
public int Commission
{
get { return _commission; }
}
public int Bonus
{
get { return _bonus; }
}
public int PayAmount()
{
return _employeeType.Pay(this);
}
}
}
EmployeeType class after refactoring
using System;
namespace RefactoringExamples.ReplaceConditionalWithPolymorphism
{
public abstract class EmployeeType
{
public abstract int TypeCode { get; }
public static EmployeeType TypeFrom(int value)
{
switch (value)
{
case Engineer:
return new Engineer();
case Salesperson:
return new Salesperson();
case Manager:
return new Manager();
default:
throw new Exception("Incorrect Employee Code");
}
}
public const int Engineer = 0;
public const int Salesperson = 1;
public const int Manager = 2;
public int Pay(Employee employee)
{
switch (System.TypeCode)
{
case EmployeeType.Engineer:
return employee.MonthlySalary;
case EmployeeType.Salesperson:
return employee.MonthlySalary + employee.Commission;
case EmployeeType.Manager:
return employee.MonthlySalary + employee.Bonus;
default:
throw new Exception("Incorrect Employee");
}
}
}
class Engineer : EmployeeType
{
public override int TypeCode => Engineer;
}
class Salesperson : EmployeeType
{
public override int TypeCode => Salesperson;
}
class Manager : EmployeeType
{
public override int TypeCode => Manager;
}
}
Notice the reference to System.TypeCode in the EmployeeType.Pay method on line 30 above; this doesn’t compile, and is most definitely not what we wanted. My hypothesis is that once ReSharper has rewritten the code, it attempts to resolve and properly qualify any symbol names; in this case, it spots TypeCode and, because this is the name of a type known to the compiler, it decides that the name needs to be appropriately qualified, adding the System namespace. It’s failing to recognise that TypeCode is actually the name of a property on the EmployeeType class, and that no qualification is necessary.
I have found three ways to work round this:
First, you can give the property a name that not already used by a type known to the compiler. If I rename TypeCode to Code, then the refactoring move works perfectly. In this case, this is probably a nice idea, as the name EmployeeType.TypeCode contains redundancy. However, in many cases the most appropriate name for a property may coincide with a type name, and renaming it will not be a good option.
The second option is to recognise that this happens, and fix it. In this case, all I need to do is remove the System namespace on line 30, and everything works correctly. I’m not a huge fan of this solution, as I like my automatic refactorings to be safe and reliable, but it may be a pragmatic choice.
The third option is to perform this refactoring by composing smaller, safe steps. This can give you finer grained control over the outcome, but at the expense of complexity.
I would be interested to hear if anyone has any further insight into what’s going on here, or if there are other techniques for overcoming it. Do let me know if you do!
Update (25 May 2016): I sent JetBrains a link to this post, and within an hour they raised a bug report. Thanks to Daria at JetBrains for the speedy response!
Matthew Butt 