# Request and callback design

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>

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>();

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


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"

{
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() {

RequestMethod::GET,

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

• 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. Feb 21, 2017 at 2:53
• @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 Feb 21, 2017 at 9:33
• 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. Feb 21, 2017 at 9:34