-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check ExecuteRequest before CreateRequest #2
Comments
I think it's just a naming issue I have here. ExecuteRequest receives the object from 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. Then CreateRequest could be renamed with CreateObject, you wouldn't have to do much about it. What do you think? Glad the project is being used :) |
I can definitely understand what you were going for with Hypothetically speaking, if it were to change, it would nearly accomplish the same thing I'm envisioning if it were only constrained to As you've said, I agree that it's not very clear to return null to stop processing. I like the idea of the Another approach I'm mulling over is to return a context object from 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 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. 👍 |
I do like the last solution you proposed. If you have time you can send a pull request with some unit tests. 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. |
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 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. |
I will close this. No action taken for several years. |
I wonder if it would make more sense to check
ExecuteRequest
beforeCreateRequest
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, callExecuteRequest
first to check if any customers need to be added (via querying the database or something); if so, return true. If true, then callCreateRequest
to create the request. If the result is not null (it really shouldn't be sinceExecuteRequest
is true), then callqbXmlRequest.GetRequest
. This essentially just reverses the order ofCreateRequest
andExecuteRequest
, but you gain the ability to check before forming the request for the Web Connector.The text was updated successfully, but these errors were encountered: