1
\$\begingroup\$

Here's the flow,

  • Load XML file
  • Deserialize to list in c#
  • Map xml object to c# list
  • Merge orders but increment order's product count

    public Order[] Load(string path)
    {
        var element = XElement.Load(path);
        var orders = new List<XElement>();
        orders = element.Elements();
    
        var _orders = new List<Order>();
        var _alreadyAddedOrders = new List<string>();
        foreach (var order in orders)
        {
            var orderTemp = new Order();
            orderTemp.OrderDate = Convert.ToInt32(order.Attribute("date").ToString());
            orderTemp.Id = Convert.ToInt32(order.Attribute("id").ToString());
            orderTemp.CategoryId = Convert.ToInt32(order.Attribute("categoryId").ToString());
            orderTemp.ProductCount = Convert.ToInt32(order.Attribute("productCount").ToString());
    
            foreach (var secondOrder in _orders)
            {
                var orderMatches = (orderTemp.OrderDate == secondOrder.OrderDate && 
                                    orderTemp.Id == secondOrder.Id && 
                                    orderTemp.CategoryId == secondOrder.CategoryId);
                if (orderMatches)
                {
                    orderTemp.ProductCount += secondOrder.ProductCount;
                    _alreadyAddedOrders.Add(secondOrder.OrderDate.ToString() + secondOrder.Id.ToString() + secondOrder.CategoryId.ToString());
                }
            }
    
            if(!_alreadyAddedOrders.Contains(orderTemp.OrderDate.ToString() + orderTemp.Id.ToString() + orderTemp.CategoryId.ToString()))
                _orders.Add(orderTemp);
        }
    
        return _orders.ToArray();
    }
    
    public class Order
    {
        public int Id;
        public int CategoryId;
        public int OrderDate;
        public int ProductCount;
    }
    

and XML is like this, more then 20,000 rows,

<orders>
    <order date='2000-04-20' Id='1' categoryId='100' productCount='3' />
    <order date='2000-06-21' Id='2' categoryId='101' productCount='14' />
</orders>

Note : Order ID is not unique, date + id + category, is unique altogether

\$\endgroup\$
  • \$\begingroup\$ @Mathematics: This code is still not working. All the Convert-statements are invalid and the conversion of the date should be to DateTime. Please make sure, that you post tested code, that produce the expected result. \$\endgroup\$ – Henrik Hansen Aug 22 '18 at 14:22
  • \$\begingroup\$ @HenrikHansen the Order class (at the end of the code block) stores the date as an Integer. you should take another look at the code that has been posted, it looks fully functional to me. \$\endgroup\$ – Malachi Aug 22 '18 at 14:31
  • 1
    \$\begingroup\$ I see what you are saying, @HenrikHansen, you are very correct. Mathematics, have you tested this code to make sure that it works? is the XML that you have provided in the right format? \$\endgroup\$ – Malachi Aug 22 '18 at 15:41
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Aug 23 '18 at 7:56
  • 1
    \$\begingroup\$ @HenrikHansen answer actually addresses that very issue, in the future please make sure to post your most current working code. if you have errors in your code your question could be closed for that reason. \$\endgroup\$ – Malachi Aug 23 '18 at 13:17
4
\$\begingroup\$

This statement doesn't work:

orderTemp.Id = Convert.ToInt32(order.Attribute("id").ToString());

It should be

orderTemp.Id = Convert.ToInt32(order.Attribute("id").Value);

The Order.OrderDate should be of type DateTime in order to meet the format of the date-attribute in the xml-file

date='2000-04-20'


If you modify the Order class to:

  public class Order
  {
    public int Id;
    public int CategoryId;
    public DateTime OrderDate;
    public int ProductCount;

    public string GroupId => $"{OrderDate}{Id}{CategoryId}";

    public override string ToString()
    {
      return $"{Id} -> {CategoryId} -> {OrderDate} -> {ProductCount}";
    }

    public static Order FromXml(XElement xElement, IFormatProvider formatProvider)
    {
      if (xElement == null) throw new ArgumentNullException(nameof(xElement));

      return new Order
      {
        OrderDate = Convert.ToDateTime(xElement.Attribute("date").Value, formatProvider),
        Id = Convert.ToInt32(xElement.Attribute("id").Value),
        CategoryId = Convert.ToInt32(xElement.Attribute("categoryId").Value),
        ProductCount = Convert.ToInt32(xElement.Attribute("productCount").Value)
      };
    }

  }

Then you can handle the xml-input like:

public IEnumerable<Order> Load(string path, IFormatProvider formatProvider)
{
  var xElements = XElement.Load(path);
  var xOrders = xElements.Elements();

  formatProvider = formatProvider ?? CultureInfo.CurrentCulture;

  var groups = xOrders.Select(xo => Order.FromXml(xo, formatProvider)).GroupBy(o => o.GroupId);

  foreach (var group in groups)
  {
    Order order = group.First();
    order.ProductCount = group.Sum(o => o.ProductCount);
    yield return order;
  }
}

