Matthew Butt

A strange behaviour in ReSharper’s Move Instance Method refactoring

Posted in programming by bnathyuw on 24 May 2016

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!

Watch out for exceptions in NUnit TestFixture constructors

Posted in programming by bnathyuw on 16 February 2015

Don’t do anything that might throw an exception in a TestFixture constructor: your tests will be ignored, rather than failing.

My colleague Chris and I have been playing around with polymorphous testing, taking advantage of the fact that you can pass parameters to an NUnit TestFixture like this:

[TestFixture(parameter1)]
[TestFixture(parameter2)]
public class BazTests{
  public BazTests(Blah parameter){
    ...
  }
}

We wanted to use these parameters to pass in a collaborator, which would then enable us to run the same tests against different implementations of the same interface. However, we immediately encountered difficulties, as the TestFixture attribute will only take parameters that are compile-time constants, which rules out directly passing in a collaborator class. If you are adding parameters to a Test, you can use the TestDataSource attribute to pass in more complex arguments; however, no such attribute is available for TestFixtures.

To get round this, we created a static factory that would then create our dependency according to the enum value passed in:

[TestFixture(TestSetting.Foo)]
[TestFixture(TestSetting.Bar)]
public class BazTests{
  private ITestHelper _testHelper;
  
  public BazTests(TestSetting testSetting){
    _testHelper = TestHelperFactory.Create(testSetting);
  }
}

However, some of our implementations of ITestHelper require a connection string, and when it is not present, a NullReferenceException is thrown. We would have expected some of our tests to fail because of this, but instead they were just ignored, and as TeamCity disregards ignored tests, this meant that our builds were treated as successful.

It suspect the tests are ignored because exceptions in the test constructor are not sent to TeamCity as errors; rather the exception must happen in one of the attribute-decorated methods.

The code gave us the correct failures once we had rewritten it like this:

[TestFixture(TestSetting.Foo)]
[TestFixture(TestSetting.Bar)]
public class BazTests{
  private TestSetting _testSetting;
  private ITestHelper _testHelper;

  public BazTests(TestSetting testSetting){
    _testSetting = testSetting;
  }

  [TestFixtureSetUp]
  public void TestFixtureSetUp(){
    _testHelper = TestHelperFactory.Create(_testSetting);
  }
}
Tagged with: , , , ,

The State Pattern, explored through the medium of cake!

Posted in programming by bnathyuw on 28 March 2012

At 7digital, we are running a weekly discussion group on design patterns, and I have been creating some small projects to exlore the issues raised in these discussions. The other week my colleague Emily presented the State Pattern, and it soon became clear that this is one of the more controversial patterns, as it seems to ride roughshod over the SOLID principles.

In this post I will give a bit of background, and then discuss the various approaches illustrated in my project.

The State Pattern

The classic case for the State Pattern is when an object’s behaviour changes according to the state it is in. A vending machine can only sell when it contains products, a shopping basket can only be checked out when it has products in it, a person can only get divorced if they are already married.

This pattern is implemented as an extension of the Strategy Pattern: the context object (vending machine, shopping basket, person) has a dependency on a state object, which in turn implements the behaviour (sell, check out, divorce) in an appropriate manner. Unlike cardinal examples of the Strategy Pattern, in which the strategy is set once and left alone, the State Pattern lets the strategy change during execution — often as the result of a method call on the state itself (if the vending machine sells the last product, it then becomes empty; if the only item is removed from the basket, it cannot be checked out; if the person gets divorced, they are no longer married).

Smelly

An immediate criticism of the State Pattern is that it leads to a system that is fairly tightly coupled. Not only is the context coupled to the state in order to make method calls, but a mechanism is then needed for updating the state, and a classical implementation of this is for the state to be coupled back to the context — a two-way coupling that is already a code smell.

Other implementations are possible: the context can update its own state after each state method call, or the state method can give the successor state as a return value; however, putting this question aside, it is the State Pattern’s lack of SOLIDity that makes it most problematic.

VAPID

Consider a context object that exposes several methods and has several states. Now consider that only some of these methods will be appropriate for any given state of the object (just as in the examples given above). Under the classic State Pattern, each concrete state must implement a method for every delegated method of the context object, even if it is not appropriate for it to do so (often implemented by throwing an exception).

First, this is a violation of the Single Responsibility Principle, as each concrete state now has responsibilities that it is designed to shirk. If we consider the ‘reason to change’ criterion, each concrete state can change because a) it changes the implementation of the stuff it does do, and b) it changes the implementation of failing to do the stuff it does not do.

More graphically, this is a violation of the Liskov Substitution Principle. This principle requires interchangeable objects (eg, those with the same interface) to behave in the same way, ie, given the same input conditions, they produce the same output conditions. The problem with the State Pattern is that it requires each concrete state to have different behaviour from its peers: violation of the LSP is seemingly baked into this pattern.

