Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
template <class T, int max_size>
class FixedSizedUniqueStack
{
    std::vector<T> m_vec;
    std::unordered_set<T> m_uset;
public:
    FixedSizedUniqueStack():m_vec(max_size){}
    bool push(T& x)
    {
        bool success = true;
        if(m_uset.insert(x)) m_vec.push_back(x);
        else success = false;
        return success;
    }
    void pop()
    {
        if(m_vec.size() > 0)
        {
            m_uset.erase(m_vec.back());
            m_vec.pop_back();
        }
    }
};
share|improve this question
2  
Please make the title be a ... title. Post your information about the code in the body of the question, covering your concerns which the answers will talk about. –  black Feb 22 at 11:30
1  
This question could really use some description about the problem you are trying to solve. Try to give people a little bit of context about the code you wrote. –  glampert Feb 22 at 23:05

1 Answer 1

up vote 2 down vote accepted

Duplicate value storage:

One disadvantage of your approach is that it stores a duplicate of each value inserted. The cost can become prohibitive for large objects, such as class instances stored by value.

A possible approach to avoid the duplicate would be just doing a linear search over the vector every time a new insertion takes place. For small arrays, this is usually quite fast.

Another possible implementation would be to keep the vector always sorted, then a fast binary search could be performed before insertions to ensure uniqueness.

Proper way of testing set/map insertion:

This code doesn't seem correct:

    bool success = true;
    if(m_uset.insert(x)) m_vec.push_back(x);
    else success = false;
    return success;

The insert method of sets and maps of the Standard Library will return you a pair consisting of an iterator to the inserted element (or to the element that prevented the insertion) and a bool denoting whether the insertion took place (from here).

So the correct way to test if the insertion was successful would be:

if (m_uset.insert(value).second == true)
{
    ...
}

max_size is never checked:

You've stated in the title that the container should have a fixed maximum size, however, you never check to see if the limit size was reached. push_back in a vector will resize the container if its initial capacity is exceeded. Same is true for set::insert.

push() is taking a mutable reference, which can cause confusion:

The signature of bool push(T& x), taking a mutable reference to T, shows the called that this method has the intention or possibility of altering the input parameter. That would be a pretty strange behaviour for a container. When storing some data, you don't want it to be changed by the container. The proper signature here would be to pass the value by const reference:

bool push(const T& x);

That will prevent a copy and will pass a clear message to the caller that the input value is not changed, only stored.

As you polish this interface, you should consider move semantics (C++11), and add a move reference (T&&) overload for push().

empty() is a more idiomatic way of checking for empty containers:

The traditional way of testing if a Standard container is empty is by calling the empty() method, as opposed to if (c.size() > 0).

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.