5
\$\begingroup\$

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

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;
}
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Neeraj Feb 21 '17 at 2:53
  • \$\begingroup\$ @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 \$\endgroup\$ – LWimsey Feb 21 '17 at 9:33
  • \$\begingroup\$ 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. \$\endgroup\$ – LWimsey Feb 21 '17 at 9:34

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.