C++ Query String Parser

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 &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 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 getParamNames() const;
    //Question 2: Instead of returning a vector, would something like this be better?
    //void fillParamNames(const std::vector 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> parameters_;
};

HttpUtility.cpp:

```
#include "HttpUtility.h"

#include

/*
* UrlUtil Methods
*/
//Note: static method
std::string UrlUtil::urlDecode(const std::string &urlEncodedString)
{
std::stringstream input(urlEncodedString);
std::string decodedString = "";

while(!input.eof())

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 UrlUtil

The 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& 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 above
std::cout << it << std::endl;  // boom, dereference a dangling pointer

So 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 void addValue(T&& value) { values_.emplace_back(std::forward(value)); }

std::vector getParamNames() const;
//Question 2: Instead of returning a vector, would something like this be better?
//void fillParamNames(std::vector& 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-constr

StackExchange Code Review Q#151815, answer score: 4