Finally, this pattern can lead to a violation of the Interface Segregation principle, insofar as a multiplicity of methods on the context class (which may be justified as being appropriate to that object) can then delegate to a a multiplicity of methods on the state interface, which, given their state-dependence, will no longer form a coherent collection.

Sharlotka

Let’s take a break from theory here, and look at my implementation.

I considered the stereotypical State Pattern examples, but let’s be honest: there are quite enough vending machines and shopping baskets in the world, and discussion of design patterns is not the context to be locking horns with Cardinal O’Brien.

So I thought I would use a culinary example instead.

Sharlotka (шарлотка) is a Russian apple cake. It is made by filling a cake tin with chopped apples, then pouring an eggy batter over the apples and baking in a warm oven till the batter has set. It is served dusted with sugar and cinnamon, and is really rather lovely.

It also makes a good, simple example of the State Pattern: each stage of cooking must be carried out in sequence, but there’s a nice loop in the middle, where you have to keep checking the cake until it’s cooked through before turning it out.

Classic Implementation

My first implementation follows the classic pattern.

You can see that there is one ISharlotkaState interface, which contains a method for each of calls that the Sharlotka context object delegates to its state. Each of these methods takes a reference to the Sharlotka as a parameter, so it can alter its state if need be. (The Sharlotka is passed with the interface IHasState<ISharlotkaState> to avoid making the State part of the public interface of the Sharlotka class itself.)

If you look at any of the concrete states (eg, ReadyToAddApplesState), you will see that most of the methods are left throwing a WrongStateException, as well as the mechanism for updating the context’s state. The _successor state is injected in the constructor, as this makes it possible to leave the mechanics of wiring everything together to the IoC container, rather than having to new up states within other states. In a small way this is reminiscent of the Chain of Resposibility Pattern, and does something to alleviate the tight coupling smell.

If you want to unit test one of the concrete states (eg, ReadyToAddApplesStateTests) then you are again left with a lot of boilerplate code.

This implementation highlights some of the deficiencies of the classic State Pattern implementation; however, it does still work, and may be appropriate for cases where it is not so much the behaviour of the context that is dependent on state, but rather its internal implementation.

Segregating the Interfaces

A more parsimonious approach has been suggested by Jan van Ryswyck, and I have implemented a version of this as the Small Interface project.

The key difference here is that rather than having a single, monolithic interface for all the states, the individual behaviours are broken into their own interfaces. When the context comes to call each state method, it first checks whether it can cast the state to the appropriate interface, and only then makes the call.

This implementation makes no further efforts to remedy the tight coupling, but does makes some improvements to the SOLID violations:

The Single Responsibility Principle is better supported, as each state is now only responsible for implementing the interfaces that are actually appropriate; we no longer have states also taking responsibility for throwing WrongStateExceptions, as this is done by the context object.

The Liskov Substitution Principle is better supported, as we no longer have sets of defective implementations of interface methods; instead we have used the Interface Segregation Principle to split out a set of atomic interfaces, each of which can more easily support the LSP. There are still opportunities for violation of the LSP, as it is entirely possible to implement the same interface in several states, and for each implementation to be different, but this problem is no longer inherent in the implementation.

This implementation makes good steps towards SOLIDity, but still has a major flaw, which is intrinsic to the state pattern: you can call methods on the stateful object that are simply not appropriate to its state; to continue the sharlotka example, you can call Serve before you have called Bake. Whilst doing this will throw an exception, it should arguably not be possible to do so in the first place.

A Fluent Implementation

A third possibility came up in our discussion — I think it was Greg who raised it —: you can implement your stateful class with a fluent interface.  You can see my version of this in the Fluent project.

Like the Small Interface implementation, this uses several atomic interfaces instead of one monolithic one; unlike that implementation however the interfaces are directly implemented by the Sharlotka class, rather than being delegated to a state object, and at any point in its lifecycle the Sharlotka is only considered as an implementation of the interface that is appropriate to its current state, ie, you never have access to methods that are not currently supported.

The trick in implementing this is in the fluent idiom: rather than calling

sharlotka.AddApples();
sharlotka.AddBatter();
sharlotka.Bake();

we want to chain the methods like this:

sharlotka.AddApples()
	.AddBatter()
	.Bake();

We can do this by making each method return this, but with a return type of the appropriate next interface.

For example, TurnOut is implemented like this:

ICanDustWithSugar ICanTurnOut.TurnOut() {
	return this;
}

which means that the next call must be a method of ICanDustWithSugar.

