Request and callback design

Question

I am just trying to improve my coding and designing skills in C++ and for that I am trying to solve same age old problem of mapping a http request to method.

Could you guys please look at my program and point me to mistakes I am doing with design and coding style?

resource.hpp

#include <functional>
#include <string>


typedef std::function<void(void)> Handler;

enum class RequestMethod {
  GET,
  PUT,
  POST,
  DELETE
};

class Resource {

  public:
    Resource() {}  
    void SetMethodHandler(const RequestMethod method, const Handler& callback);
    Handler GetMethodHandler();

    void SetPath(const std::string& path);
    std::string& GetPath();

  private:
    Resource(const Resource& other) = delete;
    Resource& operator=(Resource& rhs) = delete;

    std::string m_path;
    RequestMethod m_method;
    Handler m_callback;
};

resource.cpp

#include "resource.hpp"

void Resource::SetPath(const std::string& path) {
  m_path = path;
}


std::string& Resource :: GetPath() {
  return m_path;
}

void Resource :: SetMethodHandler(RequestMethod method, const Handler& callback) {
  m_method = method;
  m_callback = callback;
}

Handler Resource :: GetMethodHandler() {
  return m_callback;
}

main.cpp

#include "resource.hpp"
#include <memory>
#include <iostream>
#include <map>

void LoginMethodHandler() {
  std::cout << "Code here to handle a login request." << std::endl;
}

int main() {

  using ResourceSharedPtr = std::shared_ptr<Resource>;

  ResourceSharedPtr resource_ptr = std::make_shared<Resource>();
  resource_ptr->SetPath("/login");
  resource_ptr->SetMethodHandler(RequestMethod::GET, LoginMethodHandler);

  /* call to method handler */
  /* ideally in full blown application, logic will be search for path and   
     method and call approriate handler */
  resource_ptr->GetMethodHandler()();

  auto path_map = std::make_pair(resource_ptr->GetPath(),resource_ptr);
  return 0;
}

Answer 1

I think the idea to create an abstraction class for request handling is a good one..

A few remarks and/or changes to consider:

• This code is flagged as C++14 but there is only C++11 specific code.

• I like the idea of using scoped enumerations (enum class ...) instead of the old-style enumerations. Scoped enumerations are more strictly typed.

• There are no include guards in resource.hpp. They can protect against (potential) double inclusions.

• Copy construction and copy assignment have been deleted, but by convention, deleted functions are declared public, not private.. As for the (deleted) copy assignment, the argument should be a const reference.

• In main.cpp, it is good pratice to keep the application specific include file (resource.hpp) under the system include files.

• get and set methods are defined in resource.cpp, but these functions are so limited, why not include them directly in the header file, which would essentially eliminate resource.cpp.

• For a default, compiler generated, constructor, it is common practice to use:

  Resource() = default;

  However, since an uninitialized object is created and populated with values after that, why not set those during construction ? - the constructor then becomes:

  Resource(const std::string &path, RequestMethod method, const Handler& callback) : m_path{path}, m_method{method}, m_callback{callback} { }

  The object can still be modified by calling set functions later.

• Resource::GetPath() returns a reference to an lvalue which gives callers direct access to Resource::m_path, which is private. Many consider this a violation of data encapsulation. Consider changing the return type to a plain std::string.

• A std::map is not used yet, but the intention seems to be to store a std::pair in a std::map called path_map. You can call std::make_pair directly on the map inserter:
  std::map< std::string, ResourceSharedPtr> path_map; ... path_map.insert(std::make_pair(resource_ptr->GetPath(),resource_ptr));

• If you are planning to use only the ResourceSharedPtr in combination with the std::map pair, you could possibly eliminate m_path inside the Resource class and use the std::map key. Otherwise they are redundant.

With this, the updated source code becomes:

resource.hpp

#ifndef RESOURCE_H
#define RESOURCE_H

#include <functional>
#include <string>

typedef std::function<void(void)> Handler;

enum class RequestMethod {
  GET,
  PUT,
  POST,
  DELETE
};

class Resource {

  public:
    Resource(const std::string &path, RequestMethod method, const Handler& callback) :
        m_path{path}, m_method{method}, m_callback{callback} { }
    void SetMethodHandler(const RequestMethod method, const Handler& callback)
    {
      m_method = method;
      m_callback = callback;
    }

    Handler GetMethodHandler() { return m_callback; }

    void SetPath(const std::string& path) { m_path = path; }
    std::string GetPath() { return m_path; }

    Resource(const Resource& other) = delete;
    Resource& operator=(const Resource& rhs) = delete;


private:
    std::string m_path;
    RequestMethod m_method;
    Handler m_callback;
};

#endif // RESOURCE_H

main.cpp

#include <memory>
#include <iostream>
#include <map>

#include "resource.hpp"

void LoginMethodHandler()
{
  std::cout << "Code here to handle a login request." << std::endl;
}

using ResourceSharedPtr = std::shared_ptr<Resource>;

std::map<std::string, ResourceSharedPtr> path_map;


int main() {

  ResourceSharedPtr resource_ptr = std::make_shared<Resource>("/login",
                                                              RequestMethod::GET,
                                                              LoginMethodHandler);

  /* call to method handler */
  /* ideally in full blown application, logic will be search for path and
     method and call approriate handler */
  resource_ptr->GetMethodHandler()();

  path_map.insert(std::make_pair(resource_ptr->GetPath(),resource_ptr));

  return 0;
}