4
\$\begingroup\$

I'm dealing with a damn scary function which I would wish to refactor so I could unit test it properly and improve the design. I will try to explain what it does and how it works in detail (with comments in the code) and with approaches I've been considering to tackle the issues:

public Result RegisterJustCustomerContact(string firstName, string lastName, string emailAddress, string Cif, string NumberColegiado, string OrganizationName,
    string DaytimePhoneNumber, string ZipCode, string Address, string Community, string City, string Province, string sanibrickCode,
        string orgid, string territoryCode)
    {
    //Code smell #1: Too many parameters. Maybe encapsulate Contact parameters and Address details in separate classes?

        var alreadyExists = true;

        //Repository is an external interface injected, IMHO ok here.
        //CustomerContact is a sealed class used by a CMS Framework I'm using.
        CustomerContact customerContact = repository.GetPharmacy(orgid);
        if (customerContact == null) {
            alreadyExists = false;

            //Code smell #2: CreateInstance is an static method that creates an instance
            //of CustomerContact. 
            customerContact = CustomerContact.CreateInstance();
        }

        customerContact.FirstName = firstName;
        customerContact.FullName = firstName + Lastname;
        customerContact.LastName = lastName;

        //Code smell #3: SiteContext singleton class with static method, unmockable
        //in its current state (and sealed in the CMS framework I'm provided). Maybe create an adapter around this class which implements ISiteContext?
        customerContact.RegistrationSource = String.Format("{0}, {1}", "Job Run", SiteContext.Current);
        customerContact.Email = emailAddress;

        //Code problem/smell #4: these indexers only work if the CMS Framework I work
        //with has these defined attributes. If not, NullReferenceExceptions are rised.
        //Maybe should I find some way to work with a possible "ICustomerContact" that works as a wrapper?
        customerContact["SannibrickCode"] = sanibrickCode;
        customerContact["TerritoryCode"] = territoryCode;
        customerContact["Orgid"] = orgid;
        customerContact["EsDelegado"] = false;

        // set up address
        //Code smell #5: Duplication. Try to fuse conditions and reduce.
        try {
            if (!alreadyExists)
                userDataHelper.SetAddress(customerContact, Address, City, Community, Province, ZipCode, OrganizationName,
                    DaytimePhoneNumber, emailAddress, firstName, lastName);
            else {
                if (customerContact.ContactAddresses.Count() == 0)
                    userDataHelper.SetAddress(customerContact, Address, City, Community, Province, ZipCode, OrganizationName,
                        DaytimePhoneNumber, emailAddress, firstName, lastName);
                else userDataHelper.SaveAddress(customerContact, customerContact.ContactAddresses.First().AddressId.ToString(), Address, City, Community, Province, ZipCode, OrganizationName,
                    DaytimePhoneNumber, emailAddress, firstName, lastName);
            }
        }
        catch { }


        MembershipUser user = null;
        //Check first if the membershipUser exists for the given emailAddress. If it does not, create it.
        if (customerContact.ProviderUserKey != null)
            user = CustomerContext.Current.GetUserForContact(customerContact);

        //Code smell #6: Membership static functions, not mockable. Maybe abstract this into some kind of "IAuthProvider" that wraps this functionality?
        if (user == null) user = Membership.GetUser(emailAddress);
        if (user == null) {
            alreadyExists = false;
            MembershipCreateStatus createStatus;
            var generatedPassword = utils.RandomString(6);
            user = Membership.CreateUser(emailAddress, generatedPassword, emailAddress,
                                        null, null, true, out createStatus);
            if (createStatus == MembershipCreateStatus.DuplicateUserName || createStatus != MembershipCreateStatus.Success)
                throw new Exception("Could not create the delegate account. The contact doesn't have an associated account and " +
                    "the email could not be found nor created");
        }

        user.IsApproved = true;

        //Once again, direct interaction with DB
        Membership.UpdateUser(user);

        //userDataHelper is an interface, OK here I guess.
        if (!alreadyExists) {
            userDataHelper.SaveProfileData(user, new List<UserWholesaler>(), Cif, NumberColegiado);
        }
        else {
            userDataHelper.SaveProfileDataCif(user, Cif);
        }

        //Bind membershipuser with the contact
        MapUserKey _mapUserKey = new MapUserKey();
        customerContact.UserId = _mapUserKey.ToTypedString(user.ProviderUserKey);

        //Code smell #7: This function interacts directly with the DB. As said, 
        //CustomerContact is provided by the framework to manage contacts in DB
        customerContact.SaveChanges();

        //Assign roles to the user
        try {
            if (!alreadyExists)
                AssignDefaultRolesToPharmacy(user);
        }
        catch {
        }

        return new Result(true);
    }