This implementation does away with the smelly tight coupling of the State-Pattern examples, as state is represented by the return type interface of each method, rather than as a property of the Sharlotka object. The application of the Single Responsibility Principle is rather less clear in this example, as the methods are stubbed out, rather than being implemented in any meaningful way. It is quite likely that in a real system the implementation of each method would be delegated to another object in order to honour this principle; this would look rather similar to the Small Interface implementation, with the crucial distinction that the implementation would not be responsible for updating the Sharlotka’s state.  The Liskov Substitution Principle ceases to be a question here, as we have moved to small interfaces with single implementations, and this fact also supports the Interface Segregation Principle.

Where this implementation is not so suitable is in cases where the state of the object after a method call is not clearly determined. For instance, withdrawing money from a bank account can leave the account in credit or overdrawn; in such a case the trick of returning a particular interface is not sufficient. A small example of this in my implementation is the Bake loop, and I have overcome the problem in this particular case by checking the return type until it is not null. However, this technique is already a significant departure from the fluent idiom, as it relies on runtime checking to make sure that the code actually works.

There is another danger with this implementation, in that it relies on the consumer knowing to use it fluently. There is no protection against storing the return value of any method in a variable, and calling the same method more than once (indeed, this is what is done during the Bake loop), and any person coding with this interface needs to know that methods must be chained. However, the fluent idiom is fairly commonplace now, and neither of these considerations is one of code quality.

& Messe it Forth

These three implementations illustrate different approaches to the same problem. The Classic implementation is unlikely to be the best in most circumstances because of its tendency to produce Liskov-violating defective methods; the Small Interface implementation overcomes these problems, and is probably most suitable for situations where the state of the object does no change in easily predictable ways; the Fluent implementation is handy when a particular sequence of methods should be called, but less idiomatic when the sequence can branch at runtime.

There are also tantalising prospects for implementing this type of system with monads, but I’m going to leave that for another day.

Tagged with: , ,

ActionInvoker method sequence

Posted in programming by bnathyuw on 26 October 2011

In one of the projects I’m playing with, I’m doing a bit of a hack over .NET MVC 3.

I’m providing my own implementation of IActionInvoker, currently by extending ControllerActionInvoker, and as part of this work, I’ve done a quick audit of the methods of this class, and the order they are called in.

I’m reproducing them here, in case it’s useful to anyone else:

  • InvokeAction
  • GetControllerAction
  • FindAction
  • GetFilters
  • InvokeAuthorizationFilters
  • GetParameterValues
  • InvokeActionMethodWithFilters
  • InvokeActionMethod
  • CreateActionResult
  • InvokeActionResultWithFilters
  • InvokeActionResult
Tagged with: , , ,

TPT mapping in Entity Framework CTP 5

Posted in programming by bnathyuw on 13 February 2011

I’ve been working on a little project with MVC 3 and Entity Framework Code-First CTP (community technology preview) 5, and have been implementing TPT (type-per-table) inheritance for the various types of user in the system: Customers, Administrators &c.

Yesterday, I added an Administrator class to my model, and this happened:

Yellow screen of death: “The configured property 'Forename' is not a declared property on the entity 'Administrator'. Verify that it has not been explicitly excluded from the model and that it is a valid primitive property.”

The exceptions message reads: The configured property ‘Forename’ is not a declared property on the entity ‘Administrator’. Verify that it has not been explicitly excluded from the model and that it is a valid primitive property.

All very puzzling, as here is my User class:

public class User
{
	public int Id { get; set; }
	[Display(Name = "Forename"), Required]
	public string Forename { get; set; }
	[Display(Name = "Surname"), Required]
	public string Surname { get; set; }
}

and here is my Administrator class:

public class Administrator:User
{
}

(OK, the Administrator class isn’t terribly useful right now, but I’m just fleshing out the schema at the moment.)

Here, finally, is my Repository class, somewhat abbreviated:

public class Repository : DbContext
{
	protected override void OnModelCreating(ModelBuilder modelBuilder)
	{
		modelBuilder.Entity().ToTable("Administrators");
		modelBuilder.Entity().ToTable("Customers");
	}
	public DbSet Administrators { get; set; }
	public DbSet Customers { get; set; }
	public DbSet Users { get; set; }
}

After some futile reading around, I finally decided to roll back to my previous working version, at which point the yellow screen of death disappeared. And it was at this point that the revelation came to me: one of the changes I had made was to alphabetise my DbSet properties, moving Users past Administrators and Customers.

So I went about adding the Administrator class again, but this time keeping the Users property above those for its subclasses:

public class Repository : DbContext
{
	protected override void OnModelCreating(ModelBuilder modelBuilder)
	{
		modelBuilder.Entity().ToTable("Administrators");
		modelBuilder.Entity().ToTable("Customers");
	}
	// Declare superclass first
	public DbSet Users { get; set; }
	public DbSet Administrators { get; set; }
	public DbSet Customers { get; set; }
}

and as if by magic the application started working again.

Moral

When implementing TPT inheritance in CTP 5, declare the superclass DbSet property before its subclasses.

Tagged with: , , , ,