This query string parser is intended to parse the parameters and values of a parser query string. I'm particularly concerned about my use of std::move(...).I haven't used C++ almost 15 years and have never done so professionally, so any and all feedback is welcome.HttpUtility.h:class UrlUtil final{public:    static std::string urlDecode(const std::string &urlEncodedString);};class QueryParameter final{public:    const std::string &name() const { return name_; }    const std::string &firstValue() const;    //Question 1: because this is a const&, this is not bad practice, correct?    const std::vector<std::string> &allValues() const { return values_; }    int valueCount() const { return values_.size(); }    QueryParameter(const std::string name) : name_(std::move(name)) {   }private:    friend class QueryString;    void addValue(std::string value) { values_.push_back(value);    }    std::string name_;    std::vector<std::string> values_;};class QueryString final{public:    QueryString(std::istream &input);    int numParams() const { return parameters_.size(); }    bool hasParam(const char *name) const { return parameters_.find(std::string(name)) != parameters_.end(); }    std::vector<std::string> getParamNames() const;    //Question 2: Instead of returning a vector<string>, would something like this be better?    //void fillParamNames(const std::vector<std::string> into) const;    const QueryParameter *getParam(const std::string &name) const;    const QueryParameter *getParam(const char* name) const { return getParam(std::string(name)); };private:    static std::string parseName(std::istream &input);    static std::string parseValue(std::istream &input);    std::map<std::string, std::unique_ptr<QueryParameter>> parameters_;};HttpUtility.cpp:#include HttpUtility.h#include <sstream>/* * UrlUtil Methods *///Note: static methodstd::string UrlUtil::urlDecode(const std::string &urlEncodedString){    std::stringstream input(urlEncodedString);    std::string decodedString = ;    while(!input.eof())    {        int c = input.get();        if(c < 0 || c == '&')            break;        if(c == '+')        {            decodedString.push_back(' ');        }        else if(c == '%')        {            char octalCode[3];            octalCode[0] = (char)input.get();            octalCode[1] = (char)input.get();            octalCode[2] = 0;            int ch = std::stoi(octalCode, 0, 16);            decodedString.push_back(ch);        }        else            decodedString.push_back(c);    }    return std::move(decodedString);}/* * QueryParameter Methods */const std::string &QueryParameter::firstValue() const{    if(valueCount() == 0)        //Question 3: I am really not sure of the appropriate exception to throw here...        throw std::runtime_error(Attempted to obtain first value of query string parameter that does not have any values);    return values_[0];}/* * QueryString Methods */QueryString::QueryString(std::istream &input){    //Example:  param1=value1&param2=value2    while (!input.eof() && input.peek() > 0)    {        //Should eat param1=        auto name = parseName(input);        //Should eat value1&        std::string value = parseValue(input);        auto foundItr = parameters_.find(name);        if(foundItr == parameters_.end())        {            auto newParam = std::make_unique<QueryParameter>(name);            if(value.size() > 0)            {                newParam->addValue(value);            }            parameters_.emplace(name, std::move(newParam)).first;        }        else            (*foundItr).second->addValue(value);    }}std::vector<std::string> QueryString::getParamNames() const{    std::vector<std::string> names;    for(auto const& pair : parameters_)        names.push_back(pair.second->name());    return std::move(names);}const QueryParameter *QueryString::getParam(const std::string &name) const{    auto itr = parameters_.find(name);    return itr == parameters_.end() ? nullptr : (*itr).second.get();};//Note: static methodstd::string QueryString::parseName(std::istream &input){    std::string name = ;    while(!input.eof() && input.peek()!= '=')        name.push_back(input.get());    //Eat the '='    if(!input.eof())        input.get();    return name;}//Note: static methodstd::string QueryString::parseValue(std::istream &input){    std::string urlEncodedValue;    int c = input.get();    while(c > 0 && c != '&')    {        urlEncodedValue.push_back(c);        c = input.get();    }    if(urlEncodedValue.size() == 0)        return ;    std::string decodedValue = std::move(UrlUtil::urlDecode(urlEncodedValue));    return std::move(decodedValue);}tests.cpp:#define BOOST_TEST_MODULE cgi-test-tests#define BOOST_TEST_DYN_LINK#include <string>#include <sstream>#include <boost/test/unit_test.hpp>#include HttpUtility.husing namespace std;BOOST_AUTO_TEST_CASE(UrlUtil_urlDecode_works){    string decoded = UrlUtil::urlDecode(testing+space);    BOOST_CHECK_EQUAL(decoded, testing space);    decoded = UrlUtil::urlDecode(!%40%23%24%25%5E%26*()_%2B);    BOOST_CHECK_EQUAL(decoded, !@#$%^&*()_+);}BOOST_AUTO_TEST_CASE(QueryString_can_parse_query_string_with_no_value_ending_with_eof){    stringstream stream(name1=);    QueryString qs(stream);    BOOST_CHECK(qs.numParams() == 1);    BOOST_CHECK_EQUAL(qs.hasParam(name1), true);    auto p = qs.getParam(name1);    BOOST_CHECK_NE(p, (QueryParameter*)nullptr);    BOOST_CHECK_EQUAL(p->name(), name1);    BOOST_CHECK_EQUAL(p->valueCount(), 0);}BOOST_AUTO_TEST_CASE(QueryString_can_parse_query_string_with_no_value_ending_with_amp){    stringstream stream(name1=&);    QueryString qs(stream);    BOOST_CHECK(qs.numParams() == 1);    BOOST_CHECK_EQUAL(qs.hasParam(name1), true);    auto p = qs.getParam(name1);    BOOST_CHECK_NE(p, (QueryParameter*)nullptr);    BOOST_CHECK_EQUAL(p->name(), name1);    BOOST_CHECK_EQUAL(p->valueCount(), 0);}BOOST_AUTO_TEST_CASE(QueryString_can_parse_query_string_with_one_parameter){    stringstream stream(name1=value1);    QueryString qs(stream);    BOOST_CHECK(qs.numParams() == 1);    BOOST_CHECK_EQUAL(qs.hasParam(name1), true);    auto p = qs.getParam(name1);    BOOST_CHECK_NE(p, (QueryParameter*)nullptr);    BOOST_CHECK_EQUAL(p->name(), name1);    BOOST_CHECK_EQUAL(p->valueCount(), 1);    BOOST_CHECK_EQUAL(p->firstValue(), value1);}BOOST_AUTO_TEST_CASE(QueryString_can_parse_query_string_with_one_parameter_multiple_values){    stringstream stream(name1=value1&name1=value2);    QueryString qs(stream);    BOOST_CHECK(qs.numParams() == 1);    BOOST_CHECK_EQUAL(qs.hasParam(name1), true);    auto p = qs.getParam(name1);    BOOST_CHECK_NE(p, (QueryParameter*)nullptr);    BOOST_CHECK_EQUAL(p->name(), name1);    BOOST_CHECK_EQUAL(p->valueCount(), 2);    auto values = p->allValues();    BOOST_CHECK_EQUAL(values[0], value1);    BOOST_CHECK_EQUAL(values[1], value2);}BOOST_AUTO_TEST_CASE(QueryString_can_parse_query_string_with_two_parameters){    stringstream stream(name1=value1&name2=value2&);    QueryString qs(stream);    BOOST_CHECK(qs.numParams() == 2);    //first parameter    BOOST_CHECK_EQUAL(qs.hasParam(name1), true);    auto p = qs.getParam(name1);    BOOST_CHECK_NE(p, (QueryParameter*)nullptr);    BOOST_CHECK_EQUAL(p->name(), name1);    BOOST_CHECK_EQUAL(p->valueCount(), 1);    BOOST_CHECK_EQUAL(p->firstValue(), value1);    //second parameter    BOOST_CHECK_EQUAL(qs.hasParam(name2), true);    p = qs.getParam(name2);    BOOST_CHECK_NE(p, (QueryParameter*)nullptr);    BOOST_CHECK_EQUAL(p->name(), name2);    BOOST_CHECK_EQUAL(p->valueCount(), 1);    BOOST_CHECK_EQUAL(p->firstValue(), value2);}
C++ Query String Parser
c++;unit testing;c++14;url;boost
Looks largely correct on the surface. Of course there's plenty to fix!class UrlUtil final{public:    static std::string urlDecode(const std::string &urlEncodedString);};Personally, I despise final. It serves no purpose in the average codebase, but it does gratuitously break several metaprogramming techniques, such as the detection idiom. If your codebase actually makes use of inheritance, okay, maybe final is worth the headaches; but even in an inheritance codebase, personally I'd chuck it.A class all of whose members are public could equally well be a struct.In fact, a class all of whose members are static could equally well be a namespace! That's what you should do here. C++ is not Java; not everything needs to be a class.namespace UrlUtil {    std::string urlDecode(const std::string& urlEncodedString);} // namespace UrlUtilThe repetition of Url...url...url...Url is a bit silly; you might find that you don't need the namespace at all.std::string url_decode(const std::string& encoded);Mildly cleaning up as we go:class QueryParameter{public:    const std::string& name() const { return name_; }    const std::string& firstValue() const;    //Question 1: because this is a const&, this is not bad practice, correct?    const std::vector<std::string>& allValues() const { return values_; }Correct, this is fine and nicely efficient, as long as you're sure your callers know what they're doing. But if your callers don't know what they're doing, they could easily end up with dangling references:QueryParameter p(hello);p.addValue(1);const std::string& it = p.firstValue();p.addValue(2);  // causes the vector to resize, invalidating the reference we got abovestd::cout << it << std::endl;  // boom, dereference a dangling pointerSo your callers have to be careful not to do this kind of thing. Just be aware of that.    int valueCount() const { return values_.size(); }Nit: I might make this method return size_t instead of int, just because that's the natural type of the size of a vector. But there's a good argument in favor of int here too: namely, that int is the natural integer type for the platform.    QueryParameter(const std::string name) : name_(std::move(name)) {   }Here is your first major bug. name is a const std::string, so std::move(name) is an rvalue of type const std::string&&. Now, which constructor do you think will get called for name_  the copy constructor or the move constructor?Can't be the move constructor, because the move constructor takes a non-const std::string&&. So it'll end up falling back on the copy constructor, and you'll end up making a(nother) copy of name when you thought you were being efficient. The correct code is    QueryParameter(std::string name) : name_(std::move(name)) {}As a bonus, the correct code is shorter! (This is a common occurrence in C++.)private:    friend class QueryString;Friends are a code smell. Prefer to expose the functionality to all the users of this class; and if you don't want anyone besides your QueryString to create instances of this class, then give it an obscure name, such as namespace detail { struct qs_parameter; } or whatever. Or just leave it out of your documentation.    void addValue(std::string value) { values_.push_back(value); }Again, you're copying (twice) when you don't need to. At the very least make this    void addValue(std::string value) { values_.push_back(std::move(value)); }and if you really want to get fancy and eliminate even the move-construction, you might use perfect forwarding:    template<class T> void addValue(T&& value) { values_.emplace_back(std::forward<T>(value)); }std::vector<std::string> getParamNames() const;//Question 2: Instead of returning a vector<string>, would something like this be better?//void fillParamNames(std::vector<std::string>& into) const;As of C++11, absolutely not! Chandler Carruth has a good talk on why inout reference parameters are terribly awkward for optimizers. Prefer to return a nice clean newly-constructed value whenever possible.//Question 3: I am really not sure of the appropriate exception to throw here...throw std::runtime_error(Attempted to obtain first value of query string parameter that does not have any values);The easy answer is: Don't throw anything. Just declare that calling firstValue on an empty QueryParameter is a bug and you're not allowed to do that. (In other words, use undefined behavior to your advantage.)(*itr).second is a needlessly complicated way of writing itr->second.return std::move(names); is a needlessly inefficient way of writing return names;. When you return a local variable, you'll get move-construction at worst and copy-elision at best (or, as of C++17, copy-elision at worst). When you return an expression like std::move(names), you'll get move-construction at worst and move-construction at best. When I say move-construction, I mean a single pointer copy. When I say copy-elision, I mean literally zero instructions generated.Here's an egregious example:std::string decodedValue = std::move(UrlUtil::urlDecode(urlEncodedValue));return std::move(decodedValue);I count two move-constructions there, whereasstd::string decodedValue = UrlUtil::urlDecode(urlEncodedValue);return decodedValue;would have had zero move-constructions (and zero copies). Of course the right thing to write is justreturn UrlUtil::urlDecode(urlEncodedValue); as usual, the most correct code is also the shortest code.The statement parameters_.emplace(name, std::move(newParam)).first; is nonsense. What's that .first doing there? Doesn't your compiler warn you about it? Turn up your warning level and (on GCC or Clang) use -W -Wall -Werror habitually!std::map<std::string, std::unique_ptr<QueryParameter>> parameters_;This unique_ptr is a code smell. Why would you want an extra layer of heap-allocation here? What's wrong withstd::map<std::string, QueryParameter> parameters_;? (Protip: Nothing is wrong with the latter. Use it.)I don't really see why you want all these intermediate entities, if the underlying data structure is simply a std::map<std::string, std::vector<std::string>>. If it were me, I'd probably just write a single functionusing query_parameter_set = std::map<std::string, std::vector<std::string>>;query_parameter_set parse_query_string(const std::string& input);and then I could write your entire test suite asauto qs = parse_query_string(name1=value1&name2=value2&);BOOST_CHECK_EQUAL(qs.size(), 2);BOOST_CHECK_NE(qs.find(name1), qs.end());BOOST_CHECK_EQUAL(qs[name1].size(), 1);BOOST_CHECK_EQUAL(qs[name1][0], value1);BOOST_CHECK_NE(qs.find(name2), qs.end());BOOST_CHECK_EQUAL(qs[name2].size(), 1);BOOST_CHECK_EQUAL(qs[name2][0], value2);And because I was using STL containers, I'd have the entire algorithms library at my disposal if I needed it  you know, count_if and sort and whatnot.