Improve HttpHeaders using HashMap(enum, String) #1463
Conversation
…ay class * Lookups faster * Custom field names handled using CStringArray WHashMap.h * Add assert() statements to check bounds on operator[] index
| */ | ||
| HttpHeaderFieldName findCustomFieldName(const String& name) const; | ||
|
|
||
| CStringArray _customFieldNames; |
There was a problem hiding this comment.
Please, don't use underscores for private members or methods. Rename _customFieldNames to customFieldNames.
There was a problem hiding this comment.
OK, so are you saying that the coding style is not to use leading underscores for private member variables? If so, could you perhaps add that to the Coding Style Rules?
I mentioned this in #1431 under Class variables. Perhaps when you get time you could have another read through this and mark it up with some comments?
There was a problem hiding this comment.
(I also used underscores in the ElapseTimer class #1442)
There was a problem hiding this comment.
... that the coding style is not to use leading underscores for private member variables? If so, could you perhaps add that to the Coding Style Rules?
Right. A private member variable can be promoted to protected or public. If we use underscores then we should also make changes to the name after increasing the visibility. Which can be awkward.
If so, could you perhaps add that to the Coding Style Rules?
I mentioned this some time ago "Also do not add changes that prefix with underscore private attributes." (from here: #1431)". But you are right it has to be added to the coding style rules to be more clear and strict. I added that section: https://github.com/SmingHub/Sming/wiki/Coding-Style-Rules#naming. Feel free to improve it further, if needed.
There was a problem hiding this comment.
(I also used underscores in the ElapseTimer class #1442)
That slipped between the cracks. If this isn't a big problem for you can you please fix it and submit a PR? I don't remember if clang-format was able to detect this but I will try later today and see if that can be enforced with tools.
There was a problem hiding this comment.
Thanks for clearing that up.
For robust and readable code we do need in some way be able to easily identify scope from a variable name. One reason is to avoid the inevitable this->param = param in class methods, which is just asking for trouble. Up until recently I'd been using m_var for class and g_var for global scope. (I've also seen s_var for static scope.) The convention for Arduino seems to be _var so I switched to that. However, the practice does appear to conflict with the C++ standard, specifically that it's reserved for global scope. https://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier. So I agree, bad idea.
Looking for an authoritative source on the matter, I found this https://isocpp.org/wiki/faq/coding-standards, which recommends Sutter and Alexandrescu, C++ Coding Standards (edited by Stroustrup). Right at the start, under Don't sweat the small stuff, they do suggest the var_ convention for member variables.
I agree that retrospectively applying any agreed standard is probably a separate job, but new classes/modules should obviously hit the standard from the start. Might I suggest, therefore, that member variables use the var_ convention, and globals use _var ?
There was a problem hiding this comment.
The coding standards can be even more heated discussion than just standards (and this always comes to my mind https://xkcd.com/927/ ). @mikee47 Thanks a lot for your understanding and help ;-).
There was a problem hiding this comment.
@slaff LOL. Your point about promotion of member variables is interesting. It does highlight the confusion over why the convention(s) exist! Whilst digging around the web I found discussion about how Hungarian notation basically got abused - myself included! The Wikipedia article is an interesting read https://en.m.wikipedia.org/wiki/Hungarian_notation. I'm happy to go along with whichever coding standard is required to get the job done, but there's always room for improvement!
I think we're starting to get into the more interesting stuff in the PRs. Fun!
There was a problem hiding this comment.
IMHO some of the coding standards were introduced to address issues that are solved since quite some time.
For example nowadays with an IDE (and there are lots of good free/open-source IDEs) you can immediately see the type of a variable, its scope and visibility. Introspection advanced immensely. Then why the heck we should use some kind of Hungarian notation when we have that information already from the IDE?
Or using an underscore to mark a variable as private? That I guess came from Perl which ages ago did not have a proper way to declare a member variable as private. And the only way to hide it was to use underscore. Then PHP jump the wagon and soon others followed.
Or coding style rules used to alleviate shortcomings of the compilers at the time.
I think we're starting to get into the more interesting stuff in the PRs. Fun!
Yep, let's concentrate on the more important and interesting part. Coding standard discussions will never die similar to discussion between French and Italian cuisine lovers, etc. It is quite often a matter of taste.
* Pre-allocating String result size and using concat() methods avoid intermediate heap reallocations * Missed usage of headers in HttpRequest::toString()
* Rename CStringArray::append() to add() and optimse. * Add count() method
…1463 * Comparison accomplished using stack, avoids allocating temporary String variable on heap
Revision of HttpHeaders to use enum instead of String for key parameter.