Expert: Ralph McArdell - 10/13/2008

Question
hello sir,
i wanna insert in ma set datastructure 2 values.<char mac[],int p>.but i am not able to do it.since u also have to get and input and compare it in the enteries in ma set.for comaprison i have overloaded < operator to compare the char.i am stuck.i dont know how to proceed.now i want my program to get a set of <mac,p> values.insert in set.then get a comparison char value s.now compare s and mac.if they are equal return the <mac,p>  entry..kindly help me
#include<conio.h>
#include <iostream>
#include <string>
#include<set>
using namespace std;
class myset
{
public:
  enum{SZ=20};
  char mac_addr[SZ];
  int p;
public:
  myset()
  {
     strcpy(mac_addr," ");
  }
  myset(char s[])
  {
     strcpy(mac_addr,s);
  }
  void getstr()
  {
     cin >>mac_addr;
     cin >>p;
  }
  
  bool operator < (myset &ss) const
  {
     cout << "Using default comparison (operator<): ";
string str=this->mac_addr;
string str1=ss.mac_addr;
     return(str.compare(str1)==0) ? true:false;
 
  }
};
int main(int c,char* v[])
{
  set< myset > p1;
  myset s="ad.jh.89.qw.er";
myset s1;

  for(int i=0;i<5;i++)
  {
     cout<<"enter:";
      s1.getstr();
     p1.insert(s1);
  }

if(s < s1)
cout<<"they are equal";
//cout<<s.mac_addr;
else
cout<<"follow instructuction";
return 0;
}

Answer
Well, other than some rather odd design decisions (e.g. p is never initialised in constructors, mixed use of char array and std::string) the primary problem is the following:

   return (str.compare(str1)==0) ? true:false;

First the result of (str.compare(str1)==0) is a Boolean value anyway the ternary ? : operator is equivalent to saying:

   if (str.compare(str1)==0)
       return true;
   else
       return false;

Which is redundant. You can just say:

   return str.compare(str1)==0;

Secondly this is a _less_ _than_ operator and not an equality operator. The compare member function of std::string returns zero if the argument is lexographically equal to the string, a value less than zero if the argument is lexographically less than the string or a value greater than zero if the argument is lexographically greater than the string. You have therefore compared for mac_addr equality not mac_addr a being less than mac_addr b. From the above it follows that you should have written:

   return str.compare(str1)<0;
   
A simpler way with std::strings is to just use their < operator directly:

   return str < str1;
   
Note: that the C++ standard library associative containers such as std::set _need_ a proper ordering operation such as less than - and for it to actually produce an ordering of values so calling it operator< when it really does an equality test is going to end in tears.

However C-style strings have their own set of C library functions to operate on them. To use them you include <cstring>. To compare one C string with another we use the function strcmp, which works much like std::string::compare, except that we pass two pointers to two C-style, zero character terminated char arrays (remember the name of a built in array will convert to a pointer to the first item of the array) so you could also have just said:

   #include <cstring>      // replaces #include <string> if myset::operator< the only use of std::string

   // ...

       bool operator < (myset &ss) const
       {
           cout << "Using default comparison (operator<): ";
           return strcmp(this->mac_addr, ss.mac_addr) < 0;
       }

Oh, and as ss is never modified - pass it by const reference, not mutable reference thus:

       bool operator < (myset const & ss) const

We can also place the const before the type we have a reference to thus:

       bool operator < (const myset & ss) const

However I prefer the former as we can then deduce what type we have by reading things backwards. In this case if read backwards, and adding a little  English, we get "ss is a reference to a constant myset". If we try that with the second form it reads "ss is a reference to a myset that is constant" which I find confusing - which is constant the myset or the reference to it? It is not so much a problem with references which are always constant by definition so specifying a reference as constant is pointless and wrong (this would be myset & const ss: "ss is a constant reference to a myset"), but it is with pointers - which may or may not be constant e.g. int const * p: "p is a pointer to a constant int",  int * const p : "p is a constant pointer to int" and even int const * const * p : "p is a constant pointer to a constant int" are all valid C and C++ types.

Here are a few other points about your code I noticed:

1/ The include of conio.h is for a non-standard C/C++ header file. None of the things it defines/declares seem to be used. If so then remove it.
2/ The data member p is never initialised. At least set it to zero in your constructors:

       myset()
       : p(0)
       {
           strcpy(mac_addr," ");
       }

etc.

