C++ Query String Parser

Question

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 method
std::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 method
std::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 method
std::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.h"

using 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");
}

Answer 1

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<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 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<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, whereas

std::string decodedValue = UrlUtil::urlDecode(urlEncodedValue);
return decodedValue;

would have had zero move-constructions (and zero copies). Of course the right thing to write is just

return 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 with

std::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 function

using 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 as

auto 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.

Answer 2

Question 1: because this is a const&, this is not bad practice, correct?

Returning a const& is fine IMO, but there is a caveat: you must not let the reference live longer than the object. Things like this will be invalid (I think, but at least it's questionable whether it's valid, which is a problem):

QueryParameter someFunction();

void myFunction() {
    someFunction().allValues()[0]
}

Basically, when a member function returns a const&, you need to ensure that you never use that function on a temporary, and never keep the reference past the lifetime of the object.

---

QueryParameter(const std::string name) : name_(std::move(name)) {   }

This is a long horizontal line. You really like long horizontal lines. I would recommend splitting this:

QueryParameter(const std::string name)
    : name_(std::move(name))
{}

This way, there is only one "chunk" of information per line; it makes it easier to read.

However, this isn't how you'd take a parameter that you want to std::move; you should just take it by value without const; it doesn't make a difference to the end user. Also, use brace initialization:

QueryParameter(std::string name)
    : name_{ std::move(name) }
{}

Finally, I'd probably have implemented this like so, for more terseness in the constructor, and to avoid the use of std::move (which requires more cognitive effort to ensure it was used right):

QueryParameter(const std::string &name)
    : name_{ name }
{}

---

bool hasParam(const char *name) const { return parameters_.find(std::string(name)) != parameters_.end(); }

Don't take a const char * as a parameter. Yes, I know you are intending support for string literals, maybe thinking, "It's more efficient." It may be true that the program will be more efficient, but that's only in niche cases. If you took a std::string as a parameter, you would save having to construct one every time the function is called:

// Also, my rule of thumb is that only simple getters and setters
// should be one line like this
bool hasParam(const std::string &name) const {
    return parameters_.find(name) != parameters_.end();
}

Note that this is simpler as:

bool hasParam(const std::string &name) const {
    return parameters_.count(name);
}

---

class UrlUtil final
{
public:
    static std::string urlDecode(const std::string &urlEncodedString);
};

A class with only static methods is a code smell in C++. You almost always want a namespace:

namespace url_util {
    static std::string urlDecode(const std::string &urlEncodedString);
}

I'd go even further and suggest that your entire header should be in a namespace:

namespace url {

namespace util {
    static std::string urlDecode(const std::string &urlEncodedString);
}
....
}

---

You need header guards on your header file:

#pragma once // you can use this at the beginning of the .h file for most compilers, but it's often considered to use the #ifndef method

---

Returning from your function with return std::move(result); in theory helps, but I wouldn't do it unless the code has proven to be a bottleneck with profiling. It's a small optimization, and one which I would expect the compiler to be able to do on its own.