aruZeta / QRgen

A QR code generation library.
https://aruzeta.github.io/QRgen/
MIT License
103 stars 8 forks source link

Organize the code better #4

Closed aruZeta closed 2 years ago

aruZeta commented 2 years ago

There are some issues with the actual code organization, mainly about the usage of templates, for example in EncodedQRCode.nim, where there are some templates used to write myEncodedQRCode[] which maybe should be just written as myEncodedQRCode.data[], and this implies that the encodedData field should be renamed to data.

Why though?

Well, I think this actually adds more complexity than it helps to understand the code, it may reduce the amount of code written, but at the expense of readability.

Then, why having these [] templates for BitArray?

Because a BitArray, as said in the docs (haven't pushed those atm) should be treated as a regular sequence but with the difference that it has some specialized procedures, those being both add and nextByte.

Also unsafeAdd and unsafeDelete should be moved to BitArray.nim, maybe some of the fields of objects like BitArray or Drawing should be private to their module, and only accessible via getter procedures if actually needed.

aruZeta commented 2 years ago

As can be seen in the previous commit, the decided approach to follow is to encapsulate the BitArray implementation, and the same may happen with Drawing. Also removed almost all templates regarding accessing its fields with procedures, like unsafeAdd or [] operators.

aruZeta commented 2 years ago

Also, to expand on these changes, I haven't noticed any change on performance:

Using flags -d:benchmark --mm:orc -d:lto:

Before the commit: Image

After the commit: Image

aruZeta commented 2 years ago

After the last commit these are the results of the benchmark: Image

Some noticeable 5ms (another 1ms was added, strangely, by 05033b5).

aruZeta commented 2 years ago

I have decided that I'm going to remove the encapsulations made to both BitArray and EncodedQRCode.

Why?

Well, I just remembered what I most hate about encapsulation: boilerplate and a huge amount of calls to do stuff. Maybe encapsulation is good to use when providing objects to a user, but in the actual code this is not done (well, you can touch the fields of the DrawedQRCode given from the main API, but that may be changed later), so there is actually no need to have all these procedures for the sake of "controlling the setting and getting of values".

aruZeta commented 2 years ago

Encapsulation bad! Image

aruZeta commented 2 years ago

The issues mentioned about templating has been removed and they are now located in their respective module and the point of encapsulating been bad has been proved, hence I'm closing this issue.