current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 8 down vote favorite
1

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.

share|improve this question

2 Answers 2

up vote 11 down vote accepted

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;
    });
}
share|improve this answer
    
+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. –  Loki Astari Mar 11 '12 at 17:00
    
@LokiAstari: What do you mean? The struct and class keywords are almost synonyms. –  Anton Golov Mar 11 '12 at 19:25
    
@AntonGolov: I use struct for property bags. Its not anything major just a stylistic thing that I have adopted. –  Loki Astari Mar 11 '12 at 22:31
up vote 0 down vote

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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