Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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;
}
share|improve this question
    
What kind of feedback are you interested in? Performance, readability, general feedback? – SuperBiasedMan Oct 1 '15 at 21:03
1  
Performance, readability and maintenance. – jaor Oct 1 '15 at 21:04

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.

share|improve this answer
  • 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.

share|improve this answer

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.

share|improve this answer
    
"Why not use auto in lambda" - because it's c++11 :) – jaor Oct 2 '15 at 8:23

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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