Comments (6)
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 :)
from quickbooks-sync.
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. 👍
from quickbooks-sync.
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.
from quickbooks-sync.
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.
from quickbooks-sync.
from quickbooks-sync.
I will close this. No action taken for several years.
from quickbooks-sync.
Related Issues (20)
- InvoiceAdd.InvoiceLineAdd items being reordered HOT 4
- Parsing amounts incorrectly from string to decimal HOT 18
- SoapCore 0.9.9 is not compatible with .NET core 3.1 HOT 3
- How can i Iterator for JournalEntryAddRqType. HOT 3
- Tracking iterator progress HOT 4
- Common Interfaces HOT 11
- Unexpected keys in EmployeePayrollInfo causing issues deserializing HOT 5
- Support QBXml 14. HOT 11
- Add support for Nullable=enable
- Upgrade to QbSync 2.3.0 causes MissingMethodException HOT 8
- Payment receipt template returns `true` for template type HOT 4
- decimal precision in QUANTYPE HOT 6
- QuickBooks Sync does not work with .NET 7 HOT 9
- Unable to add a new Application to the Web Connector in 3.1.0 HOT 21
- SONumber HOT 6
- XmlCodeExporter Missing HOT 1
- Authenticator Null Reference HOT 5
- UnitOfMeasureUnit is missing from ColDesc in reports.
- Upgrade from old version HOT 2
- Rate or RatePercent not being set on SalesOrderLineRet
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from quickbooks-sync.