I recently did a programming exercise for a company which I didn't pass. I didn't get any good feedback from my test so I wanted some advice/opinions for what I did wrong and what I could have done better. I had to refactor this class:
using System; using System.Collections.Generic; using System.Linq; using System.Text; namespace App { public class CustomerService { public bool AddCustomer(string firname, string surname, string email, DateTime dateOfBirth, int companyId) { if (string.IsNullOrEmpty(firname) || string.IsNullOrEmpty(surname)) { return false; } if (!email.Contains("@") && !email.Contains(".")) { return false; } var now = DateTime.Now; int age = now.Year - dateOfBirth.Year; if (now.Month < dateOfBirth.Month || (now.Month == dateOfBirth.Month && now.Day < dateOfBirth.Day)) age--; if (age < 21) { return false; } var companyRepository = new CompanyRepository(); var company = companyRepository.GetById(companyId); var customer = new Customer { Company = company, DateOfBirth = dateOfBirth, EmailAddress = email, Firstname = firname, Surname = surname }; if (company.Name == "VeryImportantClient") { // Skip credit check customer.HasCreditLimit = false; } else if (company.Name == "ImportantClient") { // Do credit check and double credit limit customer.HasCreditLimit = true; using (var customerCreditService = new CustomerCreditServiceClient()) { var creditLimit = customerCreditService.GetCreditLimit(customer.Firstname, customer.Surname, customer.DateOfBirth); creditLimit = creditLimit*2; customer.CreditLimit = creditLimit; } } else { // Do credit check customer.HasCreditLimit = true; using (var customerCreditService = new CustomerCreditServiceClient()) { var creditLimit = customerCreditService.GetCreditLimit(customer.Firstname, customer.Surname, customer.DateOfBirth); customer.CreditLimit = creditLimit; } } if (customer.HasCreditLimit && customer.CreditLimit < 500) { return false; } CustomerDataAccess.AddCustomer(customer); return true; } } }
Also there is the caveat that you cannot change their test harness file:
using System; using App; namespace Harness { public class Customer { public static void ProveAddCustomer(string[] args) { /* * You MUST NOT change this code. This is an existing consumer of the CustomerService and you must maintain * backwards compatibility. */ var custService = new CustomerService(); var addResult = custService.AddCustomer("Joe", "Bloggs", "[email protected]", new DateTime(1980, 3, 27), 4); Console.WriteLine("Adding Joe Bloggs was " + (addResult ? "successful" : "unsuccessful")); } } }
This is what I did:
using System;
using System.Text.RegularExpressions;
using App.DataAccess;
using App.Entities;
using App.Repositories;
namespace App.Services
{
public class CustomerService
{
public virtual bool AddCustomer(string firstName, string surname, string email, DateTime dateOfBirth, int companyId)
{
// Validate customer fields before going further
if (!IsValidCustomerHelper(firstName, surname, email, dateOfBirth)) return false;
var company = GetCustomerCompanyHelper(companyId);
var customer = new Customer
{
Company = company,
DateOfBirth = dateOfBirth,
EmailAddress = email,
Firstname = firstName,
Surname = surname
};
SetCreditValuesHelper(company, customer);
GetCreditLimitHelper(customer);
if (customer.HasCreditLimit && customer.CreditLimit < 500)
{
return false;
}
CustomerDataAccess.AddCustomer(customer);
return true;
}
protected virtual Company GetCustomerCompanyHelper(int companyId)
{
var companyRepository = new CompanyRepository();
var company = companyRepository.GetById(companyId);
return company;
}
protected virtual void GetCreditLimitHelper(Customer customer)
{
using (var customerCreditService = new CustomerCreditServiceClient())
{
var creditLimit =
customerCreditService.GetCreditLimit(customer.Firstname, customer.Surname, customer.DateOfBirth);
customer.CreditLimit = creditLimit;
}
}
/// <summary>
/// Set
/// </summary>
/// <param name="company"></param>
/// <param name="customer"></param>
protected virtual void SetCreditValuesHelper(Company company, Customer customer)
{
if (company.Name == "VeryImportantClient")
{
// Skip credit check
customer.HasCreditLimit = false;
}
else if (company.Name == "ImportantClient")
{
// Do credit check and double credit limit
customer.HasCreditLimit = true;
using (var customerCreditService = new CustomerCreditServiceClient())
{
var creditLimit =
customerCreditService.GetCreditLimit(customer.Firstname, customer.Surname, customer.DateOfBirth);
creditLimit = creditLimit * 2;
customer.CreditLimit = creditLimit;
}
}
else
{
// Do credit check
customer.HasCreditLimit = true;
}
}
protected virtual bool IsValidCustomerHelper(string firstName, string surname, string email, DateTime dateOfBirth)
{
if (string.IsNullOrEmpty(firstName) || string.IsNullOrEmpty(surname))
{
return false;
}
var isValidEmail = Regex.IsMatch(email,
@"^(?("")("".+?(?<!\\)""@)|(([0-9a-z]((\.(?!\.))|[-!#\$%&'\*\+/=\?\^`\{\}\|~\w])*)(?<=[0-9a-z])@))" +
@"(?(\[)(\[(\d{1,3}\.){3}\d{1,3}\])|(([0-9a-z][-0-9a-z]*[0-9a-z]*\.)+[a-z0-9][\-a-z0-9]{0,22}[a-z0-9]))$",
RegexOptions.IgnoreCase);
// use regular expression because this can break easily
if (!isValidEmail)
{
return false;
}
var now = DateTime.Now;
var age = DateTime.Now.Year - dateOfBirth.Year;
if (now.Month < dateOfBirth.Month || now.Month == dateOfBirth.Month && now.Day < dateOfBirth.Day) age--;
return age >= 21;
}
}
}
I added a new class that inherits from this class so I could do dependency injection:
using System;
using App.Entities;
using App.Interfaces.Repositories;
using App.Interfaces.Services;
using App.Interfaces.Wrappers;
namespace App.Services
{
/// <summary>
/// Refactored CustomerService to CustomerCompanyService
/// This version has de-coupled dependencies to make it easier to unit test
/// and swap out implementations via interfaces
/// </summary>
public class CustomerCompanyService : CustomerService, ICustomerService
{
private readonly ICustomerCreditService _customerCreditService;
private readonly ICompanyRepository _companyRepository;
private readonly ICustomerDataAccessWrapper _customerDataAccessWrapper;
public CustomerCompanyService(ICustomerCreditService customerCreditService, ICompanyRepository companyRepository, ICustomerDataAccessWrapper customerDataAccessWrapper)
{
_customerCreditService = customerCreditService;
_companyRepository = companyRepository;
_customerDataAccessWrapper = customerDataAccessWrapper;
}
public override bool AddCustomer(string firstName, string surname, string email, DateTime dateOfBirth, int companyId)
{
// Validate customer fields before going further
if (!IsValidCustomerHelper(firstName, surname, email, dateOfBirth)) return false;
var company = GetCustomerCompanyHelper(companyId);
var customer = new Customer
{
Company = company,
DateOfBirth = dateOfBirth,
EmailAddress = email,
Firstname = firstName,
Surname = surname
};
SetCreditValuesHelper(company, customer);
GetCreditLimitHelper(customer);
if (customer.HasCreditLimit && customer.CreditLimit < 500)
{
return false;
}
_customerDataAccessWrapper.AddCustomer(customer);
return true;
}
protected override Company GetCustomerCompanyHelper(int companyId)
{
var company = _companyRepository.GetById(companyId);
return company;
}
protected override void GetCreditLimitHelper(Customer customer)
{
//TODO: NG - removed using (have to make sure IDisposable is still being used correctly
var creditLimit =
_customerCreditService.GetCreditLimit(customer.Firstname, customer.Surname, customer.DateOfBirth);
customer.CreditLimit = creditLimit;
}
/// <summary>
/// Refactored SetCreditValuesHelper
/// Reduced line count
/// </summary>
/// <param name="company"></param>
/// <param name="customer"></param>
protected override void SetCreditValuesHelper(Company company, Customer customer)
{
// Has limit if not VeryImportantClient.
// Could re-factor further and remove magic string
customer.HasCreditLimit = company.Name != "VeryImportantClient";
// Do credit check and double credit limit
if (!customer.HasCreditLimit) return;
var creditLimit =
_customerCreditService.GetCreditLimit(customer.Firstname, customer.Surname, customer.DateOfBirth);
creditLimit = creditLimit * 2;
customer.CreditLimit = creditLimit;
}
}
}
I also had to make this wrapper class so I could use their static class but also inject it as a dependency:
using App.DataAccess;
using App.Entities;
using App.Interfaces.Wrappers;
namespace App.Wrappers
{
public class CustomerDataAccessWrapper : ICustomerDataAccessWrapper
{
/// <summary>
/// This is a wrapper so we can mock adding customer without
/// changing legacy CustomerDataAccess class.
/// </summary>
/// <param name="customer"></param>
public void AddCustomer(Customer customer)
{
CustomerDataAccess.AddCustomer(customer);
}
}
}
Keep in mind, this test was about refactoring legacy code but also changing the code to where it can be unit testable. This is the full description of the task:
The most common mistakes come from candidates not following the instructions so please make sure you read them clearly and do not divert away from what is being asked
Make sure you don’t “over-engineer” the test and stick to the 2 hour limit, if the test is over engineered you will not be progressed
Ensure that you maintain and apply basic engineering principles such as SOLID, DRY, YAGNI and KISS
The only conclusion I could reach is AddCustomer should return an object with the credit limit so we can write tests around the calculation being correct. I also believe that maybe I should of added a new constructor to CustomerService which would use the dependency injection. That way I wouldn't have to make a second class (CustomerCompanyService).