gilmaimon / ArduinoWebsockets

A library for writing modern websockets applications with Arduino (ESP8266 and ESP32)
GNU General Public License v3.0
482 stars 97 forks source link

Using default headers if user has not declared them. #74

Closed zastrixarundell closed 4 years ago

zastrixarundell commented 4 years ago

Based on issue #69 (nice).

Pull request where the user can write headers before the connection. If the user does not write some required headers like Origin it will use the default hard-coded headers, if the user does write them they they are used rather than the hard-coded ones.

This is only implemented for the following headers:

Has been tested on an ESP8266. The server for websockets was an Elixir Phoenix one but it doesn't really matter what is the websocket server language.

gilmaimon commented 4 years ago

Hi! Thanks for helping out with this issue. The code looks good, there are few points I'd like you to change:

  1. I prefer that every if has a {...} block after it (instead of one-liner ifs)
  2. Arguments should be const and passed-by-ref when possible, please fix keywordDoesNotExist according to this rule.
  3. Is there really a need for the duplication in usedKeys? Can you modify the code such that keywordDoesNotExist will look in customHeaders to check if the key exists?
  4. keywordDoesNotExist is a cool name, but how about shouldAddDefaultHeader?

Thank you again, awesome job!

zastrixarundell commented 4 years ago

Yeah it won't be an issue at all, I'll update it and push it in a min.

zastrixarundell commented 4 years ago

Oh and I'm cool with everything except the 4th one because IMHO that name is counter-intuitive when used with the 3rd (task?):

            if (keywordDoesNotExist(usedKeys, header.first)) {
                usedKeys.push_back(header.first);
            }

VS

            if (shouldAddDefaultHeader(usedKeys, header.first)) {
                usedKeys.push_back(header.first);
            }

I mean you just say if you want to keep it I'll keep it and if not I'll change it, no problem.

gilmaimon commented 4 years ago

Because usedKeys is not actually needed, I imagine the code could look like this:

if (shouldAddDefaultHeader("Origin", customHeaders)) {
    // add default Origin
}

You could give it another fitting name, but the important point is that usedKeys is not needed in my opinion because that info is already in customHeaders.

zastrixarundell commented 4 years ago

Ah I see now what you meant. I'll update the code.

zastrixarundell commented 4 years ago

Not sure whether I have to directly say but: The deed has been done. Tested on 2 different ESP8266's and worked correctly.

gilmaimon commented 4 years ago

Looks good! I assume you tested this and it is working fine. Merged :)

Thank you!