4
\$\begingroup\$

I have a data access layer method to cancel an order. This operation should have an associated "operation history" entry.

My first (naïve) implementation looks like this:

public void CancelOrder(int orderMastId, string reason)
{
    string order_id = orderMastId.ToString();
    var order = this.db.Commandes.Where(c => c.Order_Mast_Id == order_id);

    if (order == null || order.Count() == 0)
    {
        throw new EntityNotFoundException();
    }

    Parallel.ForEach(order, commande =>
        {
            string previousStatus = commande.Status;
            if (previousStatus != ShippingStatus.Printed)
            {
                // this is what marks the order as cancelled (legacy DB power!)
                commande.Status = ShippingStatus.Printed;

                // this table stores many operations on the order (status change, etc)
                var hist = new DB.Operations_History
                           {
                               Commande_id = commande.Commande_id,
                               Created_Date = DateTime.Now,
                               Feedback = reason,
                               Final_Shipper = string.Empty,
                               Final_Status = ShippingStatus.Printed,
                               Information = "API",
                               Initial_status = previousStatus,
                               Operation_id = 0,
                               Shipping_Info = string.Empty,
                               Shipping_Reference = string.Empty,
                               User_id = 0
                           };

                this.db.Operations_History.Add(hist);
            }
        });

    this.db.SaveChanges();
}

But this is a clear side-effect and in violation of single responsibility principle.

I'm guessing there exist a design pattern for this kind of thing, but I don't know which.

For instance, I could decide that the history entry is a kind of logging, and use (for instance) PostSharp attributes to "log" the history once the methods exits, but somehow that feels wrong, would create a whole logging structure only for operations on a single table, and might prove difficult: here the example history is very simple, but in reality there might be a bunch of rules on what to log and how exactly to log it (depending on what is logged and how, it's used differently).

\$\endgroup\$
3
\$\begingroup\$

I am not familiar with C# but if the requirement is having the following two functionalities done with design patterns, i.e.:

  1. Cancel the order
  2. Log the history

Try the ChainOfResponsibility design pattern wherein first implementation say (OrderDaoImpl would implement the order related operations like add, cancel, search, etc). Once that is done call the other implementation that logs the order.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This sounds fine, but now you've made me realize that the cancelling of an order may always need to add an entry in the history (I'll have to check the specs). In that case, is my design better suited than the CoR pattern ? (otherwise an order might be cancelled without an entry in the history table). Thanks :) \$\endgroup\$ – thomasb Dec 5 '14 at 10:32
  • \$\begingroup\$ If I have correctly gauged, you have two different tables, one for history , one for current. You may have independent queries (search()) or interlinked queries (cancelOrder()). So ensure that you have the responsibilities separated in multiple impementations. \$\endgroup\$ – thepace Dec 5 '14 at 10:36
3
\$\begingroup\$
  • Cancelling?

    I have a data access layer method to cancel an order.

    All I see is code that creates a history-entry for each order you have in your collection. Please forgive and correct me if I'm wrong but if that is the cancellation, I don't see any violation of a single responsibility principle.

  • Consistency

    I see you declaring your variables explicitly and implicitly. Use var everywhere you can or declare them all explicitly. It's just a personal choice, although I'd prefer using var wherever I can.

  • Naming

    Field names use camelCase. The name ´order_id´ is not bad as it describes well what it stands for, but orderId would be more correct as it follows the capitalization styles of Microsoft. Since order is a collection of items, rename it to orders, order would imply you're only expecting one item.

  • Code

    Checking if an IEnumerable<T> collection has any items, can be done using the Enumerable.Any() method. This is faster than using the Count() method. Let's say the collection has 3 items. Any() will return when it finds the first item whereas Count() will iterate the whole collection. But, if you have a collection that has a Length or Count property, use that for performance.

    Since you also do a null-check, I would create an extension method that combines the two. I've written this one for myself which I use often:

    public static class Extensions
    {
        public static bool AnySafe<T>(this IEnumerable<T> coll)
        {
            return coll.AnySafe(null);
        }
    
        public static bool AnySafe<T>(this IEnumerable<T> coll, Func<T, bool> predicate)
        {
            if(coll == null)
                return false;
    
            return (predicate == null) ? coll.Any() : coll.Any(predicate);
        }
    }
    
    //Usage:
    if (!order.AnySafe())
        throw new EntityNotFoundException();
    
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Cancelling: yes, due to many legacy things, cancelling an order is setting its status to sent (commande.Status = ShippingStatus.Printed;). The history table contains many things. I actually was used to using var everywhere, and I'm in the process of slowly switching to explicit declaration, so I mix them up sometimes. For order_id: for the API I'm writing I try to stick with Rails conventions, so I'm mixing my conventions apparently. Thanks for the Any() tip. Thank you ! :) \$\endgroup\$ – thomasb Dec 5 '14 at 10:11
1
\$\begingroup\$

