3
\$\begingroup\$

I am extending std::list in order to have access to convenience member functions that simplify my everyday developments (e.g. ways to deal with doubloons, delete all the stored pointers, etc.). I removed some of these member functions, but I kept the essential ones.

#ifndef __PJ_LIST_H__
#define __PJ_LIST_H__

#include <list>
#include <iostream>

// This class is a wrapper of std::list to provide some smarter functionalities
template<class T>
class PjList: public std::list<T>
{
    public:

        typedef typename PjList<T>::iterator PjIterator;

        PjList() {};
        virtual ~PjList() {};

        // Helper functions to deal with doubloons
        bool contains(const T& t);
        bool containsDoubloons();
        PjList<T> removeDoubloons();

        // Calls destructor of all list item and empty the list
        void destroyContents();

        // Accessing elements at index
        T& at(int index);

        // Inserting element around "ref"
        bool insertItem(const T& item, const T& ref, bool before = true);

};

template<class T>
bool PjList<T>::contains(const T& t)
{
    PjIterator it;

    for( it = std::list<T>::begin(); it != std::list<T>::end(); it++ )
    {
        if ( *it == t )
            return true;
    }
    return false;
}

template<class T>
bool PjList<T>::containsDoubloons()
{
    PjIterator it;
    PjIterator jt;
    int nb_occurence;

    for( it = std::list<T>::begin(); it != std::list<T>::end(); it++ )
    {
        nb_occurence = 0;
        for( jt = std::list<T>::begin(); jt != std::list<T>::end(); jt++ )
        {
            if ( *it == *jt )
                nb_occurence += 1;
        }
        if ( nb_occurence > 1 )
            return true;
    }
    return false;
}

template<class T>
PjList<T> PjList<T>::removeDoubloons()
{
    PjList<T> output;

    PjIterator it;
    for( it = std::list<T>::begin(); it != std::list<T>::end(); it++ )
    {
        T& t = *it;
        if ( !output.contains(t) )
            output.push_back(t);
    }

    return output;
}

template<class T>
void PjList<T>::destroyContents()
{
    PjIterator it;

    for( it = std::list<T>::begin(); it != std::list<T>::end(); it++ )
        delete *it;

    std::list<T>::clear();
}

template<class T>
bool PjList<T>::insertItem(const T& t, const T& ref, bool before)
{
    bool ref_found = false;

    PjIterator it;

    for( it = std::list<T>::begin(); it != std::list<T>::end(); it++ )
    {
        if ( *it == ref )
        {
            ref_found = true;
            break;
        }
    }

    if ( !ref_found )
    {
        std::cerr << "Insertion in list failed since ref item is not found" << std::endl ;
        return false;
    }

    insert(it, t);
    return true;
}

template<class T>
T& PjList<T>::at(int index)
{
    static T dummy;
    int i = 0;
    PjIterator it;

    for( it = std::list<T>::begin(); it != std::list<T>::end(); it++ )
    {
        if ( index == i++ )
            return *it;
    }
    std::cerr << "Bad index " << index << std::endl ;
    return dummy;
}

#endif
\$\endgroup\$
  • \$\begingroup\$ Why add destroyContents() when there is clear and erase? Also why doesn't containsDoubloons() use contains()? \$\endgroup\$ – pacmaninbw Apr 6 '16 at 16:19
  • \$\begingroup\$ Why doesn't at() check against size() before going into the loop? \$\endgroup\$ – pacmaninbw Apr 6 '16 at 16:23
  • \$\begingroup\$ Welcome to Code Review! I hope you get some good answers. \$\endgroup\$ – Phrancis Apr 6 '16 at 17:08
  • \$\begingroup\$ In containsDoublooons, instead of iterating from the beginning in the inner loop, you can just iterate from it to the end. Earlier elements have already been compared to *it. \$\endgroup\$ – Nick Matteo Apr 6 '16 at 18:46
  • \$\begingroup\$ @pacmaninbw The destroyContents also deletes the pointed data. @kundor I did not think about this, thanks. @Phrancis Thank you. \$\endgroup\$ – P. Rodriguez Apr 7 '16 at 15:00
10
\$\begingroup\$

I see no reason to have this be an entirely new data structure that inherits from std::list<T>. All of the new member functions you've defined are really just wrapper functions that could just be delegated to std::list<T> functions or functions that could be implemented as regular procedural functions.

For example, the contains() and at() and insertItem() (which should really just be named insertItemBefore()) functions could simply be:

namespace list_util {

template <class T>
bool contains(const std::list<T>& l, const T& elem)
{
    return std::find(l.begin(), l.end(), elem) != l.end();
}

template <class T>
T& at(const std::list<T>& l, std::size_t index)
{
    assert(index < l.size());
    auto begin = l.begin();
    std::advance(begin, index);
    return *begin;
}

template <class T>
bool insertItemBefore(std::list<T>& l, const T& ref_item, const T& elem)
{
    auto itemItr = std::find(l.begin(), l.end(), ref_item);

    // if we found the item, itemItr != l.end()
    if (itemItr != l.end()) {
        l.insert(itemItr, elem);
        return true;
    }
    return false;
}

}

You'd have to #include<algorithm> and #include<iterator> for this to work. Notice that the index parameter is now std::size_t to avoid having to check for negative indexes. They could then simply be called as: list_util::contains(the_list, elem); and list_util::at(the_list, the_index);.

Moreover, containsDoubloons() could be implemented using std::find() and destroyContents() is really just a call to std::list<T>::clear():

namespace list_util {

template<class T>
bool containsDoubloons(const std::list<T>& l)
{
    for (auto it = l.begin(); it != l.end(); ++it) {
        const auto beginItr = std::next(it);
        if (beginItr != l.end() && 
            std::find(beginItr, l.end(), *it) != l.end()) {
            return true;
        }
    }
    return false;
}

template<class T>
void destroyContents(std::list<T>& l)
{
    l.clear();
}

}

Moreover, since the new functions we've written are completely generic and use the STL iterators for all operations, there's no reason to limit them only to std::list<T>. We could allow any container of containing T to be a parameter. An example is the contains() function:

template < template <class> class C, class T>
bool contains(const C<T>& cont, const T& elem)
{
    return std::find(cont.begin(), cont.end(), elem) != cont.end();
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for your time. I figure out I need to work on my C++ a little bit... \$\endgroup\$ – P. Rodriguez Apr 7 '16 at 15:02
1
\$\begingroup\$

I don't think the idea is sound in the first place. First, destroyContains is definitely wrong. Use a container ot std::unique_ptr<> if you have reponsibility of the content. If you don't know if you have, solve the issue, that's a really bad code smell.

Then, if you need to have a container of unique elements, why are you using a list? If you need ordering, use a set, if you don't need ordering, use an ordered_set.

What you are trying to do is definitely not what you should be doing. Sorry, you definitely must not do this.

\$\endgroup\$
  • \$\begingroup\$ BTW, the destructor should not be virtual and is useless here as the parent destructor is obviously not virtual. \$\endgroup\$ – Matthieu Brucher May 23 '16 at 14:05

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.