Dynamically sorting with IComparer

Question

I have the following code I would like reviewed. I was recently tasked with sorting products coming from SOLR. The sorting is not by description or by the product number but rather by the below criteria. Normally I use linq to use grouping and sorting, but in this case I have existing (older code) and my approach was to be able to just plug in the new code without disturbing too much of the current code by implementing the IComparable interface. My approach dynamically creates a list of unique products that has an associated sort position that is passed to ProductSortComparer.

I'm interested in feedback as well as perhaps a better approach.

Sorting Criteria

1. Sort by 10th position of the item number based of which group the char belongs to
2. Then sort by the Merchandiser Code

For example:

10th position Item Number sort order

 List<String> group1 = new List<String> { "E", "F", "G", "R", "C", "A" }; // top
 List<String> group2 = new List<String> { "B", "D", "S" };
 List<String> group3 = new List<String> { "J", "M", "Q" };
 List<String> group4 = new List<String> { "H", "I", "L" };
 List<String> group5 = new List<String> { "K", "N" }; // bottom

Merchandiser Codes (second level sort):

MerchandiseCodes

E = Everyone - Has to higher than A or G
A = Acedemic - Has to be lower than E
G = Government - Has to be lower than E

Code:

class Program
 { 
 static void Main(string[] args)
 {
 // build ArrayList of products
 ArrayList products = new ArrayList();
 Product product1 = new Product { 
 ItemNumber = "MSBCDEFG1B01",
 Description = "Dell Computer Support",
 MerchandiseCode = "E"
 };
 products.Add(product1);
 Product product2 = new Product
 {
 ItemNumber = "MSBCDEFG1E01",
 Description = "Acedemic Computer, DELL",
 MerchandiseCode = "A"
 };
 products.Add(product2);
 Product product3 = new Product
 {
 ItemNumber = "MSBCDEFG1E20",
 Description = "Government Computer, DELL",
 MerchandiseCode = "G"
 };
 products.Add(product3);
 Product product4 = new Product
 {
 ItemNumber = "MSBCDEFG1F01",
 Description = "Small Computer, DELL",
 MerchandiseCode = "E"
 };
 products.Add(product4);
 Product product5 = new Product
 {
 ItemNumber = "MSBCDEFG1M01",
 Description = "Dell Computer Repair",
 MerchandiseCode = "E"
 };
 products.Add(product5);
 // before sort
 Console.WriteLine("** Before Sort **");
 foreach (var product in products) {
 Product prod = product as Product;
 Console.WriteLine(String.Format("{0}--{1}", prod.ItemNumber, prod.Description)); 
 }
 ProductSortFactory sortFactory = new ProductSortFactory(products);
 products.Sort(sortFactory.GetComparer());
 Console.WriteLine("********************************");
 // after sort
 Console.WriteLine("** After Sort **");
 foreach (var product in products)
 {
 Product prod = product as Product;
 Console.WriteLine(String.Format("{0}--{1}", prod.ItemNumber, prod.Description));
 }
 Console.ReadLine();
 }
 }

 public class Product {
 public String ItemNumber { get; set; }
 public String Description { get; set; }
 public String MerchandiseCode { get; set; }
 }

 public class ProductSortComparer : IComparer {
 public List<ProductSortAttribute> SortOrder { get; set; }
 public int GetHashCode(Product obj)
 {
 return obj.ItemNumber.GetHashCode();
 }
 public int Compare(object x, object y)
 {
 Product product = (Product)x;
 Product otherProduct = (Product)y;
 if (this.SortOrder != null && this.SortOrder.Any())
 {
 // Find product in the index
 var thisTagged = this.SortOrder.Select((item, i) => new { Item = item, Index = (int?)i });
 int? thisIndex = (from pair in thisTagged
 where pair.Item.ItemNumber == product.ItemNumber
 select pair.Index).FirstOrDefault();
 var otherTagged = this.SortOrder.Select((item, i) => new { Item = item, Index = (int?)i });
 int? otherIndex = (from pair in otherTagged
 where pair.Item.ItemNumber == otherProduct.ItemNumber
 select pair.Index).FirstOrDefault();
 if (thisIndex > otherIndex)
 {
 return 1;
 }
 else if (thisIndex < otherIndex)
 {
 return -1;
 }
 else
 {
 //if pos 10 not found in reference list treat both obj as equal
 return 0;
 }
 }
 else
 {
 return 0;
 }
 }
 }

 public class ProductSortAttribute : IComparable<ProductSortAttribute>
 {
 public int Index { get; set; }
 public String ItemNumber { get; set; }
 public String Description { get; set; }
 public int CompareTo(ProductSortAttribute other)
 {
 if (this.Index == other.Index)
 {
 return 0;
 }
 if (this.Index > other.Index)
 {
 return 1;
 }
 return -1;
 }
 public override bool Equals(Object obj)
 {
 ProductSortAttribute temp = obj as ProductSortAttribute;
 if (this.Index == temp.Index)
 {
 return true;
 }
 return false;
 }
 public override int GetHashCode()
 {
 return this.Index.GetHashCode();
 }
 public override string ToString()
 {
 return String.Format("{0}--{1}--{2}", this.Index, this.ItemNumber, this.Description);
 }
 }

 public class ProductSortFactory
 {
 private ArrayList _products;
 public ProductSortComparer Sorter { get; set; }
 public ProductSortFactory(ArrayList products)
 {
 this._products = products;
 }
 /// <summary>
 /// Get the ProductSortComparer object for sorting
 /// </summary>
 /// <returns>ProductSortComparer object to sort Collection of Products</returns>
 public ProductSortComparer GetComparer()
 {
 this.Sorter = new ProductSortComparer();
 this.Sorter.SortOrder = this.GetSortOrder();
 return this.Sorter;
 }
 /// <summary>
 /// Builds the sort order for a collection of Products.
 /// This is public as it can be accessed without returning the ProductSortComparer
 /// Which will provide a List<ProductSortAttribute> to look up indexes (sort position)
 /// </summary>
 /// <returns>List of ProductSortAttributes </returns>
 public List<ProductSortAttribute> GetSortOrder()
 {
 List<ProductSortAttribute> sortAttributes = new List<ProductSortAttribute>();// build sort attributes 
 List<String> group1 = new List<String> { "E", "F", "G", "R", "C", "A" };
 List<String> group2 = new List<String> { "B", "D", "S" };
 List<String> group3 = new List<String> { "J", "M", "Q" };
 List<String> group4 = new List<String> { "H", "I", "L" };
 List<String> group5 = new List<String> { "K", "N" };
 foreach (var item in this._products)
 {
 if (item is Product)
 {
 Product product = item as Product;
 ProductSortAttribute attribute = new ProductSortAttribute();
 attribute.ItemNumber = product.ItemNumber.Trim();
 attribute.Description = product.Description;
 if (product.ItemNumber.Length > 9)
 {
 // find group
 if (group1.IndexOf(product.ItemNumber.Substring(9, 1)) > -1)
 {
 if (product.MerchandiseCode != null && (product.MerchandiseCode.Contains('A') || product.MerchandiseCode.Contains('G')))
 {
 attribute.Index = 1; // make lower than E - E is full product and will float to top, but MerchandiseDescription 'A' will be lower 
 }
 else
 {
 attribute.Index = 0;
 }
 }
 else if (group2.IndexOf(product.ItemNumber.Substring(9, 1)) > -1)
 {
 attribute.Index = 2;
 }
 else if (group3.IndexOf(product.ItemNumber.Substring(9, 1)) > -1)
 {
 attribute.Index = 3;
 }
 else if (group4.IndexOf(product.ItemNumber.Substring(9, 1)) > -1)
 {
 attribute.Index = 4;
 }
 else if (group5.IndexOf(product.ItemNumber.Substring(9, 1)) > -1)
 {
 attribute.Index = 5;
 }
 }
 else
 {
 attribute.Index = 5;
 }
 System.Diagnostics.Debug.WriteLine(attribute.ToString());
 sortAttributes.Add(attribute);
 }
 else
 {
 throw new InvalidCastException("ProductSorterFactory expects ArrayList of type Product");
 }
 }
 sortAttributes.Sort();
 return sortAttributes;
 }
 }