Maybe even add a second parameter to the other constructor that defaults to zero:

   myset(char s[], int v=0)
   : p(v)
   {
       strcpy(mac_addr,s);
   }



3/ Use of strcpy: you are lucky that some other header included <cstring>. This cannot be relied upon. Which headers other C/C++ library headers include is not standardised so your code may work for one implementation but fail with the compiler not knowing about strcpy for another.

4/ No checks on length of C-strings assigned to mac_addr member. If such values exceed SZ-1 characters (remember one slot is reserved for the terminating zero character for C-string) then your program will overwrite memory beyond that allocated for mac_addr - which may well be that used by the p member. This applies equally to the constructors and the the getstr member function.

5/ The name myset is not very clear - it is not a set type it is a type for items that are placed in a set.

6/ The name of the getstr member function seems somewhat misleading as it is the _only_ function that not only modifies the mac_addr string but also the p member.

7/ Using std::string for the mac_addr member would probably simplify your code:

   class myset
   {
   public:
       enum{SZ=20};            // could still be used to enforce maximum length constraints
       std::string mac_addr;
       int p;

   public:
       myset()  // mac_addr will be default constructed to an empty string
       : p(0)
       {
       }

       myset(std::string const & ma, int v=0 )
       : mac_addr(ma)
       , p(v)
       {
       }

       void getstr()
       {
           cin >>mac_addr;
           cin >>p;
       }

       bool operator < (myset const & ss) const
       {
           return this->mac_addr < ss.mac_addr;
       }
   };


8/ You are not using the command line argument parameters passed to main so use the form without these:

   int main()
   {
   // ...
   }

9/ The preferred form of object initialisation is "constructor call form", so rather than:

       myset s="ad.jh.89.qw.er";

we can write:

   myset s("ad.jh.89.qw.er");

Or even:

   myset s("ad.jh.89.qw.er", 23);

If you added a second int parameter to the value taking constructor.

10/ I do not understand why in main you test s for being less than s1 and if so display they are equal, nor why if this is not so you display "follow instructuction" when the only instruction given was "enter:"

10/ If you only intend to associate your set items by the mac_addr string then maybe use a std::map instead. In this case a std::map<std::string, int> would be sufficient. A map is like a set except it stores key, value pairs (as std::pair type objects). You can look up a key, value pair by its key value. In this case you would look up entries in a map by mac_addr string and obtain a std::pair<string, int> containing the mac_addr key and p integer value. Here is one example of a possible use of std::map similar to your requirements:

   #include <iostream>
   #include <string>
   #include <map>
   #include <utility>

   class MyMap
   {
   public:
       typedef std::string                   KeyType;
       typedef int                           ValueType;
       typedef std::pair<KeyType, ValueType> ItemType;

   private:
       typedef std::map<KeyType, ValueType>  MapType;
       MapType   map_;

   public:

       bool  AddItem( KeyType const & k, ValueType const & v )
       {
       // insert returns a std::pair of the position
       // and whether the insertion succeeded.
       // We are only interested in the success value.
         return map_.insert( std::make_pair(k,v) ).second;
       }

       bool Find( KeyType const & k, ValueType & v )
       {
         MapType::iterator pos( map_.find(k) );
         if ( pos != map_.end() )
         {
             v = pos->second;
             return true;
         }
         else
         {
             return false;
         }
       }
   };

   int main()
   {
       MyMap p1;
       p1.AddItem("ad.jh.89.qw.er", 0);

       for(int i=0;i<5;i++)
       {
            std::cout << "enter mac_addr value pair: ";
            MyMap::KeyType key;
            MyMap::ValueType value;
            std::cin >> key;
            std::cin >> value;
            if ( !p1.AddItem(key, value) )
            {
                 std::cerr << "Mac address duplicate, data not stored." << std::endl;
            }
       }

       std::cout << "Enter mac address to lookup: ";
       MyMap::KeyType searchKey;
       std::cin >> searchKey;
       MyMap::ValueType foundValue;
       if ( p1.Find( searchKey, foundValue) )
       {
           std::cout << "Found value " << foundValue << " for search key " << searchKey << std::endl;
       }
       else
       {
           std::cout << "No such mac address as " << searchKey << " in map." << std::endl;
       }
       return 0;
   }

I suggest that if you are going to be using the C++ standard library you obtain a good reference such as "The C++ Standard Library A Tutorial and Reference" by Nicolai M. Josuttis.

Hope these notes are of use.