So, which would be a possible good way to implement this so I can isolate this function and unit test it properly?

\$\endgroup\$
6
\$\begingroup\$

Style

  • Use the same coding style consistently. If you aren't using braces {} for single if statements you should stick with it.

Naming

  • parameter names should be named using camelCase casing.
  • names should be meaningful so Cif doesn't tell anything about the parameter. In 6 months you or Mr.Maintainer will read this code again and you won't have any clue what this is about.

Try with empty catch

If you really want to swallow an exception, you should explain why you do so with a comment.

General

Passing 15 parameters into a method is to much. You should "group" them into some poco's. E.g

Address, City, Community, Province, ZipCode, OrganizationName, DaytimePhoneNumber

are used more than once so they would made a good AddressInfo class.

This will reduce the input parameters to 9 and can be used for userDataHelper.SetAddress() and userDataHelper.SaveAddress() methods.

You can do the same with firstName, lastName, emailAddress and name the class CustomerDetail and now your parameters are reduced to 7 which is also quite a lot, so you should consider one more poco.

This shouldn't compile at all. Where does LastName come from ?

customerContact.FullName = firstName + Lastname;  

Refactoring

I would create a new CustomerContactExt class like

public class CustomerContactExt
{

    public CustomerContact Contact { get; private set; }
    public Boolean AlreadyExists { get; private set; }

    public CustomerContactExt(AddressInfo addressInfo, CustomerDetail customerDetail, 
                              string sanibrickCode, string territoryCode, string orgId)
        : this(addressInfo, customerDetail, orgId)
    {

        //Code problem/smell #4: these indexers only work if the CMS Framework I work
        //with has these defined attributes. If not, NullReferenceExceptions are rised.
        //Maybe should I find some way to work with a possible "ICustomerContact" that works as a wrapper?
        Contact["SannibrickCode"] = sanibrickCode;
        Contact["TerritoryCode"] = territoryCode;
        Contact["Orgid"] = orgId;
        Contact["EsDelegado"] = false;

    }
    public CustomerContactExt(AddressInfo addressInfo, CustomerDetail customerDetail,
                              string orgId)
    {
        InitializeContact(orgId);
        FillCustomerDetails(customerDetail);
        FillAddressInfo(addressInfo);
    }

    private void InitializeContact(String orgId)
    {
        Contact = repository.GetPharmacy(orgId);
        AlreadyExists = Contact != null;
        if (!AlreadyExists)
        {
            //Code smell #2: CreateInstance is an static method that creates an instance
            //of CustomerContact. 
            Contact = CustomerContact.CreateInstance();
        }
    }
    private void FillCustomerDetails(CustomerDetail customerDetail)
    {
        Contact.FirstName = customerDetail.FirstName;
        Contact.FullName = customerDetail.FirstName + customerDetail.LastName;
        Contact.LastName = customerDetail.LastName;

        //Code smell #3: SiteContext singleton class with static method, unmockable
        //in its current state (and sealed in the CMS framework I'm provided). Maybe create an adapter around this class which implements ISiteContext?
        Contact.RegistrationSource = String.Format("{0}, {1}", "Job Run", SiteContext.Current);
        Contact.Email = customerDetail.EmailAddress;
    }
    private void FillAddressInfo(AddressInfo addressInfo)
    {
        try
        {
            if (!AlreadyExists || !Contact.ContactAddresses.Any())
            {
                userDataHelper.SetAddress(Contact, addressInfo);
                return;
            }
            userDataHelper.SaveAddress(Contact, Contact.ContactAddresses.First().AddressId.ToString(), addressInfo);

        }
        catch { }
    }
}

Which would reduce the former method to

