Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Organize the code better #4

Closed
aruZeta opened this issue Aug 29, 2022 · 6 comments
Closed

Organize the code better #4

aruZeta opened this issue Aug 29, 2022 · 6 comments
Labels
roadmap This ISSUE or PR is part of the roadmap of the project.

Comments

@aruZeta
Copy link
Owner

aruZeta commented Aug 29, 2022

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 added a commit that referenced this issue Aug 29, 2022
This makes the BitArray implementation encapsulated, and all
operations over it are made via procedures.

Issue: #4
@aruZeta
Copy link
Owner Author

aruZeta commented Aug 29, 2022

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
Copy link
Owner Author

aruZeta commented Aug 29, 2022

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 added a commit that referenced this issue Aug 29, 2022
@aruZeta
Copy link
Owner Author

aruZeta commented Aug 29, 2022

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

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

@aruZeta
Copy link
Owner Author

aruZeta commented Aug 29, 2022

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
Copy link
Owner Author

aruZeta commented Aug 29, 2022

Encapsulation bad!
Image

@aruZeta
Copy link
Owner Author

aruZeta commented Aug 29, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
roadmap This ISSUE or PR is part of the roadmap of the project.
Projects
No open projects
Status: DONE
Development

No branches or pull requests

1 participant