1
\$\begingroup\$

Purpose

This was designed to be able to seamlessly replace string SAPCustomerID and int CustomerID in parameter lists with CustomerIdentity customer without breaking existing code. This will allow the developer to choose which they can provide and gain access to the other (if needed). Parameters lists being Web API functions or Service functions alike.

Cache

The cache is initialized at the start of the application using CustomerIdentity.SetCache and needs to be thread-safe (assuming ICustomerCache is thread-safe).

Immutable

The class is also designed to be immutable so that I may avoid all of the tedious checks that come along with the two ways to identify a customer (e.g. CustomerID > 0 and String.IsNullOrWhiteSpace(SAPCustomerID)).

Consistency

I am considering the removal/adjustment of the CustomerIdentity(int id, string sapCustomerID) constructor. Having it how it is now does open the possibility of a customer missmatch (ID from one customer and SAPCustomerID from another). Haven't decided whether to remove it or use the cache to validate, leaning towards removal since the other constructors should be sufficient.

CustomerIdentity.cs

/// <summary>
/// Identifies a customer via CustomerID or SAPCustomerID.
/// This class can implicitly convert to and from an integer (CustomerID) or a string (SAPCustomerID) looking up the other in the process.
/// </summary>
public class CustomerIdentity
{
    #region Customer Cache

    private static ICustomerCache CustomerCache;
    private static readonly object CacheLock = new object();

    /// <summary>
    /// Sets the customer cache repository used to get the mappings of SAPCustomerID to CustomerID and vice-versa.
    /// </summary>
    /// <remarks>Called in Application_Start()</remarks>
    public static void SetCustomerCache(ICustomerCache cache)
    {
        lock (CacheLock)
        {
            CustomerCache = cache;
        }
    }

    #endregion

    public int ID { get; private set; }
    public string SAPCustomerID { get; private set; }

    /// <summary>
    /// Constructs a customer identity using an ID.
    /// Looks up SAPCustomerID from the cache loaded at the start of the application.
    /// </summary>
    /// <exception cref="ArgumentNullException" />
    /// <exception cref="ArgumentOutOfRangeException" />
    public CustomerIdentity(int id)
    {
        SetCustomerID(id);

        lock (CacheLock)
        {
            var sapCustomerID = CustomerCache.GetSAPCustomerID(id);
            SetSAPCustomerID(sapCustomerID);
        }
    }

    /// <summary>
    /// Constructs a customer identity using an SAPCustomerID.
    /// Looks up ID from the cache loaded at the start of the application.
    /// </summary>
    /// <exception cref="ArgumentNullException" />
    /// <exception cref="ArgumentOutOfRangeException" />
    public CustomerIdentity(string sapCustomerID)
    {
        SetSAPCustomerID(sapCustomerID);

        lock (CacheLock)
        {
            var id = CustomerCache.GetCustomerID(sapCustomerID);
            SetCustomerID(id);
        }
    }

    /// <summary>
    /// Constructs a customer identity using both ID and SAPCustomerID.
    /// No look ups required for this method of construction.
    /// </summary>
    /// <exception cref="ArgumentNullException" />
    /// <exception cref="ArgumentOutOfRangeException" />
    public CustomerIdentity(int id, string sapCustomerID)
    {
        SetCustomerID(id);
        SetSAPCustomerID(sapCustomerID);
    }

    private void SetCustomerID(int id)
    {
        if (id <= 0)
        {
            throw new ArgumentOutOfRangeException($"Parameter '{nameof(id)}' must be a positive integer.");
        }

        ID = id;
    }
    private void SetSAPCustomerID(string sapCustomerID)
    {
        if (String.IsNullOrWhiteSpace(sapCustomerID))
        {
            throw new ArgumentNullException($"Parameter '{nameof(sapCustomerID)}' cannot be null, empty, or whitespace.");
        }

        SAPCustomerID = sapCustomerID.Trim().TrimStart('0');
    }

    /// <summary>
    /// Checks if the SAPCustomerID is a route customer.
    /// </summary>
    /// <remarks>
    /// This should be renamed to IsRouteCustomer because only SAPCustomerIDs are allowed here, not routes.
    /// </remarks>
    public bool IsRouteCustomer()
    {
        return SAPCustomerID.Length <= 4;
    }

    #region Implicit Conversion Operators

    /// <summary>
    /// Implicity converts a customer identity to a string by using its SAPCustomerID as the string.
    /// </summary>
    public static implicit operator String(CustomerIdentity customer)
    {
        return customer.SAPCustomerID;
    }

