Smart Constructor Anti-Pattern
If you look at the definition of a constructor in the context of software development on Wikipedia (a completely factual and reliable source :), you’ll find this simple statement of their responsibility…
“Their responsibility is to initialize the object’s data members and to establish the invariant of the class, failing if the invariant isn’t valid.”
Simple really. Just initialize the (usually) private data members and ensure the arguments won’t put the object in an invalid state. But so often I see constructors doing so much more than that. One example I’ve seen of this recently is in presenters.
The (perhaps contrived) scenario:
When a view loads for the first time, you need to load some lookup data that gets populated in a combo box on the view and set the number of logged in users on the view.
Here is our IView interface for reference:
1: public interface IView
2: {
3: void BindWith(IEnumerable<User> users);
4: bool HasUserSelected { get; }
5: void SetNumberOfLoggedInUsersTo(int numberOfUsers);
6: }
</div> </div>
“Smart” Constructor Approach
One might be tempted to start writing a spec like the one below.
1: public class SmartConstructorPresenterSpecs
2: {
3: public class when_the_presenter_is_constructed : ContextSpecification
4: {
5: private IView View;
6: private IUserRepository UserRepository;
7: private IAuthenticationService AuthenticationService;
8: private SmartConstructorPresenter SUT;
9: private IEnumerable<User> Users;
10:
11: protected override void When()
12: {
13: Users = new List<User> {new User(true), new User(true), new User(false)};
14:
15: View = Mock<IView>();
16: UserRepository = Mock<IUserRepository>();
17: AuthenticationService = Mock<IAuthenticationService>();
18:
19: UserRepository.Stub(x => x.All()).Return(Users);
20:
21: SUT = new SmartConstructorPresenter(View, UserRepository, AuthenticationService);
22: }
23:
24: [Test]
25: public void it_should_populate_the_view_with_a_list_of_users()
26: {
27: View.AssertWasCalled(x => x.BindWith(Users));
28: }
29:
30: [Test]
31: public void it_should_set_the_number_of_logged_in_users_on_the_view()
32: {
33: View.AssertWasCalled(x => x.SetNumberOfLoggedInUsersTo(2));
34: }
35: }
36:
37: public class when_attempting_to_login_and_a_user_is_not_selected : ContextSpecification
38: {
39: private IView View;
40: private IUserRepository UserRepository;
41: private IAuthenticationService AuthenticationService;
42: private SmartConstructorPresenter SUT;
43: private IEnumerable<User> Users;
44:
45: protected override void EstablishContext()
46: {
47: Users = new List<User> {new User(true), new User(true), new User(false)};
48:
49: View = Mock<IView>();
50: UserRepository = Mock<IUserRepository>();
51: AuthenticationService = Mock<IAuthenticationService>();
52:
53: UserRepository.Stub(x => x.All()).Return(Users);
54: View.Stub(x => x.HasUserSelected).Return(false);
55:
56: SUT = new SmartConstructorPresenter(View, UserRepository, AuthenticationService);
57: }
58:
59: protected override void When()
60: {
61: SUT.Login();
62: }
63:
64: [Test]
65: public void it_should_NOT_perform_a_login()
66: {
67: AuthenticationService.AssertWasNotCalled(x => x.Login());
68: }
69: }
70: }
</div> </div>
And implement the presenter.
1: public class SmartConstructorPresenter
2: {
3: private readonly IView view;
4: private readonly IAuthenticationService authenticationService;
5:
6: public SmartConstructorPresenter(IView view, IUserRepository userRepository,
7: IAuthenticationService authenticationService)
8: {
9: this.view = view;
10: this.authenticationService = authenticationService;
11:
12: var users = userRepository.All();
13:
14: this.view.BindWith(users);
15: this.view.SetNumberOfLoggedInUsersTo(users.Where(x => x.IsLoggedIn).Count());
16: }
17:
18: public void Login()
19: {
20: if (view.HasUserSelected)
21: authenticationService.Login();
22: }
23: }
</div> </div>
And then the view.
1: public class View : IView
2: {
3: private event VoidHandler loginButtonClick = delegate { };
4: private readonly SmartConstructorPresenter presenter;
5:
6: public View()
7: {
8: presenter = new SmartConstructorPresenter(this, null, null);
9:
10: // simulate a possible login button click
11: loginButtonClick += () => presenter.Login();
12: }
13:
14: public void BindWith(IEnumerable<User> users)
15: {
16: // bind this stuff to something
17: }
18:
19: public bool HasUserSelected
20: {
21: get { return false; }
22: }
23:
24: public void SetNumberOfLoggedInUsersTo(int numberOfUsers)
25: {
26: // set some label or something
27: }
28: }
29:
30: public delegate void VoidHandler();
</div> </div>
If you take a look at the code above, you can see that a the view is being populated with a list of users when the presenter is constructed. While this might seem harmless at first, take a look at the second specification : when_attempting_to_login_and_a_user_is_not_selected.it_should_NOT_perform_a_login.
Notice that the only thing we care about is verifying that when a user is not selected, that we’re NOT attempting to call login on the authentication service. But since our constructor has some “smarts”, we’re required to stub out the call to the user repository just to keep the test from breaking. This may not look like a big deal in a simple case like this. But as your codebase grows, it can make your specs brittle and harder to understand.
“Dumb” Constructor Approach
Another way this could be done is by delegating the initial operations on the view to an explicit “initialize” method. Let’s see what that might look like.
First the specs of course. 🙂
1: public class DumbConstructorPresenterSpecs
2: {
3: public class when_initializing : ContextSpecification
4: {
5: private IView View;
6: private IUserRepository UserRepository;
7: private IAuthenticationService AuthenticationService;
8: private DumbConstructorPresenter SUT;
9: private IEnumerable<User> Users;
10:
11: protected override void EstablishContext()
12: {
13: View = Mock<IView>();
14: UserRepository = Mock<IUserRepository>();
15: AuthenticationService = Mock<IAuthenticationService>();
16:
17: SUT = new DumbConstructorPresenter(View, UserRepository, AuthenticationService);
18: }
19:
20: protected override void When()
21: {
22: Users = new List<User> {new User(true), new User(true), new User(false)};
23:
24: UserRepository.Stub(x => x.All()).Return(Users);
25:
26: SUT.Initialize();
27: }
28:
29: [Test]
30: public void it_should_populate_the_view_with_a_list_of_users()
31: {
32: View.AssertWasCalled(x => x.BindWith(Users));
33: }
34:
35: [Test]
36: public void it_should_set_the_number_of_logged_in_users_on_the_view()
37: {
38: View.AssertWasCalled(x => x.SetNumberOfLoggedInUsersTo(2));
39: }
40: }
41:
42: public class when_attempting_to_login_and_a_user_is_not_selected : ContextSpecification
43: {
44: private IView View;
45: private IUserRepository UserRepository;
46: private IAuthenticationService AuthenticationService;
47: private DumbConstructorPresenter SUT;
48:
49: protected override void EstablishContext()
50: {
51: View = Mock<IView>();
52: UserRepository = Mock<IUserRepository>();
53: AuthenticationService = Mock<IAuthenticationService>();
54:
55: SUT = new DumbConstructorPresenter(View, UserRepository, AuthenticationService);
56: }
57:
58: protected override void When()
59: {
60: View.Stub(x => x.HasUserSelected).Return(false);
61:
62: SUT.Login();
63: }
64:
65: [Test]
66: public void it_should_NOT_perform_a_login()
67: {
68: AuthenticationService.AssertWasNotCalled(x => x.Login());
69: }
70: }
71: }
</div> </div>
Then the implementation of the presenter.
1: public class DumbConstructorPresenter
2: {
3: private readonly IView view;
4: private readonly IUserRepository userRepository;
5: private readonly IAuthenticationService authenticationService;
6:
7: public DumbConstructorPresenter(IView view, IUserRepository userRepository,
8: IAuthenticationService authenticationService)
9: {
10: this.view = view;
11: this.userRepository = userRepository;
12: this.authenticationService = authenticationService;
13: }
14:
15: public void Initialize()
16: {
17: var users = userRepository.All();
18:
19: view.BindWith(users);
20: view.SetNumberOfLoggedInUsersTo(users.Where(x => x.IsLoggedIn).Count());
21: }
22:
23: public void Login()
24: {
25: if (view.HasUserSelected)
26: authenticationService.Login();
27: }
28: }
</div> </div>
And a slight change to the view implementation.
1: public class View : IView
2: {
3: private event VoidHandler loginButtonClick = delegate { };
4: private event VoidHandler load = delegate { };
5: private readonly DumbConstructorPresenter presenter;
6:
7: public View()
8: {
9: presenter = new DumbConstructorPresenter(this, null, null);
10:
11: // simulate a couple possible events
12: load += () => presenter.Initialize();
13: loginButtonClick += () => presenter.Login();
14: }
15:
16: public void BindWith(IEnumerable<User> users)
17: {
18: // bind this stuff to something
19: }
20:
21: public bool HasUserSelected
22: {
23: get { return false; }
24: }
25:
26: public void SetNumberOfLoggedInUsersTo(int numberOfUsers)
27: {
28: // set some label or something
29: }
30: }
31:
32: public delegate void VoidHandler();
</div> </div>
I would prefer this approach since I think it keeps the specs and the presenter implementation itself quite a bit cleaner. Notice now in the spec: when_attempting_to_login_and_a_user_is_not_selected.it_should_NOT_perform_a_login.
It cares nothing about retrieving users from the repository. All it cares about is exactly what the context/spec sentence states. And if you notice, there is even an opportunity to DRY up these specs, if you prefer, by breaking out a base context class, since they both have identical EstablishContext() methods. This is something that wouldn’t be quite as easy or clean to do in the SmartConstructorPresenterSpecs in the previous section.
Quick Tip : Load/Initialize Convention
You might be thinking that you don’t want to have to remember to create or call an Initialize() method all the time. Well recently, a co-worker of mine pointed out a simple way we could leverage some auto-wiring magic + a base presenter class to make it more of a convention that you can use when needed and to negate the need to manually call Initialize() all the time. So here is one approach to make that easier.
NOTE: The presenter specs don’t need to change at all for this refactoring.
First we add a Load event handler to our IView interface:
1: public interface IView
2: {
3: void BindWith(IEnumerable<User> users);
4: bool HasUserSelected { get; }
5: void SetNumberOfLoggedInUsersTo(int numberOfUsers);
6: event EventHandler Load;
7: }
</div> </div>
Then we introduce a base presenter class (which I’m simply calling Presenter ‘cause I think Base* class naming is the new hungarian notation…. :).
1: public class Presenter<T> where T : IView
2: {
3: public Presenter(T view)
4: {
5: view.Load += delegate { Initialize(); };
6: }
7:
8: public virtual void Initialize()
9: {
10: }
11: }
</div> </div>
Then our DumbConstructorPresenter just changes slightly to inherit and override the Initialize() method.
1: public class DumbConstructorPresenter : Presenter<IView>
2: {
3: private readonly IView view;
4: private readonly IUserRepository userRepository;
5: private readonly IAuthenticationService authenticationService;
6:
7: public DumbConstructorPresenter(IView view, IUserRepository userRepository,
8: IAuthenticationService authenticationService) : base(view)
9: {
10: this.view = view;
11: this.userRepository = userRepository;
12: this.authenticationService = authenticationService;
13: }
14:
15: public override void Initialize()
16: {
17: var users = userRepository.All();
18:
19: view.BindWith(users);
20: view.SetNumberOfLoggedInUsersTo(users.Where(x => x.IsLoggedIn).Count());
21: }
22:
23: public void Login()
24: {
25: if (view.HasUserSelected)
26: authenticationService.Login();
27: }
28: }
</div> </div>
And finally our view implementation.
1: public class View : IView
2: {
3: // in the context of a winforms app,
4: // this event would be automatically declared and fired
5: public event EventHandler Load = delegate { };
6:
7: public event VoidHandler LoginButtonClick = delegate { };
8: private readonly DumbConstructorPresenter presenter;
9:
10: public View()
11: {
12: presenter = new DumbConstructorPresenter(this, null, null);
13:
14: // simulate a possible login button click
15: LoginButtonClick += () => presenter.Login();
16: }
17:
18: public void BindWith(IEnumerable<User> users)
19: {
20: // bind this stuff to something
21: }
22:
23: public bool HasUserSelected
24: {
25: get { return false; }
26: }
27:
28: public void SetNumberOfLoggedInUsersTo(int numberOfUsers)
29: {
30: // set some label or something
31: }
32: }
</div> </div>
Notice in the Presenter
Then notice in the updated view implementation, we don’t have explicitly call the Initialize() method anymore in our views, since that’s done for us automatically in the Presenter
As I’m writing this I realize that the act of overriding this Initialize() method would have to be remembered as well. But I think what you gain from a convention standpoint and the fact that you don’t have to explicitly call it from the views is enough of a win to warrant this technique. But as always, I’ll probably change my mind tomorrow when I figure out a better way to do all of this. 🙂
So, putting aside the possibly poor examples and the cloud of MVP in all of this, my goal was just to simply give an example of why it can be a good idea to keep your constructors simple and lean across the board, in many different types of classes.
Of course feedback is always welcome.