public Result RegisterJustCustomerContact(CustomerDetail customerDetail, string cif, string numberColegiado, AddressInfo addressInfo, string sanibrickCode,
    string orgId, string territoryCode)
{

    var alreadyExists = true;

    //Repository is an external interface injected, IMHO ok here.
    //CustomerContact is a sealed class used by a CMS Framework I'm using.
    CustomerContactExt customerContactExt = new CustomerContactExt(addressInfo, customerDetail, 
                                              sanibrickCode, territoryCode, orgId);  

    CustomerContact customerContact = customerContactExt.Contact;  

    String emailAddress = customerContact.Email;
    MembershipUser user = null;
    //Check first if the membershipUser exists for the given emailAddress. If it does not, create it.
    if (customerContact.ProviderUserKey != null)
        user = CustomerContext.Current.GetUserForContact(customerContact);

    //Code smell #6: Membership static functions, not mockable. Maybe abstract this into some kind of "IAuthProvider" that wraps this functionality?
    if (user == null) user = Membership.GetUser(emailAddress);
    if (user == null) {
        alreadyExists = false;
        MembershipCreateStatus createStatus;
        var generatedPassword = utils.RandomString(6);
        user = Membership.CreateUser(emailAddress, generatedPassword, emailAddress,
                                    null, null, true, out createStatus);
        if (createStatus == MembershipCreateStatus.DuplicateUserName || createStatus != MembershipCreateStatus.Success)
            throw new Exception("Could not create the delegate account. The contact doesn't have an associated account and " +
                "the email could not be found nor created");
    }

    user.IsApproved = true;

    //Once again, direct interaction with DB
    Membership.UpdateUser(user);

    //userDataHelper is an interface, OK here I guess.
    if (!alreadyExists) {
        userDataHelper.SaveProfileData(user, new List<UserWholesaler>(), cif, numberColegiado);
    }
    else {
        userDataHelper.SaveProfileDataCif(user, cif);
    }

    //Bind membershipuser with the contact
    MapUserKey _mapUserKey = new MapUserKey();
    customerContact.UserId = _mapUserKey.ToTypedString(user.ProviderUserKey);

    //Code smell #7: This function interacts directly with the DB. As said, 
    //CustomerContact is provided by the framework to manage contacts in DB
    customerContact.SaveChanges();

    //Assign roles to the user
    try {
        if (!alreadyExists)
            AssignDefaultRolesToPharmacy(user);
    }
    catch {
    }

    return new Result(true);
}

Next I would extract the creation of the MembershipUser to a separate method.

private MembershipUser GetUser(CustomerContact customerContact, ref Boolean userExists)
{
    userExists = true;
    MembershipUser user = null;
    if (customerContact.ProviderUserKey != null)
    {
        user = CustomerContext.Current.GetUserForContact(customerContact);
    }
    if (user == null)
    {
         //Code smell #6: Membership static functions, not mockable. Maybe abstract this into some kind of "IAuthProvider" that wraps this functionality?
         user = Membership.GetUser(emailAddress);
    }

    if (user != null)
    {
        user.IsApproved = true;
        return user;
    }
    userExists = False;
    return CreateUser(customerContact.Email);
}  

private MembershipUser CreateUser(String emailAddress)
{
    MembershipCreateStatus createStatus;
    var generatedPassword = utils.RandomString(6);
    MembershipUser user = Membership.CreateUser(emailAddress, generatedPassword, emailAddress,
                                null, null, true, out createStatus);
    if (createStatus == MembershipCreateStatus.DuplicateUserName || createStatus != MembershipCreateStatus.Success)
    {
        throw new Exception("Could not create the delegate account. The contact doesn't have an associated account and the email could not be found nor created");
    }
    user.IsApproved = true;
    return user;
}  

which reduces the former method to

