1
\$\begingroup\$

My goal is to create a URL query params from std::map<std::string, std::string>. For example, we call to_query_params( {{"one","1"},{"two","2"}} ) and we get ?one=1&two=2.

Is this a good way of doing it?

std::string to_query_params(std::map<std::string, std::string> map)
{
    if (map.empty()){
        return "";
    }
    auto map_beg = std::begin(map);
    auto concat = [](const decltype(map)::value_type& a){
                        return a.first+"="+a.second;
                    };
    std::string result("?"+concat(*map_beg));
    std::for_each(std::next(std::begin(map)), 
                  std::end(map), 
                  [&](decltype(map)::value_type param){ 
                      result+="&"+concat(param); 
                  });
    return result;
}
\$\endgroup\$
  • \$\begingroup\$ What kind of feedback are you interested in? Performance, readability, general feedback? \$\endgroup\$ – SuperBiasedMan Oct 1 '15 at 21:03
  • 1
    \$\begingroup\$ Performance, readability and maintenance. \$\endgroup\$ – jaor Oct 1 '15 at 21:04
2
\$\begingroup\$

You are not performing any percent encoding of parameter names or values, which would likely lead to breakage or even a security vulnerability if the you are given parameters that contain a special character.

A std::map might not be the right type; a std::multimap may be more appropriate.

\$\endgroup\$
1
\$\begingroup\$
  • Give your operators some breathing space:

    return a.first + "=" + a.second;
    
  • Concatenation involves constructing and deleting many temporary strings. std::stringstream avoids it:

    std::stringstream result;
    ...
    result << '&' << a.first << '=' << a.second;
    ...
    return result.str();
    
  • Pass map by reference.

\$\endgroup\$
1
\$\begingroup\$

There is no reason to use decltype

auto concat = [](const decltype(map)::value_type& a){
                    return a.first+"="+a.second;
                };

Why not use auto

auto concat = [](auto const& a){
                    return a.first+"="+a.second;
                };

Don't special case the '?' Just use '&' then change the one character after you have finished.

std::string to_query_params(std::map<std::string, std::string> map)
{
    if (map.empty()){
        return "";
    }
    std::string result;
    std::for_each(std::begin(map), 
                  std::end(map), 
                  [&](auto const& param){ 
                      result+="&"+concat(param); 
                  });
    // Just convert the first '&' into '?'
    result[0] = '?';
    return result;
}

The reason for this is that you use different techniques for both cases by only using one you make sure your code is DRY and consistent.

 // Version 1 parameter:
 const decltype(map)::value_type&

 // Version 2 parameter:
 decltype(map)::value_type          // different.

Also:

 // Vesion 1
 return a.first+"="+a.second;

 // Version 2:
 result+="&"+concat(param);    // concat or + "="

But rather than using a lambda we can use a range based for loop.

    std::string result;
    std::for(auto const& value: map) 
        result += "&" + concat(value); 
    );

I would probably use a std::map<std::string, std::vector<std::string>> that's because parameters in the URL allow multiple values and their order is significant.

\$\endgroup\$
  • \$\begingroup\$ "Why not use auto in lambda" - because it's c++11 :) \$\endgroup\$ – jaor Oct 2 '15 at 8:23

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.