2

We have moved to a more SRP model and found coming up with class names challenging. Previously we had a Order class that looked something like this:

public class Order
{
    public void Create()
    {
    }

    public void Dispatch()
    {
    }

    public void Cancel()
    {
    }
}

We are now splitting these 3 methods into seperate classes so they look like what i have described below:

public class OrderCreation
{
    public void CreateOrder(OrderDTO order)
    {
    }
}

public class OrderDispatch
{
    public void DispatchOrder(OrderDTO order)
    {
    }
}

public class OrderCancellation
{
    public void CancelOrder(OrderDTO order)
    {
    }
}

Is the naming of these classes and public methods sensible? Do they follow what naming should look like in a business logic or domain class? (Remember we are following SRP)

UPDATE: The classes have been modified so they are not anemic. Based on feedback we think this is a better solution.

public class OrderCreation
{
    public string CustomerName {get;set;}
    public string CustomerEmail {get;set;}
    public string Address {get;set;}

    public void CreateOrder()
    {
    }
}

public class OrderDispatch
{
    public string OrderNumber {get;set;}
    public string Courier {get;set;}

    public void DispatchOrder()
    {
    }
}

public class OrderCancellation
{
    public string OrderNumber {get;set;}

    public void CancelOrder()
    {
    }
}
  • Does each of those new classes only contain that one method? Or are there lots of other (probably private) methods in each class you just did not show us in this excerpt? – Doc Brown Mar 22 '16 at 23:00
  • Most of the time there will only be 1 public method with 1 or more private methods. – user1786107 Mar 22 '16 at 23:19
  • 1
    Like @DocBrown, I disagree with "We have moved to a more SRP model". It's perfectly in accordance with the SRP to have multiple methods in one business class, as long as they stay cohesive with this business object and don't try to include behaviour from another business object. – Spotted Mar 23 '16 at 12:03
2

If you push it even little further, and do this :

public interface ICommand
{
    void Invoke();
}

public class OrderCreation : ICommand
{
    private readonly OrderDTO _order;

    public OrderCreation(OrderDTO order)
    {
        _order = order;
    }

    public void Invoke()
    {
        // do the logic
    }
}

public class OrderDispatch : ICommand
{
    private readonly OrderDTO _order;

    public OrderDispatch(OrderDTO order)
    {
        _order = order;
    }

    public void Invoke()
    {
        // do the logic
    }
}

public class OrderCancellation : ICommand
{
    private readonly OrderDTO _order;

    public OrderCancellation(OrderDTO order)
    {
        _order = order;
    }

    public void Invoke()
    {
        // do the logic
    }
}

You end up with command pattern. This has major advantages :

  • The execution is separated from invocation. This allows other code to do something interesting with every executed command.
  • Allows to use inheritance to implement common behavior for invocation. For example, database transaction can be started and committed in base class and child class just uses database connection.
  • Allows composition of the commands to form more complex commands.

The major disadvantage is increase in complexity. This increase is only really viable if each command is has high complexity in itself. If each method is just few lines of code, building a whole command around it may seem unnecessary.

  • Thanks for replying, we will look into this further. What do you think of our updated solution in the question? (moved DTO object and added properties) – user1786107 Mar 23 '16 at 7:36
  • 1
    @user1786107 I think your updated solution might be even closer to what I proposed. – Euphoric Mar 23 '16 at 9:12
  • @user1786107: I agree, after your edit this seems to be the best fitting answer. – Doc Brown Mar 23 '16 at 12:26
  • Commands can be an excellent pattern. If you add an undo() method it is possible that Order could once again collapse into a single class. It is extremely useful if Commands can be persisted, say as database rows or XML tags, which gives rise to another classic SRP decision, whether an object should know how to persist itself. – user949300 Mar 23 '16 at 16:49
1

These look like gang of 4 Command objects (a bit). I dont like them but if thats what you want then go for it - and these names make perfect sense

From my point of view I now have my code that knows all about orders spread out way too much.

I would only abstract like this if I truly needed common behavior between 'Command's - like being able to persist then, pipeline them, indirectly execute them from scripts etc. But even then I would have a real class underneath. Maybe that would be a static class that has Order.Create, Order.Delete etc

1

In your rewrite, what does OrderDTO do? If it is indeed just an anemic "value object", as implied by the name, what you have done is separate the logic from the data, moving from OOP to old fashioned procedural programming. IMO a bad thing. It happens when SRP is overused and becomes the ZRP, a.k.a. Zero Responsibility Principle.

OTOH, if OrderDTO retains a lot of logic, first, give it a better name to reflect that. And I'd agree that possibly your original Order object did " too much" and it is a candidate for refactoring using SRP.

  • I have updated the question added properties to the classes. hopefully this is now not anemic? – user1786107 Mar 23 '16 at 7:26
  • My question was whether OrderDTO was Anemic. Or did it vanish completely in your revision? – user949300 Mar 23 '16 at 16:28
0

In and of itself, new OrderCancellation().CancelOrder(OrderDTO order); doesn't seem like much of an improvement to me.

I do think, however, that it is appropriate to separate concerns between an "Ordering System" and the "Order Submission Form". The ordering system's responsibility is to accept or reject orders (and otherwise manage orders, like assign an order id), whereas the order form conveys the content of the order request. This kind of goes to the change you're showing regarding the introduction of the OrderDTO as separate (responsibility/class) from creation and cancellation. (An order can be a complex entity with numerous line items, payment info, etc.. so it is reasonable to have an order form separated from an ordering system.)

I consider that an ordering system having both accept and cancel is a reasonable notion of Single Responsibility (i.e. managing orders or in some sense, the queue of orders); meaning that accept and cancel ought not be separated into different classes due to SRP. (You wouldn't separate the add and remove methods on a collection into separate classes (it wouldn't work), managing the collection is just one responsibility.)

Still, if you accept cancellations as recorded events and/or submission requests that have their own lifetime, what you're showing could make some sense as each such event (order, cancel) is a request of a different logical type. I would consider these items as requests, not guaranteed actions, i.e. named like OrderCancellationRequest.

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.