SRP

Does the refactoring below solve the SRP issue ? I would say so. This is because the business logic/rule says that canceling an order includes adding an history entry and changing the Status to Printed. As the refactored method is not involved in these 2 actions I would say that the SRP isn't violated.

Declaring and naming variables

One should use var only, if the right hand side of the declaration makes the type obvious.

So string order_id = orderMastId.ToString(); this should be using var (if you want to use var).

But this var order = this.db.Commandes.Where(c => c.Order_Mast_Id == order_id); shouldn't use var.

As the Where() method returns an IEnumerable<T> you should use the plural so order-> orders.

Based on the naming guidlines

  • Parameters and local variables should be named using camelCase casing. So order_id -> orderId.
  • Properties should be named using PascalCase casing so c.Order_Mast_Id -> c.OrderMastId

You should not shorten the name of properties, variables or parameters.

Where() method

The Where() method won't return null. Instead it will return an Enumerable.Empty<T>, so there is no need to check if order == null.

With IEnumerable<T> you should always use Any() if you want to know if the enumerable contains at least one entry.

You can call the Where() method on an IEnumerable again. So the check inside the Parallel.ForEach should be extracted.

CancelOrder() method

Basically you aren't cancelling the orders, but you change the Status to ShippingStatus.Printed. It would be more clear if you would set the Status to ShippingStatus.Canceled.

Refactoring

public void CancelOrder(int orderMasterId, string reason)
{
    var orderId = orderMasterId.ToString();
    var orders = this.db.Commandes.Where(c => c.Order_Mast_Id == orderId);

    if (!orders.Any())
    {
        throw new EntityNotFoundException();
    }

    orders = order.Where(o=> o.Status != ShippingStatus.Printed);

    if (!orders.Any()) { return; } // we can exit early because all Status is Printed

    AddHistoryEntries(orders);

    ChangeOrderStatusToPrinted(orders);

    this.db.SaveChanges();
}

private void AddHistoryEntries(IEnumerable<Order> orders)
{
    var historyEntries = orders.Select(  
                 o => new DB.Operations_History
                     {
                               Commande_id = o.Commande_id,
                               Created_Date = DateTime.Now,
                               Feedback = reason,
                               Final_Shipper = string.Empty,
                               Final_Status = ShippingStatus.Printed,
                               Information = "API",
                               Initial_status = o.Status,
                               Operation_id = 0,
                               Shipping_Info = string.Empty,
                               Shipping_Reference = string.Empty,
                               User_id = 0
                      });


    foreach (var historyEntry in historyEntries)
    {
       this.db.Operations_History.Add(hist);
    }
}

private void ChangeOrderStatusToPrinted(IEnumerable<Order> orders)
{
    foreach (var order in orders)
    {
       order.Status = ShippingStatus.Printed;
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the naming tips. The underscores in the property is powered by the legacy DB structure. I'll try to sanitize the names in my entities. Great tips for the Where() :). For the status: due to other programs using the DB and assuming "Printed", I must set it to "Printed". Do you think I should create another status with the same value ? (it's a simple const string). Thanks for the refactoring, but it doesn't quite solve the single responsibility issue, does it ? Thank you very much :) \$\endgroup\$ – thomasb Dec 5 '14 at 10:26
  • \$\begingroup\$ Thanks. So if I understand correctly, the SRP is solved depending on the business requirements. If I always have to add an entry when cancelling an order, it's fine like this ; otherwise, I would have to use something like Chain of Responsibility. Is that OK ? \$\endgroup\$ – thomasb Dec 5 '14 at 10:42
  • \$\begingroup\$ IMHO I would say so. \$\endgroup\$ – Heslacher Dec 5 '14 at 10:44

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.