Parsing httpline

Question

I have an HTTP server class (.hpp & cpp). I am trying to improve reserve data from the socket class because I have s.getline() to get HTTP call. My s.RecvData() gets data line by line in an infinite loop to build HTTP struct.

This is my old code:

while (1) {
 line = s.RecvData();
 //httpline = s.Getline();
 if (line.empty()) break;
 //find location of tab"\t"
 int location_tab_char = line.find_first_of(" ");
 int loacation_End_chars = line.find_first_of("\r\n");
 if (loacation_End_chars == 0)
 {
 break;
 }
 if ("Host:" == line.substr(0, location_tab_char))
 {
 req.hostName_ = line.substr(0, loacation_End_chars);
 }
 else if ("Connection:" == line.substr(0, location_tab_char))
 {
 req.conn_ = line.substr(0, loacation_End_chars);
 }
 else if ("Accept:" == line.substr(0, location_tab_char))
 {
 req.accept_ = line.substr(0, loacation_End_chars);
 }
 else if ("Accept-Language:" == line.substr(0, location_tab_char))
 {
 req.acceptLanguage_ = line.substr(0, loacation_End_chars);
 }
 else if ("Accept-Encoding:" == line.substr(0, location_tab_char))
 {
 req.acceptEncoding_ = line.substr(0, loacation_End_chars);
 }
 else if ("User-Agent:" == line.substr(0, location_tab_char))
 {
 req.userAgent_ = line.substr(0, loacation_End_chars);
 }
}

I just need a way to improve it because getting data from the socket line by line is too much calling. Also, when I pass to struct, I pass this string "Host: 169.254.80.80:8080"

"Host: 169.254.80.80:8080\r\nConnection: keep-alive\r\nAccept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,/;q=0.8\r\nUser-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36\r\nAccept-Encoding: gzip,deflate,sdch\r\n Accept-Language: en-US,en;q=0.8,ar;q=0.6 ";

• edmz: You should go for regular expressions to parse the header. You can check out other HTTP server's source code to see how they implement that. (with regex, I guess).

Answer 1

Your code can be made smaller and without repeated constructs, by mapping your keys to values in a map:

// #include <map>
std::map<std::string,std::string> headers;
while(true) { // better use true instead of 1
 line = s.RecvData();
 if (line.empty() || line == "\r\n\r\n")
 break;
 auto key_value_sep = line.find(":"); // do not split by space
 // you could stop here in case of an exception in the data
 // if(0 == key_value_sep || std::string::npos == key_value_sep)
 // throw std::runtime_error{"..."}; // or break, or whatever
 // line is a single line; as such, end chars location not needed
 // int loacation_End_chars = line.find_first_of("\r\n");
 auto key = line.substr(0, key_value_separator);
 auto value = line.substr(key_value_separator + 2, // skip ": "
 line.size() - key.size() - 4); // size less key,
 // separator and
 // "\r\n"
 // you could break here in case of an exception in the data
 // if (value.empty() || key.empty()) || ...)
 // throw std::runtime_error{"..."}; // or break, or whatever
 // set header 
 headers[key] += value;
}
// set headers (or use directly from map)
req.hostName_ = headers["Host"];
req.conn_ = headers["Connection"];
req.accept_ = headers["Accept"];
req.acceptLanguage_ = headers["Accept-Language"];
req.acceptEncoding_ = headers["Accept-Encoding"];
req.userAgent_ = headers["User-Agent"];

Answer 2

First of all, a few notes about style:

• You probably have a typo in loacation_End_chars. I think that you meant location_End_chars instead.
• Also, you should be consistent with location_tab_char and use snake_case everywhere without any capital. Therefore, loacation_End_chars should actually be named location_end_chars.

---

Your program may compute line.substr(0, location_tab_char) an awful lot of time if it has to "User-Agent:", which may be extremely inefficient. It is bad generally bad to repeat code when it could be avoided. Here, a solution would be to compute line.substr(0, location_tab_char) and line.substr(0, location_end_char) only once before your conditions:

std::string first = line.substr(0, location_tab_char);
std::string second = line.substr(0, location_end_chars);
if ("Host:" == first)
{
 req.hostName_ = second;
}
else if ("Connection:" == first)
{
 req.conn_ = second;
}
// etc...

Unfortunately, I have no idea what would be good names for these variables, so I named them first and second, but you should find a relevant and meaninful name for them instead.

---

Another way to simplify your code would be to map directly the strings to the fields of your struct thanks to a std::reference_wrapper:

std::map<std::string, std::reference_wrapper<std::string>> mapping = {
 { "Host:", req.hostName_ },
 { "Connection:", req.conn_ },
 { "Accept:", req.accept_ },
 { "Accept-Language:", req.acceptLanguage_ },
 { "Accept-Encoding:", req.acceptEncoding_ },
 { "User-Agent:", req.userAgent_ }, 
};

Then your code would become:

while (true) {
 line = s.RecvData();
 //httpline = s.Getline();
 if (line.empty()) break;
 //find location of tab"\t"
 int location_tab_char = line.find_first_of(" ");
 int location_end_chars = line.find_first_of("\r\n");
 if (location_end_chars == 0)
 {
 break;
 }
 auto key = line.substr(0, location_tab_char);
 auto value = line.substr(0, location_end_chars);
 auto it = mapping.find(key);
 if (it != mapping.end())
 {
 it->second.get() = value;
 }
}

Some additional notes (thanks to @utnapistim comment):

• std::map::operator[] invokes the default constructor of the value type. However, std::reference_wrapper does not have a default constructor, that's why I used the method at instead (contrary to operator[], at cannot create an element, that's why we need to check for the existence of the element beforehand with find).
• Of course, you header instance needs to live in a greater scope than the std::map instance, otherwise, trying to access the elements of mapping will trigger undefined behavior.

• utnapistim: A note about the std::reference_wrapper use: make sure the header you are mapping has a bigger scope than the map (otherwise you end up with UB). Also, the use of std::reference_wrapper in a map imposes that you always check for existence of the value before assignment (as you did - which is non-obvious unless you know the behavior of std::map::operator[]).
• Morwenn: @utnapistim Yeah, std::map::operator[] always crashes with std::reference_wrapper since it does not have a default constructor. I will add a note about that, thanks :)