When writing code, it’s unusual for me to get it right the first time. Typically I’ll just write a big block of statements that do something that I’ll need done. Then I’ll verify that it works. Then I’ll look at it, notice what an ugly mess it is, and refactor it so that the poor developer who inherits this code (and that developer might very well be me) will have some hope of maintaining it. Here’s a typical example that came up this week.
The scenario
I’m working in a controller class in an ASP.Net MVC2 application. I have some action methods that need to save the data passed in via a signup form. The signup form collects all of the data needed to create a new company and a new user in my application. So my signup form is a composite of Company data, User data, an other data that’s needed for the UI (like a ConfirmPassword field). I’ve chosen to encapsulate all of this stuff that the signup form needs in a view model class called SignupVM. It looks like this.
public class SignupVM
{
public User User { get; set; }
public Company Company { get; set; }
public string ConfirmPassword { get; set; }
public string FormError { get; set; }
public string FormMessage { get; set; }
public SignupVM()
{
this.User = new User();
this.Company = new Company();
this.ConfirmPassword = string.Empty;
this.FormError = string.Empty;
this.FormMessage = string.Empty;
}
}
So that part works really well. Here’s where it starts to get a little messy. I have a couple of Action Methods in my controller that receive a SignupVM that’s been populated with data by the View (or more accurately, the ModelBinder) and they need to save that data. Saving data involves three main steps. First I need to validate the Company, the User, and my ConfirmPassword field. Second I need to actually save the data, but I need to save the Company first and then make sure I set the correct CompanyGuid on the User before I save the User data. Third, I have some standard data that needs to get created for all new signups. Right now the only standard data is the GeneralApplicantsJob but I know that there will be a need for more standard data down the road. To handle this stuff, I created a TrySave() method in my controller class that looks like this.
// TrySave - SignupVM
private bool TrySave(SignupVM vm)
{
var userValidator = new UserValidator();
var companyValidator = new CompanyValidator();
var isError = false;
// validate user
try
{
userValidator.Validate(vm.User);
ValidateConfirmPassword(vm);
}
catch (RuleException e)
{
isError = true;
e.ErrorList.SetUiFieldNames(GetUserFieldNameMappings());
e.ErrorList.CopyToModelState(ModelState, "User");
}
// validate company
try
{
companyValidator.Validate(vm.Company);
}
catch (RuleException e)
{
isError = true;
e.ErrorList.SetUiFieldNames(GetCompanyFieldNameMappings());
e.ErrorList.CopyToModelState(ModelState, "Company");
}
// validate vm level fields
try
{
ValidateConfirmPassword(vm);
}
catch (RuleException e)
{
isError = true;
e.ErrorList.CopyToModelState(ModelState, "");
}
// if error, return false
if (isError) { return false; }
// if no error let's save the Company
this.CompanyService.Save(vm.Company);
// set the companyguid for the user
vm.User.CompanyGuid = vm.Company.CompanyGuid;
//save the user
this.UserService.Save(vm.User);
// create the GeneralApplicants job, it's created the first time we get it.
this.JobService.GetGeneralApplicantsJobForCompany(vm.Company.CompanyGuid);
return true;
}
Let’s Refactor
So, this block of code isn’t unmanageable, but I could definitely see it becoming unmanageable if I leave it like this and it gets added to a few times. Plus, it’s not immediately clear what this code is doing. What am I returning? Where am I returning it? What logic decides what my return value is? Looking at it I realize that I’m writing in a very Imperative style that puts the focus on the individual steps of “how” to do something, instead of writing in a Declarative (or Functional) style that puts the focus on “what” is being done. The code above is just a big list of steps that are a little difficult to follow without the comments. I’m just validating data and saving data, so why do I have all of these statements in my method? I can do better.
First let’s extract all of the validation code into an IsValidSignup() method. Just that one refactoring helps quite a bit. Now our TrySave method looks like this.
// TrySave - SignupVM
private bool TrySave(SignupVM vm)
{
if (!IsValidSignup(vm)) { return false; }
// if no error let's save the Company
this.CompanyService.Save(vm.Company);
// set the companyguid for the user
vm.User.CompanyGuid = vm.Company.CompanyGuid;
//save the user
this.UserService.Save(vm.User);
// create the GeneralApplicants job, it's created the first time we get it.
this.JobService.GetGeneralApplicantsJobForCompany(vm.Company.CompanyGuid);
return true;
}
The next step is to refactor the data saving code. There’s two things that bother me about this code. First it’s critical that the the entities (Company and User) are saved it the right order, and that the user.CompanyGuid needs to be set in between the saves. Second, it’s critical that the GeneralApplicantsJobForCompany get created after the Company has been saved, plus I know that there will be a need for more standard data later on, which means at some point a developer is going to have to modify this helper method in a controller class to make sure signups are created with the right standard data. That feels like something that should be in my business layer, not in my UI code. So, I decided to extract all of the persistence logic (including the creation of standard data) to a new method on my SystemService class in my business layer. The mehtod is called SignupNewCompany() and now my TrySave method looks like this.
// TrySave - SignupVM
private bool TrySave(SignupVM vm)
{
if (!IsValidSignup(vm)) { return false; }
this.SystemService.SignupNewCompany(vm.Company, vm.User);
return true;
}
This looks much cleaner to me. The focus is on what I’m doing instead of how I’m doing it, and the intent of the code is much more clear even though it no longer has comments. Now I do have three methods instead of one, but each of those three methods does one thing that is easy to understand at a glance. Best of all, I realized that I had put some logic in my UI that really needed to live in my business layer where it would be easier to reuse and easier to maintain. For anyone who’s interested, here are all 3 methods in their final form.
// TrySave - SignupVM
private bool TrySave(SignupVM vm)
{
if (!IsValidSignup(vm)) { return false; }
this.SystemService.SignupNewCompany(vm.Company, vm.User);
return true;
}
// IsValidSignup
public bool IsValidSignup(SignupVM vm)
{
var userValidator = new UserValidator();
var companyValidator = new CompanyValidator();
var isError = false;
// validate user
try
{
userValidator.Validate(vm.User);
}
catch (RuleException e)
{
isError = true;
e.ErrorList.SetUiFieldNames(GetUserFieldNameMappings());
e.ErrorList.CopyToModelState(ModelState, "User");
}
// validate company
try
{
companyValidator.Validate(vm.Company);
}
catch (RuleException e)
{
isError = true;
e.ErrorList.SetUiFieldNames(GetCompanyFieldNameMappings());
e.ErrorList.CopyToModelState(ModelState, "Company");
}
// validate vm level fields
try
{
ValidateConfirmPassword(vm);
}
catch (RuleException e)
{
isError = true;
e.ErrorList.CopyToModelState(ModelState, "");
}
return !isError;
}
public void SignupNewCompany( Company newCompany, User newUser)
{
// create the new company
this.CompanyService.Insert(newCompany);
// create the new user
newUser.CompanyGuid = newCompany.CompanyGuid;
this.UserService.Save(newUser);
// create the GeneralApplicants job, it's created the first time we get it.
this.JobService.GetGeneralApplicantsJobForCompany(newCompany.CompanyGuid);
}
No comments:
Post a Comment