Skip to content

Functional reformat #7

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

Closed
wants to merge 2 commits into from
Closed

Functional reformat #7

wants to merge 2 commits into from

Conversation

NoeMurr
Copy link

@NoeMurr NoeMurr commented Apr 22, 2022

I used this library in one of my projects and I realized that there are two main problems:

  • If the file is included in more than 1 file you're going to get an error because all the implementations are done in the .hpp file.
  • the usage of unsigned char is a problem when the user is trying to input a string in the functions and more important the non usage of the const char * don't allow the user to input literal strings to the functions.

Addressing the above problems I made the following changes:

  1. Using normal char instead of unsigned char for character of a base64
    encoded value. They are ascii characters and so they should be char.
  2. Using uint8_t instead of unsigend char when dealing with bytes. It's
    better to use such kind of standards because they are guaranteed to be 8 bit.
  3. Using size_t or ssize_tinstead of unsigned int when dealing with
    lengths.It's better because it'll be the same used in string libraries when
    building across different architectures (e.g. Arduino).
  4. Using doxygen standard tags in comments for documentation of the functions.
  5. Dividing implementations from declaration using a cpp file. This prevents the
    error of redefinition of functions if the library is used in different files
    in a project.
    N.B. for one line functions I'm using the inline keyword
  6. Using const char * instead of unsigend char[] when dealing with strings.
    that is important to allow the user to pass a literal string to the function.
  7. Using default parameters value in functions when possible.
  8. Added tests with the platformio unit testing environment
  9. Added library.json and platformio.ini files for platformio.

NoeMurr added 2 commits April 22, 2022 21:31
implementation separated in cpp file  and
added tests
@Densaugeo
Copy link
Owner

Apologies for the late reply, but I have been quite busy lately.

It's been a while since I've done a lot of C++, so I'm not up to date on everything. I'll take a look at your advice when I get a chance, though it may not be for a while.

One thing that stands out to me: have you seen issues with multiple inclusion of this library, and if so using what compiler? In all the compilers I've used, include guards were enough to prevent that.

@Densaugeo
Copy link
Owner

There've been a few changes since this was sent, and some of these changes were already made while others are conflicted, so I'm closing this.

If you still want to make some of the other changes here, please break them into individual PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants