Closed ChrisBerna closed 4 years ago
Thanks for the PR. I will look into it.
I've had a first look at it. It's quite comprehensive and of good quality. There are a few aspect however that probably need to be changed:
Bill.BillInformation
to Bill.BillInformationText
makes sense except it breaks backward compatibility. At the moment I'm not willing to do that.Dictionary<double, double>
(in BillInformation
) is not the best choice. I don't think it will ever be access by key. So a list would be a better choice. And decimal
should be preferred over double
for financial amounts as they can introduce funny effects (such as 2.499999 instead of 2.5).QRCodeText.Decode
as a different exception is thrown for the bill information field than for all other fields.//S1/
. All other errors are ignored.Is it ok if I take it over for the next steps?
Is it ok if your code is put under MIT license? I would still like to name you as the author(s). With what name would you like to be mentioned?
Hello, thanks for your feedback and yes please feel free to improve or change parts of my implementation. The license MIT is perfectly fine and thank you very much for putting me as Author (Christian Bernasconi).
I've taken your code and made the modifications to address the above issues:
Bill.RetrieveSwicoBillInformation
.Bill.BillInformation
remains a string property. Structured bill information can be set with SetSwicoBillInformation
.Regarding item 3: After having changed your code quite considerably, I'm no longer sure it's a good approach. Depending on the use case, it might be more beneficial to decode as much as possible instead of stopping at the first problem. What do you think?
The code is in a separate branch (and not in your PR): https://github.com/manuelbl/SwissQRBill.NET/tree/swico-bill-info
I had a quick look at your branch, well done thank you very much! I find a good idea to decode the bill information only on demand and regarding point 3 in my opinion would be better to decode as much as possible instead of throwing an exception.
So the decoding approach is different now: no exceptions are thrown at all. Errors are silently ignored.
I'd appreciate it if you could have a look at the API (mainly the new methods in Bill
, SwicoBillInformation
and the decoding approach) and give me feedback if it's understandable, makes sense and is useful in typical ERP/accounting project and your project (whatever it is).
Perfect, thank you! I checked the branch and in my opinion the code is very well done, the only little suggestion would be to move the constants like InvoiceNumberTag
in a separated class so that they can also be used in the SwicoS1Encoder
class.
Thanks for your contribution. I've now released it as version 2.3.
This solves issue #8
As promised with this PR the raw text containing the QR billing information is decoded in a structured data model based on the syntax definition of Swico. I added the new property in the
Bill
class which will be filled only in theDecode
method ofQRCodeText
.