schoero / swissqrbill

Swiss QR Bill generation in Node.js and browsers
MIT License
155 stars 29 forks source link

Class containing only the function needed to render the QR Bill #385

Closed danielpanero closed 1 year ago

danielpanero commented 1 year ago

Hi, first of all, thank you very much for your awesome work.

I was wondering if it would possible to externalize the QR rendering and validation part from all the other functions, such as addTable... and also not directly extend this new class from PDFKit.

This would allow more customization and ease of use for more advanced users as every time I want to add a QR Bill to a PDF, I have to modify the class to make it work with all my other functions. Furthermore, at least now, it's quite cumbersome to change the data, to render multiple bills...

So what I would propose is to divide PDF_ into two classes:

  1. One class for more advanced users, which does not extend any other class, containing only the methods needed to render the bill, such as addQRBill, _render... and furthermore as it doesn't extend PDFKitDocument, for every function we would pass along as parameter the PDFKitDocument
  2. A second class, simply extending the first and ExtendedPDF which mimics the original PDF_

Let me know, what you think and I can start working on it!

schoero commented 1 year ago

every time I want to add a QR Bill to a PDF, I have to modify the class to make it work with all my other functions.

Can you clarify that?

A second class, simply extending the first and ExtendedPDF which mimics the original PDF_

You can't extend multiple classes at the same time, only through inheritance. You could use mixing but if you pass PDFKit.PDFDocument as a parameter, that does not need to be a class.

If I understand you correctly, the goal of this would be that you can create one PDF Document and be able to attach different payment parts to it (?).

For that to work, we would only need to extract the addQRBill, _render and their helper methods into their own functions outside the class. And for the PDF_, we could simply pass the current instance (this) as the parameter and it should work.

I am imagine something like this:

// extracted function
export function attachQRBill(pdfDocument: PDFKit.PDFDocument, posX: number, posY: number, data: Data){
 // Code to render payment part
  attachQRCode(pdfDocument, ...);
  attachRectangle(pdfDocument, ...);
}

// helper functions
function attachRectangle(pdfDocument: PDFKit.PDFDocument, posX: number, posY: number, width: number, height: number){
  ...
}

function attachQRCode(pdfDocument: PDFKit.PDFDocument, posX: number, posY: number, data: Data){
  ...
}

// PDF_
export class PDF_ extends ExtendedPDF {

  addQRBill(...){
    attachQRBill(this, ...);
  }

}

I don't like that this would introduce a lot of impure functions with sideffects but I think it is better than mixins. Maybe you have a better idea. Or I completely misunderstood what you are up to 😄

danielpanero commented 1 year ago

Yes, my bad I could have explained it better... Although I would prefer using a more functional approach as you have described, we could still rely on classes:

class Bill {
  public size: Size = "A6/5";
  protected _data: Data;
  private _scissors: boolean = true;
  private _separate: boolean = false;
  private _outlines: boolean = true;
  private _language: Languages = "DE";

 constructor(data: Data, options?: PDFOptions) {...}

  public addQRBill(pdfDocument: PDFKit.PDFDocument, size: Size = "A6/5"): void {}

  private _render(pdfDocument: PDFKit.PDFDocument): void {...}
  private _renderQRCode(PDFKit.PDFDocument): void {....}
  private _formatAddress(data: Debtor | Creditor): string {...}
  private _addRectangle(PDFKit.PDFDocument, x: number, y: number, width: number, height: number): void {...}
}

class PDF_ extends ExtendedPDF {
 constructor(data: Data, options?: PDFOptions) {
    super({ autoFirstPage: false, bufferPages: true });
    this._bill = new Bill(data, options)
}

public addPage(options?: PDFKit.PDFDocumentOptions): PDFKit.PDFDocument {...}
public end(): void {...}
public addQRBill(size: Size = "A6/5"): void {
   this._bill.addQRBill(this, ...)
}

}

This would massively simplify the problem of rendering more QR bills:

schoero commented 1 year ago

You are right, we don't need to extend both classes. So yes, go ahead and implement this. I like such improvements.