public Result RegisterJustCustomerContact(CustomerDetail customerDetail, string cif, string numberColegiado, AddressInfo addressInfo, string sanibrickCode,
    string orgId, string territoryCode)
{

    CustomerContactExt customerContactExt = new CustomerContactExt(addressInfo, customerDetail, 
                                              sanibrickCode, territoryCode, orgId);  

    CustomerContact customerContact = customerContactExt.Contact;  

    var alreadyExists = true;

    MembershipUser user = GetUser(customer, ref alreadyExists);

    //Once again, direct interaction with DB
    Membership.UpdateUser(user);

    //userDataHelper is an interface, OK here I guess.
    if (!alreadyExists) 
    {
        userDataHelper.SaveProfileData(user, new List<UserWholesaler>(), cif, numberColegiado);
    }
    else 
    {
        userDataHelper.SaveProfileDataCif(user, cif);
    }

    //Bind membershipuser with the contact
    customerContact.UserId = (new MapUserKey()).ToTypedString(user.ProviderUserKey);

    //Code smell #7: This function interacts directly with the DB. As said, 
    //CustomerContact is provided by the framework to manage contacts in DB
    customerContact.SaveChanges();

    //Assign roles to the user
    try 
    {
        if (!alreadyExists)
        {
            AssignDefaultRolesToPharmacy(user);
        }
    }
    catch 
    {
    }

    return new Result(true);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Excellent answer as usual, although I would advocate the usage of var on the left hand side of a variable declaration if the right hand side is explicitly declared. \$\endgroup\$ – d347hm4n Nov 26 '14 at 9:32
  • \$\begingroup\$ @d347hm4n: I second that, reason being that refactoring types becomes a lot easier. \$\endgroup\$ – Nick Udell Nov 26 '14 at 16:02
  • \$\begingroup\$ Thank you for your answer! Please find an update to my post with an attempt to refactor it. \$\endgroup\$ – David Jiménez Martínez Nov 28 '14 at 8:44
3
\$\begingroup\$

The method does too many things and I would first look into refactoring "hard" dependencies into method params as interfaces, and then refactor this big method into smaller methods.

Here is my list: (maybe some points have already been made)

  • create a wrapper around everything cms related called ICMSWrapper and pass it as arg
  • create a wrapper around everything membership and auth related called IMembership and pass it as arg
  • same applies to other dependencies
  • it seems the method returns either a result of true or throws an exception. Maybe just return a void, which means if no exception was thrown then assume everything is OK.
  • group all contactinfo in a CustomerContactInfo class
  • be consistent in how braces are used, sometime they are there and sometimes not, i suggest you always stick to using braces
  • if userDataHelper is an interface as the comment says then rename to IUserDataHelper, btwi think the name is not specific enough ( data and helper)
  • there is no naming consistency. choose a pattern and stick to it.
  • if it is too tricky to change too much, then at least extract parts of the method into smaller parts so at least it is a bit more readable.
  • try to identify what this method actually does. I found that it basically does 2 things: create a customercontact and set its values, get or create a user and update the user with profile data and roles. So why not have 2 methods called in sequence?
  • Its funny that the method is called RegisterJustCustomerContact when it does plenty of other things.
  • consider making CustomerContact (and perhaps other data structs) immutable. The code becomes hopefully less error prone. But that would maybe be a step to take after all other issues are resolved.
  • regarding alreadyExists which annoys me a bit, you can figure out a better way to pass around state, now that you have multiple methods. Perhaps have a state struct and pass it along as method param, or have the class bookkeep it.

I refactored the code, take a look at it and it is far from ready, and full of mistakes, but I hope you get the points i try to make. Basically, I divided the method into smaller methods, I removed the "hard" dependencies in the method body with interfaces provided as method params, and some minor issues. This way the code is more testable. This could be further improved but this is all I have time for now. good luck!

class Program
    {
        static void Main(string[] args)
        {
            var test = new Test();

            var customerContact = test.RegisterCustomerContact(new CustomerContactInfo(...), cmd, context,...);

            test.UpdateCodes(customerContact, sanibrickCode, territoryCode, orgid);

            test.UpdateAddress(customerContact, contactInfo, alreadyExists);

            test.RegisterUser();
        }
    }
    public class Test
    {

        public void UpdateCodes(
            CustomerContact customerContact, 
            object sanibrickCode, 
            object territoryCode, 
            object orgid)
        {
            try
            {
                customerContact["SannibrickCode"] = sanibrickCode;
                customerContact["TerritoryCode"] = territoryCode;
                customerContact["Orgid"] = orgid;
                customerContact["EsDelegado"] = false;
            }
            catch (NullReferenceException e)
            {
                throw new Exception("could not update customer contact", e);
            }
        }

        public CustomerContact RegisterCustomerContact(
            CustomerContactInfo contactInfo,
            ICMSWrapper cms,
            SiteContext siteContext,
            string sanibrickCode,
            string orgid,
            string territoryCode)
        {
            var alreadyExists = true;

            var customerContact = repository.GetPharmacy(orgid);
            if (customerContact == null)
            {
                alreadyExists = false;
                customerContact = cms.CreateCustomerContactInstance();
            }

            customerContact.FirstName = contactInfo.FirstName;
            customerContact.LastName = contactInfo.LastName;
            customerContact.FullName = contactInfo.FirstName + contactInfo.LastName;
            customerContact.RegistrationSource = String.Format("{0}, {1}", "Job Run", siteContext);
            customerContact.Email = contactInfo.EmailAddress;

            return customerContact;
        }

        public void RegisterUser(
            CustomerContactInfo contactInfo,
            CustomerContact customerContact
            IMapper mapper,
            IMembership membership,
            IUserDataHelper userDataHelper)
        {
            var user = membership.GetUser(customerContact, contactInfo);
            if (user == null)
            {
                alreadyExists = false;
                membership.CreateUser(contactInfo);
            }

            membership.UpdateUser(user);

            if (!alreadyExists)
            {
                userDataHelper.SaveProfileData(user, new List<UserWholesaler>(), Cif, NumberColegiado);
            }
            else
            {
                userDataHelper.SaveProfileDataCif(user, Cif);
            }

            mapper.Map(customerContact, user);

            customerContact.SaveChanges();

            if (!alreadyExists)
            {
                AssignDefaultRolesToPharmacy(user);
            }
        }

        public void UpdateAddress(CustomerContact customerContact,      

CustomerContactInfo contactInfo, bool alreadyExists)
        {
            try
            {
                if (!alreadyExists)
                {
                    userDataHelper.SetAddress(customerContact, contactInfo);
                }
                else
                {
                    if (!customerContact.ContactAddresses.Any())
                    {
                        userDataHelper.SetAddress(customerContact, contactInfo);
                    }
                    else
                    {
                        userDataHelper.SaveAddress(customerContact,

customerContact.ContactAddresses.First().AddressId.ToString(), contactInfo);
                    }
                }
            }
            catch
            {
               throw new Exception("could not update address");
            }
        }
    }
\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer! Please find an update to my post with an attempt to refactor it. \$\endgroup\$ – David Jiménez Martínez Nov 28 '14 at 8:44
1
\$\begingroup\$

Thank your to all your suggestions. So far, I've arrived to the following degree of refactorization. Are there further suggestions?

First, I encapsulated and reorganized all the parameters, so the new call was simplified:

public class PharmacyInfo {
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public string Email { get; set; }
    public string Cif { get; set; }
    public string NumberColegiado { get; set; }
    public string OrgId { get; set; }
    public bool EsDelegado { get; set; }
    public string Sanibrick { get; set; }
    public string Territory { get; set; }
}

public class AddressInfo {
    public string Address { get; set; }
    public string Phone { get; set; }
    public string ZipCode { get; set; }
    public string OrganizationName { get; set; }
    public string City { get; set; }
    public string Community { get; set; }
    public string Province { get; set; }
}

public Result RegisterPharmacy(PharmacyInfo pharmacyInfo, AddressInfo addressInfo) {
    //Check first if a contact exists for the given orgid
    ICustomerContact pharmacy = repository.GetPharmacyInterface(pharmacyInfo.OrgId);

    //If it does not exists, build it
    if (pharmacy == null)
        pharmacy = customerFactory.CreateInstance(pharmacyInfo, addressInfo);
    else pharmacy = customerFactory.UpdateInstance(pharmacy, pharmacyInfo, addressInfo);

    //Update/Create the pharmacy
    repository.SavePharmacy(pharmacy);

    return new Result(true);
}

Let's get deep with the RegisterPharmacy: I refactored the sealed class "CustomerContact" using the adapter pattern. I identified that in order to register a customer in my system I needed 2 steps: a MembershipUser and a CustomerContact, so I combined them together into one interface, ICustomerContact, and created an implementation for that interface:

public interface ICustomerContact
{
    PrimaryKeyId? PrimaryKeyId { get; set; }
    IEnumerable<CustomerAddress> ContactAddresses { get; }

    CustomerAddress PreferredBillingAddress { get; set; }
    CustomerAddress PreferredShippingAddress { get; set; }

    string Email { get; set; }
    string FirstName { get; set; }
    string FullName { get; set; }
    string LastName { get; set; }
    DateTime? LastOrder { get; set; }

    PrimaryKeyId? PreferredBillingAddressId { get; set; }
    PrimaryKeyId? PreferredShippingAddressId { get; set; }
    string RegistrationSource { get; set; }
    object ProviderUserKey { get; }

    string Cif { get; set; }
    string Orgid { get; set; }
    bool EsDelegado { get; set; }
    string NumberColegiado { get; set; }
    Dictionary<IWarehouse, string> AssociatedWholesalers { get; set; }

    void DeleteContactAddress(CustomerAddress address);
    void UpdateContactAddress(CustomerAddress address);
    void AddContactAddress(CustomerAddress address);

    void SaveChanges();
}

public class Pharmacy : ICustomerContact
{
    public CustomerContact Contact {
        get;
        private set;
    }

    public PrimaryKeyId? PrimaryKeyId {
        get {
            return Contact.PrimaryKeyId;
        }
        set {
            Contact.PrimaryKeyId = value;
        }
    }

    public MembershipUser User {
        get;
        private set;
    }

    public Pharmacy(CustomerContact contact, MembershipUser membershipUser) {
        Contact = contact;
        User = membershipUser;

        var profile = EPiServerProfile.Get(User.UserName);
        if (profile != null) {
            var wholesalerList = profile[Constants.Profile.WholesalersData] as List<string>;
            AssociatedWholesalers = wholesalerList == null ? new Dictionary<IWarehouse, string>() :
                UserWholesaler.ConvertToWholesalerList(wholesalerList).ToDictionary(uw => WarehouseHelper.GetWarehouse(uw.WholesalerId), uw => uw.CollaborationCode);
            Cif = profile[Constants.Profile.CifCode] as string;
            NumberColegiado = profile[Constants.Profile.NumberColegiado] as string;
        }
        else AssociatedWholesalers = new Dictionary<IWarehouse, string>();
    }

    public void SaveChanges() {
        if (User != null) {
            Membership.UpdateUser(User);
            MapUserKey _mapUserKey = new MapUserKey();
            Contact.UserId = _mapUserKey.ToTypedString(User.ProviderUserKey);
            var wholesalersString = AssociatedWholesalers.Select(aw => new UserWholesaler {
                CollaborationCode = aw.Value,
                Region = aw.Key.ContactInformation.RegionCode,
                RegionName = aw.Key.ContactInformation.RegionName,
                WholesalerId = aw.Key.WarehouseId.Value,
                WholesalerName = aw.Key.Name,
                WholesalerCode = aw.Key.Code
            }.ToString()).ToList();

            var profile = EPiServerProfile.Get(User.UserName);
            profile[Constants.Profile.WholesalersData] = wholesalersString;
            profile[Constants.Profile.CifCode] = Cif;
            profile[Constants.Profile.NumberColegiado] = NumberColegiado;
            profile.Save();
        }

        Contact.SaveChanges();

        var defaultAddress = ContactAddresses.FirstOrDefault();
        this.PreferredBillingAddressId = defaultAddress != null ? defaultAddress.PrimaryKeyId : null;
        this.PreferredShippingAddressId = defaultAddress != null ? defaultAddress.PrimaryKeyId : null;

        Contact.SaveChanges();
    }

    public IEnumerable<CustomerAddress> ContactAddresses {
        get { return Contact.ContactAddresses; }
    }

    public CustomerAddress PreferredBillingAddress {
        get {
            return Contact.PreferredBillingAddress ?? lastAddedAddress;
        }
        set {
            Contact.PreferredBillingAddress = value;
        }
    }

    public CustomerAddress PreferredShippingAddress {
        get {
            return Contact.PreferredShippingAddress ?? lastAddedAddress;
        }
        set {
            Contact.PreferredShippingAddress = value;
        }
    }

    public string Email {
        get {
            return Contact.Email;
        }
        set {
            Contact.Email = value;
        }
    }

    public string FirstName {
        get {
            return Contact.FirstName;
        }
        set {
            Contact.FirstName = value;
        }
    }

    {...}
}

The method "SaveChanges" from CustomerContact had direct contact with the Database (unavoidable) but I wanted to manage all my connections to database through Repositories (repository pattern). So I ended up adding a new operation into the repository to which calls the SaveChanges function. probably not the best solution, but it works out quite well:

public interface IPharmacyRepository
{
    void SavePharmacy(ICustomerContact pharmacy);
}
{...}
public void SavePharmacy(ICustomerContact pharmacy) {
    pharmacy.SaveChanges();
}

Then I extracted all the logic to create an instance of ICustomerContact to a factory:

public interface ICustomerFactory
{
    ICustomerContact CreateInstance(PharmacyInfo pharmacyInfo, AddressInfo addressInfo);
    ICustomerContact UpdateInstance(ICustomerContact contact, PharmacyInfo pharmacyInfo, AddressInfo addressInfo);
}

public class PharmacyFactory : ICustomerFactory
{
    ICustomerContextWrapper customerContext;
    IMembershipProvider membershipProvider;

    public PharmacyFactory(ICustomerContextWrapper customerContext, IMembershipProvider membershipProvider) {
        this.customerContext = customerContext;
        this.membershipProvider = membershipProvider;
    }

    public ICustomerContact UpdateInstance(ICustomerContact contact, PharmacyInfo pharmacyInfo, AddressInfo addressInfo) {
        if (!pharmacyInfo.EsDelegado) {
            var address = contact.ContactAddresses.FirstOrDefault();
            if (address == null) contact.AddContactAddress(CreateAddress(contact, addressInfo));
            else contact.UpdateContactAddress(UpdateAddress(address, contact, addressInfo));
        }

        var membershipUser = UpdateMembership(contact, pharmacyInfo.Email, pharmacyInfo.Email);
        try {
            this.AssignDefaultRoles(membershipUser, pharmacyInfo.EsDelegado);
        }
        catch { /*Swallow the exception due to possible duplication of Role assignment
                 Consider refactor this later*/
        }

        contact = new Pharmacy(customerContext.GetContactById(contact.PrimaryKeyId.Value), membershipUser);
        contact = SetProperties(contact, pharmacyInfo);

        return contact;
    }

    public ICustomerContact CreateInstance(PharmacyInfo pharmacyInfo, AddressInfo addressInfo) {
        var customerContact = CustomerContact.CreateInstance();

        var membershipUser = CreateMembership(pharmacyInfo.Email, pharmacyInfo.Email);
        try {
            this.AssignDefaultRoles(membershipUser, pharmacyInfo.EsDelegado);
        }
        catch { /*Swallow the exception due to possible duplication of Role assignment.
                 Consider refactor this later*/ }

        ICustomerContact pharmacy = new Pharmacy(customerContact, membershipUser);
        pharmacy = this.SetProperties(pharmacy, pharmacyInfo);
        if (!pharmacyInfo.EsDelegado)
            pharmacy.AddContactAddress(CreateAddress(pharmacy, addressInfo));

        return pharmacy;
    }

    private ICustomerContact SetProperties(ICustomerContact contact, PharmacyInfo pharmacyInfo) {
        contact.FirstName = pharmacyInfo.FirstName;
        contact.FullName = pharmacyInfo.FirstName;
        contact.LastName = pharmacyInfo.LastName;
        contact.RegistrationSource = String.Format("{0}, {1}", "Job Run", SiteContext.Current);
        contact.Email = pharmacyInfo.Email;
        contact.Orgid = pharmacyInfo.OrgId;
        contact.EsDelegado = pharmacyInfo.EsDelegado;
        contact.Cif = pharmacyInfo.Cif;
        return contact;
    }

    private MembershipUser UpdateMembership(ICustomerContact contact, string email, string username) {
        MembershipUser membershipUser = null;
        if (contact.ProviderUserKey != null)
            membershipUser = membershipProvider.GetUser(contact.ProviderUserKey);
        else membershipUser = CreateMembership(email, username);

        return membershipUser;
    }

    private MembershipUser CreateMembership(string email, string username) {
        //Check first if the membershipUser exists for the given emailAddress. If it does not, create it.
        MembershipUser membershipUser = membershipProvider.GetUser(username);
        if (membershipUser == null) {
            var generatedPassword = membershipProvider.GeneratePassword(6, 3);
            membershipUser = membershipProvider.CreateUser(username, generatedPassword, email);
            if (membershipUser == null)
                throw new Exception("Could not create the delegate account. The contact doesn't have an associated account and " +
                    "the email could not be found nor created");
        }

        membershipUser.IsApproved = true;

        return membershipUser;
    }

    private void AssignDefaultRoles(MembershipUser user, bool esDelegado) {
        string[] DefaultUserRoles = { AppRoles.RegisteredRole, AppRoles.EveryoneRole, AppRoles.ManagerUserRole };

        var roles = from roleName in DefaultUserRoles
                    where !SecurityContext.Current.CheckUserInGlobalRole(user, roleName)
                    select roleName;

        //Assign default roles to new user
        foreach (var roleName in roles) {
            var principal = SecurityContext.Current.GetPrincipalByUser(user);
            var globalRoleAssign = new GlobalRoleAssignment(principal, roleName);
            SecurityContext.Current.CreateUserRoleAssignments(globalRoleAssign);
        }

        if (AppSettings.Roles_Guest != null && Roles.RoleExists(AppSettings.Roles_Guest))
            Roles.AddUserToRole(user.UserName, AppSettings.Roles_Guest);

        if (esDelegado && Roles.RoleExists("Delegados"))
            Roles.AddUserToRole(user.UserName, "Delegados");
    }

    private CustomerAddress CreateAddress(ICustomerContact pharmacy, AddressInfo addressInfo) {

        var address = CustomerAddress.CreateInstance();

        return UpdateAddress(address, pharmacy, addressInfo);
    }

    private CustomerAddress UpdateAddress(CustomerAddress address, ICustomerContact pharmacy, AddressInfo addressInfo) {
        address.Name = "Primero";
        address.Line1 = addressInfo.Address;
        address.City = addressInfo.City;
        address.CountryCode = "es";
        address.CountryName = "Spain";
        address.State = addressInfo.Community;
        address.RegionName = addressInfo.Province;
        address.PostalCode = addressInfo.ZipCode;
        address.OrganizationName = addressInfo.OrganizationName;
        address.DaytimePhoneNumber = addressInfo.Phone;
        address.Email = pharmacy.Email;
        address.FirstName = pharmacy.FirstName;
        address.LastName = pharmacy.LastName;

        return address;
    }
}

Unfortunately Membership holds heavy dependencies for the Web server, so I created also a wrapper for it:

public interface IMembershipProvider
{
     MembershipUser GetUser();
     {...}
}

All the dependencies are managed by an IoC container.

After all these changes, now I can finally mock dependencies and Unit test it quite well. Here are some of the tests:

[Test]
public void UpdateExistingUser() {
    //Arrange
    var customerfactoryMock = new Mock<ICustomerFactory>();
    var pharmacyrepoMock = new Mock<IPharmacyRepository>();
    var customermock = new Mock<ICustomerContact>();

    pharmacyrepoMock.Setup(m => m.GetPharmacyInterface(It.IsAny<string>()))
        .Returns(customermock.Object);

    customerfactoryMock.Setup(m => m.UpdateInstance(It.IsAny<ICustomerContact>(), It.IsAny<PharmacyInfo>(), It.IsAny<AddressInfo>()))
        .Returns(customermock.Object);

    var assignmentstestData = TestDataFactory.GetAssignments();
    var regHelper = new RegistrationHelper(Mock.Of<IMailSender>(), null, new Utils(), null, pharmacyrepoMock.Object, customerfactoryMock.Object);

    //Act
    var result = regHelper.RegisterPharmacy(new PharmacyInfo() { OrgId = "1234567890" }, null);

    //Assert
    customerfactoryMock.Verify(m => m.UpdateInstance(It.IsAny<ICustomerContact>(), It.IsAny<PharmacyInfo>(), It.IsAny<AddressInfo>()), Times.Once());
    Assert.AreEqual(result.Success, true);
}

[Test]
public void RegisterNewUser() {
    //Arrange
    var customerfactoryMock = new Mock<ICustomerFactory>();
    var pharmacyrepoMock = new Mock<IPharmacyRepository>();
    var customermock = new Mock<ICustomerContact>();

    customerfactoryMock.Setup(m => m.CreateInstance(It.IsAny<PharmacyInfo>(), It.IsAny<AddressInfo>()))
        .Returns(customermock.Object);

    var assignmentstestData = TestDataFactory.GetAssignments();
    var regHelper = new RegistrationHelper(Mock.Of<IMailSender>(), null, new Utils(), null, pharmacyrepoMock.Object, customerfactoryMock.Object);

    //Act
    var result = regHelper.RegisterPharmacy(new PharmacyInfo() { OrgId = "1234567890" }, null);

    //Assert
    customerfactoryMock.Verify<ICustomerContact>(m => m.CreateInstance(It.IsAny<PharmacyInfo>(), It.IsAny<AddressInfo>()), Times.Once());
    Assert.AreEqual(result.Success, true);
}

[Test]
public void CreateCustomerContact() {
    //Arrange
    var customerContextMock = new Mock<ICustomerContextWrapper>();
    var membershipProviderMock = new Mock<IMembershipProvider>();
    var membershipUserMock = new Mock<MembershipUser>();
    membershipUserMock.SetupGet(m => m.ProviderUserKey).Returns("Hola");
    membershipProviderMock.Setup(m => m.GetUser(It.IsAny<string>()))
        .Returns(membershipUserMock.Object);

    var factory = new PharmacyFactory(customerContextMock.Object, membershipProviderMock.Object);
    var pharmacyInfo = new PharmacyInfo() {
            Cif = "12345678A",
            Email = "a@a.com",
            EsDelegado = false,
            FirstName = "Pablo",
            LastName = "Neruda",
            OrgId = "1234567890"
        };

        var addressInfo = new AddressInfo() {
            City = "Barcelona",
            Community = "Catalunya",
            Phone = "123456788",
            Province = "Barcelona",
            Address = "Calle absurda"
        };

    //Act
    var result = factory.CreateInstance(pharmacyInfo, addressInfo);

    //Assert
    Assert.AreEqual(pharmacyInfo.OrgId, result.Orgid);
    Assert.AreEqual(pharmacyInfo.Cif, result.Cif);
    Assert.AreEqual(addressInfo.Address, result.PreferredBillingAddress.Line1);
}
\$\endgroup\$
  • \$\begingroup\$ It's customary to ask new questions as new questions. No one is going to look at your updated code and provide more advice unless you ask a new question. \$\endgroup\$ – RubberDuck Feb 16 '15 at 15:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.