Creating URL with Builder pattern

Question

What do you think about this piece of code? How would you design the builder pattern in this situation?

#ifndef CPP_URL #define CPP_URL #include<iostream> #include<vector> #include<map> using std::string; enum protocol { HTTP, HTTPS }; static string map(protocol scheme) { switch(scheme) { case HTTP: return "http"; default: return "https"; } } static int defaultPort(protocol scheme) { switch (scheme) { case HTTP: return 80; default: return 440; } } class url final { private: using list = std::vector<std::string>; using pairs = std::map<std::string, std::string>; const list paths; const pairs queries; const protocol scheme; const string domain; const string fragment; const string username; const string password; mutable int port; public: class builder { friend url; private: protocol scheme; pairs queries; list paths; string domain; string fragment; string username; string password; int port = -1; public: builder& setScheme(protocol); builder& setFragment(const string&); builder& setDomain(const string&); builder& setUsername(const string&); builder& setPassword(const string&); builder& setPort(int port); builder& addQuery(const string&, const string&); builder& addPath(const string&); url& build() const; }; explicit url(const url::builder&); string build() const; }; using builder = url::builder; inline builder& builder::setScheme(protocol scheme) { this->scheme = scheme; return *this; } inline builder& builder::setUsername(const string& username) { this->username = username; return *this; } inline builder& builder::setPassword(const string& password) { this->password = password; return *this; } inline builder& builder::setFragment(const string& fragment) { this->fragment = fragment; return *this; } inline builder& builder::setDomain(const string& domain) { this->domain = domain; return *this; } inline builder& builder::setPort(int port) { this->port = port; return *this; } inline builder& builder::addQuery(const string& key, const string& value) { queries.insert(std::pair<string, string>(key, value)); return *this; } inline builder& builder::addPath(const string& path) { paths.insert(paths.begin(), path); return *this; } inline url& builder::build() const { return *(new url(*this)); } inline url::url(const builder& object) : paths(object.paths) , queries(object.queries) , scheme(object.scheme) , domain(object.domain) , fragment(object.fragment) , username(object.username) , password(object.password) , port(object.port) {} string url::build() const { string url = map(scheme).append("://"); if (!username.empty()) { url.append(username); if (!password.empty()) { url.append(":").append(password); } url.append("@"); } url.append(domain); if (port == -1) port = defaultPort(scheme); url.append(":").append(std::to_string(port)); for (string path : paths) { url.append("/"); url.append(path); } auto it = queries.begin(); if (it != queries.end()) { url.append("?").append(it->first) .append("=").append(it->second); for(it++; it != queries.end(); it++) { url.append("&").append(it->first) .append("=").append(it->second); } } if (!fragment.empty()) { url.append("#").append(fragment); } return url; }; #endif 

This is how we can use that:

 url url = url::builder() .setScheme(HTTPS) .setDomain(YOUTUBE_DOMAIN) .addPath(WATCH_PATH) .addQuery(CATEGORY, ITEM) .build(); std::cout<<url.build()<<std::endl; 

Answer 1

Include the required headers

You use std::string extensively, but you don't include <string>. This is allowed to work (any standard header is allowed to act as if it includes any or all others), but it's not required to work (and didn't, for me).

Use the proper defaults

Right now, your code defaults to https on port 440, but the normal default for https is port 443. I'd tend to include the port in the result if and only if it's explicitly specified. Otherwise, it's probably better to let the browser sort out the port, since most know quite a few more than your code does.

Fluent Interface

I'm not particularly excited about fluent interfaces in general, but if you're going to use one, I'd prefer to get rid of the set on the beginnings of most of the setters. If you have something like:

auto uri = builder().scheme(HTTPS).domain("www.google.com").build(); 

...you don't really need a set on the beginning of each to make it apparent what you intend.

Consider unique types for the parts of a URL/URI

I tend to wonder whether we wouldn't be better served by a set of tiny classes, one for each type of element in a URL, with each able to (for example) insert itself to a stream, with the url class basically just a set of those. At least for some of the obvious cases, I'd also at least consider using overloaded operators instead of named functions. For example, a fairly minimalist version might look something like this:

struct protocol { enum prot { http, https } p_; protocol(prot p) : p_(p) {} friend std::ostream &operator<<(std::ostream &os, protocol const &p) { switch (p.p_) { case http: return os << "http://"; case https: return os << "https://"; } } }; struct query { std::string k; std::string v; }; class queries { std::vector<query> queries_; public: void add(query const &q) { queries_.push_back(q); } friend std::ostream &operator<<(std::ostream &os, queries const &q) { std::string sep = "?"; for (auto const & f : q.queries_) { os << sep << f.k << "=" << f.v; sep = "&"; } return os; } }; class path { std::vector<std::string> components; public: void add(std::string const &path) { components.push_back(path); } friend std::ostream &operator << (std::ostream &os, path const &p) { for (auto const & s : p.components) os << s << "/"; return os; } }; class url { protocol prot_; path path_; queries q_; public: url(protocol::prot p) : prot_(p) {} url &operator/(std::string const &p) { path_.add(p); return *this; } url &operator&(query const &q) { q_.add(q); return *this; } friend std::ostream &operator<<(std::ostream &os, url const &u) { return os << u.prot_ << u.path_ << u.q_; } }; int main() { url u = url(protocol::http) / "www.youtube.com" / "watch" & query{"v", "tpnrd0xGRsw"}; std::cout << u; } 

Note how url's operator<< (roughly equivalent to your build) is now almost trivially simple--and if we add in types for a fragment, port, user name, and so on, it'll remain almost equally trivial, because most of the complexity will be delegated to each of those classes instead of being crammed together into the one giant "build" that knows everything about a URL and how all the pieces need to be delimited and such. Instead, it needs to know the order of the components in a URL, but that's about it. All the details of how to write each piece are delegated to the type for that piece (most of which are individually pretty trivial too).

More syntactic sugar

After some further thought, I've decided that a little more syntactic sugar probably won't rot our teeth too badly. It might be worth adding a couple of tiny functions like this:

url http() { return url(protocol::http); } url https() { return url(protocol::https); } 

This way we can generate a url something like this:

url u = https() / "www.youtube.com" / "watch" & query{"v", "tpnrd0xGRsw"}; 

Seems a tad cleaner to me, anyway.