    /// <summary>
    /// Implicity converts a string to a customer identity by passing the string as an SAPCustomerID to the SAPCustomerID constructor.
    /// <para/>Note - Invokes CustomerIdentity constructor.
    /// </summary>
    public static implicit operator CustomerIdentity(String sapCustomerID)
    {
        return new CustomerIdentity(sapCustomerID);
    }

    /// <summary>
    /// Implicity converts a customer identity to an integer by using its ID as the integer.
    /// </summary>
    public static implicit operator Int32(CustomerIdentity customer)
    {
        return customer.ID;
    }

    /// <summary>
    /// Implicity converts an integer to a customer identity by passing the integer as an ID to the ID constructor.
    /// <para/>Note - Invokes CustomerIdentity constructor.
    /// </summary>
    public static implicit operator CustomerIdentity(int id)
    {
        return new CustomerIdentity(id);
    }

    #endregion Implicit Conversion Operators

    #region Object Function Overrides

    /// <summary>
    /// To my understanding this is the proper was to override GetHashCode.
    /// ID and SAPCustomerID shouldn't be allowed to be their default values, so no need for null checks.
    /// </summary>
    public override int GetHashCode()
    {
        return ID.GetHashCode() ^ SAPCustomerID.GetHashCode();
    }

    /// <summary>
    /// Equality should work for CustomerIdentity, String, and Int32.
    /// CustomerIdentity will make sure both ID and SAPCustomerID match.
    /// String will check that it matches SAPCustomerID only.
    /// Int32 will check that it matches ID only.
    /// </summary>
    public override bool Equals(object obj)
    {
        if (obj is CustomerIdentity)
        {
            var customer = obj as CustomerIdentity;
            return ID == customer.ID
                && SAPCustomerID == customer.SAPCustomerID;
        }

        if (obj is int)
        {
            return ID.Equals((int)obj);
        }

        if (obj is string)
        {
            return SAPCustomerID.Equals(obj.ToString());
        }

        return false;
    }

    /// <summary>
    /// This was overridden to make debugging nicer.
    /// </summary>
    public override string ToString()
    {
        return $"{nameof(ID)}: '{ID}' {nameof(SAPCustomerID)}: '{SAPCustomerID}'";
    }

    #endregion Object Function Overrides
}

Example Usage

If we have a function to call like this:

public void GetCustomerDeliveries(CustomerIdentity customer, DateTime deliveryDate);

Then calling it would look like either of the following:

GetCustomerDeliveries(12, DateTime.Today);          // CustomerID example
GetCustomerDeliveries("1001234", DateTime.Today);   // SAPCustomerID example

Concerns

I am open to any and all suggestions to improve my code; however, there are a few things I'm particularly concerned with.

  • Is my code readable? If you delete all of the comments, could you still follow what the class does?
  • Assuming ICustomerCache is thread-safe, is my usage of the cache thread-safe inside CustomerIdentity?
\$\endgroup\$
1
\$\begingroup\$

I think your code in general is easy to read and understand, so no problem with that. I would maybe change the name SAPCustomerID to just SapId as we know we're dealing with customers from the name of the owner type.


public override int GetHashCode()
{
    return ID.GetHashCode() ^ SAPCustomerID.GetHashCode();
}

If the ID and/or SAPCustomerID each uniquely defining a Customer then I don't see the necessity for the combination here. Returning one of the hash codes should be sufficient. I assume that a CustomerIdentity is only valid if it contains both ids?


Trying this:

  CustomerIdentity identity1 = (CustomerIdentity)"12345";
  CustomerIdentity identity2 = (CustomerIdentity)"12345";
  Console.WriteLine(identity1.Equals(identity2)); // true
  Console.WriteLine(identity1 == identity2); // false

Equals() returns true because you carefully implement it to check for equality for each property, and == returns false because it just performs a ReferenceEquals(a, b) by default and because identity1 is another instance than identity2 (class/reference type).

I would expect both the above statements to return true.

You should implement the ==/!= operators using Equals() so they behave equally or find a way to return the same instance of CustomerIdentity when casting from a string or a number.


I think, I would define the cache interface as holding and returning objects of type CutstomerIdentity instead of exchanging strings for numbers and vice versa:

  public interface ICustomerCache
  {
    CustomerIdentity GetIdentity(string sapId);
    CustomerIdentity GetIdentity(int id);
  }

public void GetCustomerDeliveries(CustomerIdentity customer, DateTime deliveryDate);

Having a Get...() method returning void seems a little odd :-)

\$\endgroup\$
  • \$\begingroup\$ Oh right, I overload .Equals so infrequently I always forget it's different than ==. GetCustomerDeliveries was just a funny mistake of randomly making up an example usage on the fly :P \$\endgroup\$ – Shelby115 Feb 18 at 13:33

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.