tags:

views:

106

answers:

3

I have a set where i want to find items in it. Right now i have global objects that i am using to store my finds - (ItemSetMap allMusicByBand)

I would like to get away from this and just search the sets directly.

All the cd info are stored in the private section - (ItemSet allCDS;)

here is the library.cpp -

the commented code is where i was doing my search and adding to the global object...

I would like to do the search in the musicByBand function instead..

#include "Library.h"
#include "book.h"
#include "cd.h"
#include "dvd.h"

#include <iostream>




//ItemSetMap allBooksByAuthor;       //these are what i am trying to get away from...
ItemSetMap allmoviesByDirector;
ItemSetMap allmoviesByActor;

//ItemSetMap allMusicByBand;
ItemSetMap allMusicByMusician;


const Item* Library::addMusicCD(const string& title, const string& band, const int nSongs)
{

CD* item = new CD(title,band,nSongs);

allCDS.insert(item);

//ItemSetMap::iterator myband = allMusicByBand.find(band);

//if(myband != allMusicByBand.end())
//{
    //myband->second->insert(item);

//}
//else{
    //ItemSet* obj = new ItemSet();
    //obj->insert(item);
    //allMusicByBand.insert(make_pair(band, obj));
//}




return item;
    }


const ItemSet* Library::musicByBand(const string& band) const
{



return allMusicByBand[author];
}

i hope i was clear enough on what i wanted.

I have tried to iterate through it. I have tried just about everything i can think of.. CD class is a superclass of item class.

Thank you..

A: 

The simplest way to do it would be this:

const ItemSet* Library::musicByBand(const string& band) const
{
    ItemSet* bandMusic = new ItemSet();

    for (ItemSet::const_iterator i = allCDs.begin(); i != allCDs.end(); ++i)
    {
      if ((*i)->getBand() == band) {
        bandMusic->insert(*i);
      }
    }

   return itemSet;
}

Although this runs in O(n) time, which doesn't at all take advantage of the fact that you are using a set. These could just as well be in a vector. The way you were doing it before with the "index" sets is actually a faster-performing solution, although it will take somewhat more memory. Plus, presumably the retrieval methods will be called far more frequently than the insertion methods, so it makes sense to do more work on insertion in order to save on work during retrieval. But of course if you do this, you will want the index sets to be private members, not globals.

You should also be very careful about your memory management here. The fact that you are returning a const pointer to an ItemSet from the musicByBand method concerns me. Why can it not be just an ItemSet that you are returning?

Tyler McHenry
thank you - i had to cast CD class since it is a superclass of item
icelated
The better way to handle that would be to make `allCDs` be a set of `CD*` rather than a set of `Item*`. This would also enforce at compile time the restriction that things that aren't `CDs` don't go into the `allCDs` set.
Tyler McHenry
A: 

This is a sample code which uses the functor with std::find_if algorithm to search for a particular element in the set

struct BandComparison : public std::binary_function<Item*, std::string, bool>
{
public:
    bool operator()(Item* pItem, const std::string& bandName) const
    {
        bool equal = false;
        CD* pCD = dynamic_cast<CD*>(pItem);
        if(pCD)
        {
            equal = pCD->getBand() == bandName;
        }
        return equal;
    }
};

void Library::addCD(const std::string &band)
{
    //First check whether CD with this band name exists
    ItemSet::iterator iter = std::find_if(m_cds.begin(), m_cds.end(), std::bind2nd(BandComparison(), band));
    if(iter == m_cds.end())
    {
        m_cds.insert(new CD(band));
    }
}
Naveen
Note that this is a more properly object oriented approach than what I wrote, but it isn't any faster.
Tyler McHenry
Yes, I agree. In fact, I hate those functors with binders. It can be written this much more elegantly using `boost`. But I thought of not using `boost` here as the OP doesn't seem to use it.
Naveen
+1  A: 

An "idiomatic" way to do it might be to use the std::remove_copy_if algorithm . It would look something like this:

class NotMatching {
   string bandName_;
public:
   NotMatching( const string& band ) : bandName_( band ) {}
   bool operator()( const Item& item ) {
      return item.bandName() != bandName_;
   }
};

const ItemSet musicByBand(const string& band)
{
   ItemSet matchingItems;

   std::remove_copy_if( allCDS.begin(), allCDS.end(),
      insert_iterator< ItemSet >( matchingItems, matchingItems.begin() ),
      NotMatching( band ) );

   return matchingItems;
}

But to be honest I think Tyler's approach is simpler and clearer.

Jay Walker