We've had interesting discussion recently, on whether to check constructor arguments on nulls. I want to share my thought on this.
Our coding convention has a rule stating that all public methods arguments should be checked for nulls. So our methods usually looks like this one:
public void SomeMethod(string arg)
{
Precondition.Argument.NotNull(arg, "arg");
// Real code comes here...
}
And I'm quite happy with such
DbC like coding style. But... with one exception.
I don't like to write code checking for null dependencies in a service constructor. For two main reasons:
- It is easier to write tests for the service. In case when particular dependency is not used in test it is easier to pass null instead of creating stub dependencies only to satisfy checks.
- It useless in case when all services are instantiated through DI container. That is because, as far as I know, most DI containers will throw an exception by self when it can't find dependency for the service.
I think code will show my point better. Suppose we need a service to login users. Service responsibility will be to retrieve user by the name provided, verify that provided password is correct and to set the user as a current user in the application. In case when supplied user name or password are incorrect, service should log appropriate message.
So service will need some repository to retrieve users, some service to authenticate user and a logger to log messages in case when incorrect user credentials specified.
I'll disregard TDD practice and will show the code before test (shame on me). So the LoginService code:
public class LoginService : ILoginService
{
readonly IUserRepository userRepository;
readonly IAuthenticationService authenticationService;
readonly ILog log;
public LoginService(
IUserRepository userRepository,
IAuthenticationService authenticationService,
ILog log)
{
// This is the subject of discussing.
Precondition.Argument.NotNull(userRepository, "userRepository");
Precondition.Argument.NotNull(authenticationService, "authenticationService");
Precondition.Argument.NotNull(log, "log");
this.userRepository = userRepository;
this.authenticationService = authenticationService;
this.log = log;
}
public bool Login(UserCredentials userCredentials)
{
Precondtion.Argument.NotNull(userCredentials);
var user = this.userRepository.FindByName(userCredentials.Name);
if (user == null)
{
this.log.Info("Login attempt failed due to invalid user name");
return false;
}
if (!this.authenticationService.PasswordMatches(user, userCredentials.Password))
{
this.log.Info("Login attempt failed due to invalid password");
return false;
}
SetCurrentUser(user);
return true;
}
}
Lets write a test for scenario when incorrect user name is provided. For this scenario we don't need IAuthenticationService.
[Subject(typeof(LoginService))]
public class when_unknown_user_name_provided
{
const string UnknownUserName = "Unknown user";
User user;
UserCredentials credentials;
LogSpy logSpy;
LoginService loginService;
bool loginResult;
Establish context = () =>
{
credentials = new UserCredentials(UnknownUsername, "stub password");
var userRepository = MockRepository.GenerateStub<IUserRepository>();
userRepository.Stub(x => x.FindByName(UnknownUserName).Return(null);
// We really do not need an authenticationService for this test.
// But should stub it only to satisfy null check constraints.
// Isn't it easier and more readable to use such obviuos construction?
// var authenticationServiceNotUsed = null;
var authenticationServiceNotUsed =
MockRepository.GenerateStub<IAutenticationService>();
logSpy = new LogSpy();
loginService = new LoginService(
userRepository, authenticationServiceNotUsed, LogSpy);
};
Because of = () => loginResult = loginService.Login(userCredentials);
It should_indicate_that_user_login_failed = () => loginResult.ShouldBeFalse();
It should_not_set_current_user = () => User.Current.ShouldBeNull();
It should_log_invalid_login_attempt = () =>
logSpy.InfoMessage.ShouldBeEqualTo(
"Login attempt failed due to invalid user name");
}
And wait, as far as I have serviceAuthentication instance I might want to ensure that it was not called within a test:
It should_not_use_authentication_service = () =>
authenticationService.AssertWasNotCalled(
x => x.PasswordMatches(Arg<User>.Is.Anyting, Arg<string>.Is.Anything));
Seems like a waste of time. Intent is more clear when null authenticationService is used. That's all about testing challenge.
Now lets me return back to my statement that such null checks in a constructor are useless. So what is the advantage they might give? Earlier problem discovering by throwing ArgumentNullException in a constructor, instead of NullReferenceException somewhere while executing method later. I believe that in production code such services should not be instantiated directly by hand, but resolved with DI container (in a
recent Ayende's post you can see a very nice picture of dependencies graph). As I've already said, most of DI containers will throw an exception itself if it can't find dependency. But with DI containers even better approach is to test container configuration. For given example test may look like:
[Subject(typeof(ApplicationConfiguration))]
public class when_configuring
{
Because of = () => ApplicationConfiguration.Initialize();
It should_initialize_service_locator = () =>
ServiceLocator.Current.ShouldNotBeNull();
It should_configure_login_service = () =>
ServiceLocator.Current.GetInstance<ILoginService>().ShouldNotBeNull();
}
With this approach we get even earlier problem discovering.
Summary: This post explains why using null arguments checks in services constructors is not a good idea. The better approach is letting them be null for testing purposes and testing you DI container configuration to ensure that services may be resolved in a run-time.
Note: Tests are using syntax of the awesome Machine.Specifications framework which you can find
here. All the code examples were written without any code editor. So I expect it contains errors.
