0
\$\begingroup\$

I found those functions reviewing a C++14 codebase I am working on:

struct Service {
    Service(std::string id): id(id) {}
    virtual ~Service() {}

    std::string id;
    HeartbeatCheckpoint lastPing;
    HeartbeatCheckpoint lastPong;
};

std::map<std::string, Service> services;

bool Pacemaker::updatePong(const std::string& id) {
    auto found = services.find(id);
    if(found != services.end()) {
        found->second.lastPong = std::chrono::high_resolution_clock::now();
        return true;
    }
    return false;
}

bool Pacemaker::updatePing(const std::string& id) {
    auto found = services.find(id);
    if(found != services.end()) {
        found->second.lastPing = std::chrono::high_resolution_clock::now();
        return true;
    }
    return false;
}

I would like to refactor them creating a common function to remove code duplication, but I can't figure out how to pass the member variable (lastPing and lastPong).

Is there a better way to write this code?

\$\endgroup\$

closed as off-topic by Toby Speight, Zeta, Graipher, t3chb0t, alecxe Oct 13 '17 at 15:52

This question appears to be off-topic. The users who voted to close gave these specific reasons:

  • "Questions must involve real code that you own or maintain. Pseudocode, hypothetical code, or stub code should be replaced by a concrete implementation. Questions seeking an explanation of someone else's code are also off-topic." – Toby Speight, Graipher, alecxe
  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Zeta, t3chb0t
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 2
    \$\begingroup\$ Why? They are perfectly readable now. "Remove Code Duplication" is not a good enough reason to modify these functions. Also doing so is probably going to make the code less readable (as shown by the two attempts below). The "removing code duplication" is an admirable goal learning when to do it and when not is the differentiates a good programmer from a great one. This is a situation where you leave the code alone. \$\endgroup\$ – Martin York Oct 12 '17 at 22:12
2
\$\begingroup\$

The code as posted doesn't compile; I had to add some includes and definitions:

#include <chrono>
#include <map>
#include <string>

using HeartbeatCheckpoint = std::chrono::high_resolution_clock::time_point;

Having done that, the simplest refactoring of the functions would be to just pass a member pointer to specify the field you want to set in the found object:

bool update(const std::string& id, HeartbeatCheckpoint (Service::*field))
{
    auto found = services.find(id);
    if (found == services.end())
        return false;
    found->second.*field = std::chrono::high_resolution_clock::now();
    return true;
}

bool updatePing(const std::string& id) {
    return update(id, &Service::lastPing);
}

bool updatePong(const std::string& id) {
    return update(id, &Service::lastPong);
}

Alternatively, something I've used before is a small wrapper around find to give it an interface that can be tested inline:

namespace collection
{
    template<typename T>
    inline bool contains(const T& container, const typename T::key_type& key)
    {
        return container.find(key) != container.end();
    }
    template<typename T>
    inline bool contains(const T& container, const typename T::key_type& key, typename T::const_iterator& it)
    {
        return (it = container.find(key)) != container.end();
    }
    template<typename T>
    inline bool contains(T& container, const typename T::key_type& key, typename T::iterator& it)
    {
        return (it = container.find(key)) != container.end();
    }
    template<typename T>
    inline bool contains(const T& container, const typename T::key_type& key, typename T::mapped_type& result)
    {
        typename T::const_iterator it;
        return contains(container, key, it) && (result = it->second, true);
    }
}

This is then used slightly more readably (at the expense of being unfamiliar outside your team):

bool updatePing(const std::string& id) {
    std::map<std::string, Service>::iterator it;
    const bool found = collection::contains(services, id, it);
    if (found)
        it->second.lastPing = std::chrono::high_resolution_clock::now();
    return found;
}

It's arguably slightly neater if we assign to a pointer, rather than to an iterator:

namespace collection
{
    template<typename T, typename V>
    inline bool contains(const T& container, const typename T::key_type& key, const V *& result)
    {
        auto it = container.find(key);
        if ((it = container.find(key)) == container.end())
            return false;
        result = &it->second;
        return true;
    }
    template<typename T, typename V>
    inline bool contains(T& container, const typename T::key_type& key, V *& result)
    {
        auto it = container.find(key);
        if ((it = container.find(key)) == container.end())
            return false;
        result = &it->second;
        return true;
    }
}

bool updatePing(const std::string& id) {
    Service *service;
    const bool found = collection::contains(services, id, service);
    if (found)
        service->lastPing = std::chrono::high_resolution_clock::now();
    return found;
}

As you haven't shown us where these functions are used, we can't determine whether the return value is important or not. If it's not required, we can easily use the Null Object pattern to provide a sink for the result:

Service& find_service_or_default(const std::string& id)
{
    static Service dummy{""};
    auto it = services.find(id);
    return (it == services.end()) ? dummy : it->second;
}

void updatePing(const std::string& id)
{
    find_service_or_default(id).lastPing = std::chrono::high_resolution_clock::now();
}

void updatePong(const std::string& id)
{
    find_service_or_default(id).lastPong = std::chrono::high_resolution_clock::now();
}

This is all assuming that you have more functions than just those two - if not, then the refactoring is unlikely to be an improvement, and the original (with side-by-side duplication of a clearly recognisable structure) is likely to be more readable.

\$\endgroup\$
0
\$\begingroup\$

You can try lambda, but I don't think this increases readability too much. It just depends.

bool update(const std::string& id, std::function<void(Service& target)> func)
{
    auto found = services.find(id);
    if(found != services.end()) {
        func(found->second);
        return true;
    }
    return false; 
}

bool updatePong(const std::string& id) {
    return update(id, [&](Service& service) {
        service.lastPong = std::chrono::high_resolution_clock::now(); 
    } );
}

bool updatePing(const std::string& id) {
   return update(id, [&](Service& service) {
       service.lastPing = std::chrono::high_resolution_clock::now(); 
   } );
}

Another option would be change HeartbeatCheckpoint to array<HeartbeatCheckpoint, 2> when let's say item 0 is ping and item 1 pong (you could create an enum for these indices). Then you can update single array item and there's no need for lambda.

\$\endgroup\$
  • 1
    \$\begingroup\$ Well, or one could change updatePing/updatePong to take a Service& or Service* as parameter (so the "find the service object" logic gets passed to the caller). Gets rid of the lambdas, but that's about it... \$\endgroup\$ – hoffmale Oct 12 '17 at 11:10
  • \$\begingroup\$ I've tried std::function because I read that it can store also class fields. But it seems that it cannot be used to update it's value. Am I right? \$\endgroup\$ – Marco Stramezzi Oct 12 '17 at 11:39
  • \$\begingroup\$ @MarcoStramezzi please provide code, std::function can store class members. \$\endgroup\$ – vasek Oct 12 '17 at 12:00
  • \$\begingroup\$ Slightly pedantic, but: a std::function can store a lambda expression, and the lambda can capture members (or member pointers, or references, or pointers, .... etc). \$\endgroup\$ – Toby Speight Oct 12 '17 at 17:36

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