8
\$\begingroup\$

I have a Queue class that is basically a single linked list. It has 2 remove methods. What would be the cleanest way to avoid duplication of the code? Only the comparisons in methods are different.

int Queue::remove(const string& substring)
{
    // Loop through the queue list
    size_t found;
    found = current_job->description.find(substring)
    if (found != string::npos) {
        // Remove job.
    }
    // Return the amount of removed jobs.   
}

int Queue::remove(int priority)
{
    // Loop through the queue list
    if (current_job->priority == priority) {
        // Remove job.
    }
    // Return the amount of removed jobs.     
}

I thought a template solution as answered here, but in this case, the comparison function has different arguments.

\$\endgroup\$

2 Answers 2

11
\$\begingroup\$

You can abstract out the thing that changes, the comparison expression, into a predicate for a template function.

class PriorityMatch
{
    int priority_;
public:
    explicit PriorityMatch(int priority)
        : priority_(priority) {}

    bool operator()(const Job& job)
    {
        return job->priority == priority_;
    }
};

class DescriptionMatch
{
    std::string substring_;
public:
    explicit DescriptionMatch(const std::string& substring)
        : substring_(substring) {}

    bool operator()(const Job& job)
    {
        return job->description.find(substring_) != std::string::npos;
    }
};

template<class Predicate>
int Queue::remove(Predicate predicate)
{
    // Loop through the queue list
    if (predicate(current_job)) {
        // Remove job.
    }
    // Return the amount of removed jobs.     
}

int Queue::remove(const string& substring)
{
    return remove(DescriptionMatch(substring));
}

int Queue::remove(int priority)
{
    return remove(PriorityMatch(substring));
}

Or in C++11:

template<class Predicate>
int Queue::remove(Predicate predicate)
{
    // Loop through the queue list
    if (predicate(current_job)) {
        // Remove job.
    }
    // Return the amount of removed jobs.     
}

int Queue::remove(const string& substring)
{
    return remove([](const Job& job){
        return job->description.find(substring) != std::string::npos;
    });
}

int Queue::remove(int priority)
{
    return remove([](const Job& job){
        return job->priority == priority;
    });
}
\$\endgroup\$
3
  • \$\begingroup\$ +1. The only difference; I usually make simple functors like PriorityMatch and DescriptionMatch private member structures rather than classes. This is because I do not seem them as real object but rather as property bags that are used to transport information to a call site. \$\endgroup\$ Mar 11, 2012 at 17:00
  • \$\begingroup\$ @LokiAstari: What do you mean? The struct and class keywords are almost synonyms. \$\endgroup\$ Mar 11, 2012 at 19:25
  • \$\begingroup\$ @AntonGolov: I use struct for property bags. Its not anything major just a stylistic thing that I have adopted. \$\endgroup\$ Mar 11, 2012 at 22:31
0
\$\begingroup\$

Just to add a comment, I think that renaming both member functions to Queue::removeBySubstring and Queue::removeByPriority could improve code readability at the client side.

\$\endgroup\$

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.