-
Notifications
You must be signed in to change notification settings - Fork 33
Description
Noticed what appear to be numerous issues with the WiFiAlert class:
-
getString() returns a pointer to a locally scoped char array. Would love to change this method’s signature to allow for options other than allocating a char array on the heap to solve this, but...
-
Even helper methods (e.g., getString()) are declared public, preventing us from changing their signature without potential impact to clients. We should always opt for the tightest access unless there is an explicit reason not to do so. Won’t be able to fix this.
-
The getString() method calls strcpy_P() to copy to a fixed size char array. If this target array isn’t big enough to hold the source string, it will write beyond the length of the char array, corrupting data.
-
The class has a member variable AlertMsg, which is a char pointer. This should likely be a fixed length char array to prevent having to dynamically allocate the array in the heap to help avoid heap fragmentation. (I realize the class isn’t actually dynamically allocating a char array, but more on that below.)
-
The Send() method just copies the pointer of the passed-in char array to AlertMsg. This may be fine if the message happens to be a const. But if someone sends in a locally scoped char array, this won’t work. Better to copy the message to the class.
-
This is more of a question. Seems like the first calls to Send() should default to force an immediate alert. Subsequent alerts would then be sent on the delay. Wouldn’t the user prefer an immediate message at first?
Please let me know if I’m misunderstanding anything here. I should be submitting a pull request to address these issues in the near future.