5
\$\begingroup\$

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

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

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"];
\$\endgroup\$
4
\$\begingroup\$

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.