• Jeroen Vannevel: I notice you're using ArrayList. Is this because of .NET version limitations?
• PhillyNJ: @JeroenVannevel The ArrayList is returned by a 3rd party API, so that is a limitation.

Answer 1

CompareTo

This

public int CompareTo(ProductSortAttribute other)
{
 if (this.Index == other.Index)
 {
 return 0;
 }
 if (this.Index > other.Index)
 {
 return 1;
 }
 return -1;
}

can be shortened to

public int CompareTo(ProductSortAttribute other) {
 return this.Index.CompareTo(other.Index);
}

Since Int32 also implements Comparable<T>.

The same goes for ProductSortComparer#Compare inside the if statement: return thisIndex.CompareTo(otherIndex);

Intermediate variables

Considering you never use product1-5, I would suggest to drop the intermediate variables (and whitespacing) and add it to your list immediately:

products.Add(
 new Product {
 ItemNumber = "MSBCDEFG1M01",
 Description = "Dell Computer Repair",
 MerchandiseCode = "E"
 }
);

Loop casting

You can use LINQ to cast your ArrayList() with Product objects. If it contains invalid data, it will throw an InvalidCastException (which you already use manually).

Keep in mind that you don't perform a check whether or not it's a product everywhere. For example here you would end up with a NullPointerException (which will now be changed to an InvalidCastExceptipn):

foreach (var product in products.Cast<Product>.ToList())
{
 Console.WriteLine(String.Format("{0}--{1}", product.ItemNumber, product.Description));
}

FirstOrDefault

I'm not entirely sure about this (not very comfortable around the LINQ expression syntax), so someone has to verify but I believe this

(from pair in otherTagged
 where pair.Item.ItemNumber == otherProduct.ItemNumber
 select pair.Index).FirstOrDefault();

is the same as

otherTagged.FirstOrDefault(x => x.Item.ItemNumber == otherProduct.ItemNumber).Index;

Instead of first retrieving all products with that number, selecting all of their indices and then taking the first, I take the first product that has that number and take its index.

Edit: it's been pointed out to me by Mat's Mug that there is a tiny performance difference between .Where(predicate).FirstOrDefault() and .FirstOrDefault(predicate) so you might want to keep this under consideration.

• Uri Agassi: Nice answer. Regarding FirstOrDefault your suggestion will prove problematic if no item is found (the OrDefault part) - since it will cause a NullPointerException. If there is an expectation that an item will always be found - use First instead.
• PhillyNJ: @UriAgassi - interesting point as I am always weary using either First() or FirstOrDefault(). If I recall both can return null which could cause an ArgumentNullException. Usually I check for null and Any() before getting the First object. In this case, I was willing to take that chance.
• Jeroen Vannevel: @UriAgassi: good point, that's definitely something to keep in mind.
• Uri Agassi: @PhilVallone - geekswithblogs.net/berthin/archive/2011/07/22/…
• svick: @JeroenVannevel Oh, I missed that. In that case, I would limit that ArrayList to the code that deals with that library by making the constructor of ProductSortFactory accept IEnumerable<Product> and use Cast<Product> on that ArrayList in the calling code. (And in the long term, try to run away from that API. :-))