1

I've been trying to make my code cleaner and I have came across an issue which I don't know what the best solution is.

This is the code I want to "clean". Please bare in mind that I am refactoring this code and its is not complete. I have replaced code with comments so to focus on the target issue.

void Order::cancel()
{
    // Check if order exists

    // Start of issue ----------
    Client client = getClient();
    OrderState lastCancellableOrderState = client.getLastCancelOrderState();

    if (lastCancellableOrderState > OrderState::Released && getState() != OrderState::Pending)
    {
        //throw exception"e.g. order not pending and client doesnt allow.."
    }
    if (getState() > lastCancellableOrderState)
    {
        //throw exception"e.g. client doesnt allow state to cancel order "
    }
    // End of issue ----------

    cancelOrderLines();

    setState(OrderState::Cancelled);
    update();
}

Factors such as level of abstraction I am still not sure I am doing. This issue I am having though is the issue code is should I prefer exception detail over a more neater story.

The change I am going back and forth about is to move this code to another method called something like, isInCancellableState() which would return a bool. So I could then call this from within the cancel function such as:

if (!isInCancellableState())
{
    throw exception e.g."order not in a cancellable state"
}

I like this because it is asking a nice neat question, and would make the cancel function (i think the same level of abstraction) and you can clearly see whats going on and read it. The issue though is that if that fails, because isInCancellableState is checking for more that one thing, it doesnt tell me anymore what it failed in the form of a nice exception message. It isn't too bad because is it was to fail, isInCancellableState is also quite neat and you could go down list checking data to see why it fails.

also if we were to use this original code, if i did want to just ask the question of isOrderCancellable, I dont have that so I would have to duplicate the code.

So this is my issue, when you have an action, but you have criteria for that action, is it fine to loose granularity of exceptions to change them into this format.

Whats the best solution. There might be another solution all together. Any help would be appreciated.

-1

Sorry if my C++ turns out to be a bit a rusty somewhere, but this might be a good case for letting that sort of information out (ie, the cancellation error reason) -- for your Order::cancel()'s knowledge while keeping the separation of concerns in place -- thanks to something along the lines of, eg:

void Order::cancel()
{
    // Check if order exists

    // Start of issue ----------
    Client client = getClient();

    CancellationException cancellationException = queryForCancellationException();
    if (cancellationException != NULL)
    {
        throw cancellationException;
    }

    cancelOrderLines();

    setState(OrderState::Cancelled);
    update();
}

CancellationException Order::queryForCancellationException()
{
    OrderState lastCancellableOrderState = client.getLastCancelOrderState();

    if (lastCancellableOrderState > OrderState::Released && getState() != OrderState::Pending)
    {
        return new CancellationException("order not pending and client doesnt allow");
    }

    if (getState() > lastCancellableOrderState)
    {
        return new CancellationException("client doesnt allow state to cancel order");
    }

    return NULL;
}

'Hope this helps.

  • Thank you for your comment. That is an interesting though but I want to avoid out/returning reference values, because they are counter-intuitive and does cause a reader to take a double look at this. This would make me look at this if I was to read it. Also the function does kind of do two things, checking criteria and setting value of exception. Its not preferable to return a bool as information and also exception as information. – Matster2 Mar 30 '18 at 16:37
  • @Matster2 Well, of course I won't claim that's the best solution, but only the simplest I could think of, considering your question as it stands. As for "also the function does kind of do two things, checking criteria and setting value of exception", a slight variant of my solution could be to simply return a CancellationException instead of bool, by adopting the convention that if queryIsInCancellableState returns something non-null, that's the exception to throw. I'm editing my answer accordingly. 'Hth – YSharp Mar 30 '18 at 16:44
  • I appreciate the help. C++ doesn't have null able types unless they are points, but I'm more looking for the best way to tackle this problem without being too programming specific. Its more how would you split it up to be most clean. I'm not sure returning exception helps. I want to ask questions of my code, such as is it... or can it be... I don't think exceptions should be thrown in question functions, as there not really anything wrong. They are fitting in the cancel method because they are critical to its operation. – Matster2 Mar 30 '18 at 16:54
  • I'm wondering is void checkCancellable is valid and clean with the throws? – Matster2 Mar 30 '18 at 16:55
  • I'm afraid I can't be of much more help for you about this. I can only add that, I usually prefer to throw exceptions at call sites which are the closest (in the call stack) to whatever point the code is making in its error case handling logic, ideally at the same level as the nominal case / happy path (which doesn't throw). This, as a general rule of thumb, helps with debugging and/or post mortem troubleshooting analysis (of the call stacks, precisely). In this instance of yours, here, it seems to me that should be in your cancel() method, not further down the call stack. Just imo. – YSharp Mar 30 '18 at 17:00

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.