swissqrbill icon indicating copy to clipboard operation
swissqrbill copied to clipboard

Class containing only the function needed to render the QR Bill

Open danielpanero opened this issue 1 year ago • 3 comments

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!

danielpanero avatar Aug 31 '22 09:08 danielpanero

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 😄

schoero avatar Aug 31 '22 16:08 schoero

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:

  • hands-off approach in regards to PDFKit, no more nodejs vs browser stream
  • even if the validation of the data fails, since the PDFDocument instance is detached, the stream and the PDF are still created
  • more customization, e.g: disabling the compression algorithm used by PDFKit, removing page buffering...
  • more optimizable, e.g: caching the QR code...
  • compatible with other PDFKit libraries

danielpanero avatar Aug 31 '22 22:08 danielpanero

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

schoero avatar Sep 01 '22 06:09 schoero