An alternative method to read the xml-file is to use XmlSerializer.Deserialize(...)

\$\endgroup\$
  • \$\begingroup\$ +1, Very nice. I wonder what is the complexity (Big O) of the GroupBy. It could be O(n log n), making this optimal solution (reducing it from original O(n^2) without even using the Dictionary that could be inside the GroupBy). \$\endgroup\$ – user52292 Aug 23 '18 at 6:29
5
\$\begingroup\$

Naming

var orders = new List<XElement>();
var _orders = new List<Order>();

First, according to the C# naming convention, you should never have underscores in your variable names declared inside methods. Second, this is pretty confusing.

How about :

var xmlOrders = new List<XElement>();
var convertedOrders = new List<Order>();

It's a weird (and probably controversial) thing to say, but naming has to be one of the hardest thing to do when it comes to programming. You want your name to be so clear that anyone reading your code would be able to understand it quickly. So names need to be clear and exact.

For example : secondOrder isn't a second order, so it's confusing.

What is it, how should it be named? Well it seems like this order is an alreadyAddedOrder. You might want to find another name, but it should give the same "vibe".

C# things

  1. You can use the object initializer :

    var orderTemp = new Order(); orderTemp.OrderDate = Convert.ToInt32(order.Attribute("date").ToString()); orderTemp.Id = Convert.ToInt32(order.Attribute("id").ToString()); orderTemp.CategoryId = Convert.ToInt32(order.Attribute("categoryId").ToString()); orderTemp.ProductCount = Convert.ToInt32(order.Attribute("productCount").ToString());

becomes :

var orderTemp = new Order
{
    OrderDate = Convert.ToInt32(order.Attribute("date").ToString()),
    Convert.ToInt32(order.Attribute("id").ToString()),
    //etc..
};

It's simply shorter and clearer, but it is compiled the same way as your previous code.

  1. Note : Order ID is not unique, date + id + category, is unique altogether

We've got a great case of override Equals.

As it is right now, the responsibility of deciding if two orders are equal is stored (as code) in your Load method. Is this fine? I don't think so!

An Order should be able to tell if it's equal to another Order`.

public class Order
{
    public int Id;
    public int CategoryId;
    public int OrderDate;
    public int ProductCount;

    public override Equals(object obj)
    {
       //etc.
    }
}

I recommend you read a little bit on how to correctly implement the Equals override as you'll also need to override the GetHashCode method.

  1. Don't use Convert.ToInt32 when you have a string that should be an int. Use int.Parse. reference

  2. Also, override ToString to replace orderTemp.OrderDate.ToString() + orderTemp.Id.ToString() + orderTemp.CategoryId.ToString().

Encapsulation

public class Order
{
    public int Id;
    public int CategoryId;
    public int OrderDate;
    public int ProductCount;
}

Now, you might not feel it's necessary to implement this as it's a pretty "defensive programming" thing, but I feel it's important to talk about it. If I look at this class from a business point of view, it means that if I pass an order, I should be able to modify the said order afterwards. But when an order is passed, should I be able to modify, say, the OrderDate? Not really. There's also an important point that once you override the GetHashCode method, you should consider making your object "read only", so that it doesn't mess with the hashes.

Order becomes :

public class Order
{
    public Order(int id, int categoryId, int orderDate, int productCount)
    { 
        //etc.
    }

    public int Id { get; }
    public int CategoryId { get; }
    public int OrderDate { get; }
    public int ProductCount { get; }

    //Equals and GetHashCode override.
}

General points

  • Don't use _alreadyAddedOrders to keep string. You're looking for orders, this list should contain orders.

  • Why is OrderDate an int? If it's stored as milliseconds, it should be a long, otherwise I don't see how you can parse an int to a meaningful date.

\$\endgroup\$
  • 3
    \$\begingroup\$ The Order class could be a readonly struct and implement IEquatable<Order>. I would also put some docs on the Id property with a huge warning about this not being a unique identifier. \$\endgroup\$ – Brad M Aug 22 '18 at 14:52
  • \$\begingroup\$ @BradM Or, you know, make sure it's unique. Because it should be. \$\endgroup\$ – Mast Aug 23 '18 at 7:55
  • 1
    \$\begingroup\$ @Mast If data's coming from an old legacy system there's not much you can do about it if you want to keep relation between the old and new system. \$\endgroup\$ – IEatBagels Aug 23 '18 at 12:53
  • \$\begingroup\$ @IEatBagels Of-course, legacy systems get an exception on many rules. But I haven't seen anyone mentioning legacy systems. Did I miss a comment? \$\endgroup\$ – Mast Aug 23 '18 at 13:30
  • \$\begingroup\$ @IEatBagels I agree with your point here, this isn't something we could change and belongs to legacy system \$\endgroup\$ – Mathematics Aug 23 '18 at 14:02

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.