6
\$\begingroup\$

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;
\$\endgroup\$
6
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ To be pedantic, and to follow RFC 3986, we could also add a fragment class. \$\endgroup\$ – Edward Apr 3 '18 at 12:44
  • 1
    \$\begingroup\$ @Edward: Yes, and probably username and password, while we're at it. That's why I called this "minimalist"--enough to make the intent apparent, but definitely not even an attempt at a finished product. \$\endgroup\$ – Jerry Coffin Apr 3 '18 at 14:11
  • \$\begingroup\$ @JerryCoffin Awesome! Thanks for the effort. Can we use std::pair<std::string, std::string> instead of query? \$\endgroup\$ – nullbyte Apr 4 '18 at 0:29
  • 1
    \$\begingroup\$ @nullbyte: You could--in fact, I had it that way at one point, but decided it was worth a couple extra lines of code to be able to refer to the key and value as k and v (or key and value, if you prefer) instead of first and second. \$\endgroup\$ – Jerry Coffin Apr 4 '18 at 0:41
  • \$\begingroup\$ @Jerry Coffin, thanks! Also, is it preferable to use overloaded operators in C++ rather than fluent interfaces? Which is considered as the standard way of doing it? It looks nice with operators! In Java, we can't overload operators, so I'm just curious how professional developers do it in C++. \$\endgroup\$ – nullbyte Apr 4 '18 at 1:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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