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;
}

• Neeraj Kumar: Thank you so much for your comments @LWimsey. I addressed all of them. However I have just 2 concerns with them. 1. The reason I use multimap is coz, same path can be mapped to different method. For eg (GET, POST) to same /user path. 2. I liked the idea of eliminating default constructor. However one thing I noticed new is the usage of { } (curly braces) during member intialization. Just wanted to know, what benefit it will have over ( ) (old common brackets) approach. thanks.
• LWimsey: @Neeraj Thank you for your quick reply.. I didn't know you were going to use multimap, but you can simply replace map with multimap. Note that the insert return type is a plain iterator instead of a pair<interator,bool>. Reason is that an insert cannot fail on account of the multiple key property, so you don't need the bool
• LWimsey: Curly braces is a new (C++11) initialization syntax that provides several benefits compared to the old parentheses syntax. Most important is that it prevents type narrowing. For example, int i{3.14} produces an error since a double does not fit in an int, but int i(3.14) is fine and it will truncate the value and store 3 in the variable.