# Parsing httpline

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 ";

• 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). – edmz Apr 17 '14 at 9:30

## 2 Answers

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"];


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.
• 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[]). – utnapistim Apr 17 '14 at 10:38
• @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 :) – Morwenn Apr 17 '14 at 11:51