Source updates + string bugfix

[dmadison]

This fixes the warnings + bugs related to strings being passed as char* instead of const char*. It also removes the underscore prefix for the API key, switches the stored client from a pointer to a reference, and standardizes the whitespace with tabs for indentation and spaces for alignment.

Nothing in here should change the public behavior or break the public API with the possible exception of the removed String version of the constructor. Although I'm fairly sure that isn't used since it crashes the ESP.

[witnessmenow]

The idea behind the String constructor is as you suspected to maintain backwards compatibility, but maybe it was not tested! I would like to keep it so if its causing a crash it should be fixed to work. Have raised an issue for it #35

Moving to const char makes sense to remove compiler warnings

What is the advantage of moving from a pointer to a reference? I'm not asking that combatively, I genuinely would like to know! The first library I wrote was based off someone else code that used the pointer and it worked fine so I never saw the need for changing it!

I need to provide a common solution for formatting, I should have clang format installed on my VCode instance now, but I might not have had it at the time of last working on this library

[dmadison]

What is the advantage of moving from a pointer to a reference?

There's nothing strictly 'wrong' about doing it one way or another, but references provide several small advantages over pointers (most of them meta).

For one a reference cannot be unbound (made null), so you don't have to perform nullptr checks to see if it's valid. References also can't be reassigned like pointers can, so for cases like this where you're assigning a value once and forgetting about it references are better.

And because you're not working with memory you don't have to dereference the symbol. You can treat the reference like the original symbol, which means no dereference operators (*) and no arrow notation (->). No memory manipulation also means arithmetic operations on the address itself won't work (for better or worse).

Rule of thumb as I learned it: use references when you can, pointers when you have to :thumbsup:. I hope that helps.

I need to provide a common solution for formatting

Let me know if you have a different style preference, if you're busy I'd be happy to make the changes myself.

[dmadison]

About the String constructor implementation (#35): there is no perfect solution to this.

The current version I just pushed (b7186ed) is simple but contains a fatal flaw: because it uses a pointer to the heap, if the user ever modifies the original String object the pointer is no longer valid and the program will crash. I don't know why someone would want to edit their API key on the fly, but the problem exists nonetheless.

As I see it there are four possible solutions:

1. Keep it as is

That is, using the pointer to the String's buffer on the heap: technically working but with a hidden fatal flaw. I'm not a fan of this solution, as it's possible for newcomers to unwittingly stumble into.

2. Remove the String constructor

This technically breaks backwards compatibility with the 2.0.0 release, although as no one has created an issue for the broken constructor in the last 5 months I don't think it will be missed. The advantage is that there is no persistent heap allocation, no reliance on the String class, and a minimal memory footprint. This gets my vote.

3. Store the API key in a String class member

That is, create a String member of the class and perform a copy when passing in the API key for either constructor. This would be transparent to the end user but adds a dependency to the String class, uses heap allocation, and in some cases will store the key in the program twice.

4. Store the API key in a character array on the heap

Similar to solution 3, this would copy the passed string into a character array stored on the heap, and then free the memory if/when the library object is destroyed. No dependency on the String class, but still uses heap allocation and may duplicate memory. This is also a bit redundant, as the new / copy / delete functions would be mirroring the functionality of the String class, and the String class is passed to the constructor anyways.

---

Personally I don't think the String constructor will be missed, which is why I chose to remove it rather than patch it. If you'd like to keep it for compatibility reasons, we either need to be mindful of the flaw in the current implementation or change how the key is stored within the class.

[dmadison]

I've pushed a few additional changes to improve the String handling. All Strings are now passed around as const-qualified references, which prevents making a temporary copy for each function and should make things more efficient. I've also added the missing String version of the sendGetToYoutube function, so all functions have both a C-style string and Arduino String version.

Lastly, I've changed the internal storage of the API key to use a String object (option 3 above). This forces the class to make a copy of the API key into heap memory which is inefficient but by far the safest way to keep the String version of the constructor.

[witnessmenow]

Thanks @dmadison , sorry about the delay in merging

[dmadison]

No worries, life happens :slightly_smiling_face: