Check ExecuteRequest before CreateRequest

[mscappini]

I wonder if it would make more sense to check ExecuteRequest before CreateRequest is called. This way, we can confirm that a request should even bother being made before a request object is formed.

For example: create a CustomerAdd step, call ExecuteRequest first to check if any customers need to be added (via querying the database or something); if so, return true. If true, then call CreateRequest to create the request. If the result is not null (it really shouldn't be since ExecuteRequest is true), then call qbXmlRequest.GetRequest. This essentially just reverses the order of CreateRequest and ExecuteRequest, but you gain the ability to check before forming the request for the Web Connector.

[jsgoupil]

I think it's just a naming issue I have here. ExecuteRequest receives the object from CreateRequest. The flow you want can be achieved by returning null from the CreateRequest.

I tried to have a mirror logic of ExecuteRequest/ExecuteResponse. Where you have the object you want to manipulate right in that method.

However, I do agree there is a little design problem in my current implementation where sometimes I do a check in the "CreateRequest" and sometimes I do a check in the "ExecuteRequest". Right now, those two methods can short circuit the step which is quite confusing.

I think we could come up with a 3rd method which would only check if we have to execute the step. bool ExecuteStep(). This would be run first.

Then CreateRequest could be renamed with CreateObject, you wouldn't have to do much about it. And ExecuteRequest/ExecuteResponse; maybe the word Execute is wrong, but as I said, I tried to keep them "in sync".

What do you think?

Glad the project is being used :)

[mscappini]

I can definitely understand what you were going for with CreateRequest. The only issue I'm running into with that is that it must return T (constrained by QbRequest).

Hypothetically speaking, if it were to change, it would nearly accomplish the same thing I'm envisioning if it were only constrained to class. This way, one could return an object, and if it were not null, pass that object as a state to CreateRequest to transform those results into QbRequest T. On the other hand, though, I agree that the methods should sort of mirror each other as you have mentioned. That seems like good design. To clarify, I don't think changing the constraint is the correct approach--just using that as example to illustrate my vision.

As you've said, I agree that it's not very clear to return null to stop processing. I like the idea of the ExecuteStep better for that. (Although I might consider the name to be something more like ShouldExecute vel sim.). And potentially being able to pass results from the ExecuteStep method back into the CreateRequest so the results that were used to determine that we should execute the step do not have to be fetched again, and subsequently can be transformed into a QbRequest. But I'm not entirely sold on that approach because I imagine the step itself could retain the state between method calls (a la class field/property). (Though I could see potential problems with that if the state were to somehow become invalid. But I suppose the state could be reset with each ExecuteStep invocation, thus validating the state before a CreateRequest is returned.)

Another approach I'm mulling over is to return a context object from ExecuteStep that contains a bool deciding if it should execute or not, and a state object to pass back into CreateRequest.

class StepContext
{
    public bool ShouldExecute { get; set; }
    public object State { get; set; }
}

var context = ExecuteStep(authenticatedTicket);
if (context != null && context.ShouldExecute)
{
    var requestObject = CreateRequest(authenticatedTicket, context);
    if (requestObject != null)
    {
        var qbXmlRequest = new QbXmlRequest();
        qbXmlRequest.AddToSingle(requestObject);
        return qbXmlRequest.GetRequest();
    }
}

Though, no ExecuteRequest here. But maybe not required. Your thoughts?

Great project, by the way. Excellent design. I'm very grateful it exists. I wish I could should you how the code I am working with has reduced and how much cleaner it looks now over how it used to look. :+1:

[jsgoupil]

I do like the last solution you proposed. If you have time you can send a pull request with some unit tests. I am a bit busy with current projects involving QuickBooks indeed...

Since I launched this project, there are so many quirks that I wish I should have known before. QuickBooks is always surprising me when it returns error, and I didn't handled them properly with this current design.

I don't think it needs to have CreateRequest AND ExecuteRequest... I think only one that is overridable should be good enough like you propose.

[mscappini]

I'll try to get around to it soon as I can. I'm also doing some Web Connector work and it's keeping me quite busy.

Related to your comment about error handling: yes, I ended up inheriting from StepQueryResponseBase<T, Y> with my own response base and implementing something like this (the word My in the name is merely for illustration purposes):

protected override sealed void ExecuteResponse(AuthenticatedTicket authenticatedTicket, Y response)
{
    this.Ticket = authenticatedTicket as MyAuthenticatedTicket;

    if (response != null && response.statusSeverity == "Error")
    {
        Logger.Error(response.statusMessage ?? "Unknown error");
    }

    if (response.statusCode == "0")
    {
        ExecuteMyResponse(response);
    }
}

protected virtual void ExecuteMyResponse(Y response)
{
    base.ExecuteResponse(this.Ticket, response);
}

As far as error handling goes with exceptions, I have another issue open about it with some planned changes. I've seen other services also throw exceptions when they receive a response that is an error. Is that something you'd also like to consider? We can open another issue for that.

[jsgoupil]

https://github.com/jsgoupil/quickbooks-sync/issues/4

[jsgoupil]

I will close this. No action taken for several years.