HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Customer contact unit-testing

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
contactunittestingcustomer

Problem

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.
/

Solution

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

Code Snippets

customerContact.FullName = firstName + Lastname;
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 { }
    }
}
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);
}
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;
}
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);
}

Context

StackExchange Code Review Q#70870, answer score: 6

Revisions (0)

No revisions yet.