Sorting a vector alphabetically

[coder0101]

In the function add contacts, I want to add a persons name, phone, and email in my vector my contacts. I want to add their name to keep the contacts alphabetically sorted in the function sort. Right now sort is buggy and not working as intended, does anyone know a solution as to how to sort mycontacts vector alphabetically when a new person is added to it?

#include <iostream>
using namespace std;
#include <vector>

//Holds Variables
struct contact{
  string name;
  string phone;
  string email;
  contact(){}
  contact(string n, string p, string e){
    name=n;
    phone=p;
    email=e;
  }
};

//Outputs current Contacts
void displayContacts(vector<contact>&mycontacts){
  for (int i=0; i<mycontacts.size(); i++){
    cout<<"Contact "<<i+1<<endl;
    cout<<"Name: "<<mycontacts[i].name<<endl;
    cout<<"Phone: "<<mycontacts[i].phone<<endl;
    cout<<"Email: "<<mycontacts[i].email<<endl;
  }
}

//Sorts vector lexographically
void sort(vector<contact>&mycontacts){
  int a=mycontacts.size();
  string added=mycontacts[a].name;
  for (int i=0; i<mycontacts.size(); i++){
    string first=mycontacts[i].name;
    for (int j=i+1; i<mycontacts.size(); j++){
      if (mycontacts[j].name<first){
        string first=mycontacts[j].name;
        mycontacts[i]=mycontacts[j];
        mycontacts[j]=mycontacts[i];
      } 
    }
  }
  displayContacts(mycontacts);
}

//Inserts new contact in order in the vector
void addContact(vector<contact>&mycontacts){
  string name;
  string phone;
  string email;
  cout<<"Name: ";
  cin.ignore();
  getline(cin, name);
  cout<<"Phone: ";
  cin>>phone;
  cout<<"Email: ";
  cin>>email;
  //Adding contact alphabetically
  mycontacts.resize(mycontacts.size()+1);
  mycontacts.push_back(contact(name, phone, email));
  sort(mycontacts);
  cout<<"Contact Added"<<endl;
}


//Menu of Options
char menu(){
  char choice;
  cout<<endl<<"D- Display Contacts"<<endl;
  cout<<"A- Add Contact"<<endl;
  cout<<"U- Update Contact"<<endl;
  cout<<"R- Remove Contact"<<endl;
  cout<<"Q- Quit"<<endl;
  cin>>choice;
  return choice;
}

int main() {
  char choice;
  vector<contact>mycontacts={contact("bob", "240-740-1509", "[e-mail]"), contact("joe", "240-740-1527","[e-mail]"), contact("paul", "240-740-1527", "[e-mail]")};
  while (choice!='Q'){
    choice=menu();
    if (choice=='D'){
      displayContacts(mycontacts);
    }
    else if (choice=='A'){
      addContact(mycontacts);
    }
  }
}

[Handy Andy]

Hello coder0101,

Please make you r code easier to read. It will speed response time.

It is late for me, so the best I can do for now is direct you to the comments in the code. And BTW this is much easier to read.

#include <iostream>
#include <string>  // <--- Missing "<string>".
#include <vector>
#include <cctype>  // <--- For "std::tolower() and std::toupper()" + others.

using namespace std;

//Holds Variables
struct contact
{
    string name;
    string phone;
    string email;
    contact() {}
    contact(string n, string p, string e)
    {
        name = n;
        phone = p;
        email = e;
    }
};

//Outputs current Contacts
void displayContacts(vector<contact>&mycontacts)
{
    for (int i = 0; i < mycontacts.size(); i++)
    {
        cout << "Contact " << i + 1 << endl;
        cout << "Name: " << mycontacts.name << endl;
        cout << "Phone: " << mycontacts[i].phone << endl;
        cout << "Email: " << mycontacts[i].email << endl;
    }
}

//Sorts vector lexographically
void sort(vector<contact>&mycontacts)
{
    int a = mycontacts.size();

    string added = mycontacts[a].name;  // <--- A is the total elements of the vector. "mycontacts[a]" is 1 past the end of the vector.

    for (int i = 0; i < mycontacts.size(); i++)
    {
        string first = mycontacts[i].name;
        for (int j = i + 1; i < mycontacts.size(); j++)
        {
            if (mycontacts[j].name < first)
            {
                string first = mycontacts[j].name;
                mycontacts[i] = mycontacts[j];
                mycontacts[j] = mycontacts[i];
            }
        }
    }

    displayContacts(mycontacts);
}

//Inserts new contact in order in the vector
void addContact(vector<contact>&mycontacts)
{
    string name;
    string phone;
    string email;
    cout << "Name: ";

    cin.ignore();

    getline(cin, name);

    cout << "Phone: ";
    cin >> phone;

    cout << "Email: ";
    cin >> email;

    //Adding contact alphabetically
    //mycontacts.resize(mycontacts.size() + 1);
    //mycontacts.push_back(contact(name, phone, email));
    mycontacts.emplace_back(name, phone, email);  // <--- Will construct an object of the struct and add it to the vector.

    sort(mycontacts);

    cout << "Contact Added" << endl;
}


//Menu of Options
char menu()
{
    char choice;
    cout << 
        "\n"
        "D- Display Contacts\n"
        "A- Add Contact\n"
        "U- Update Contact\n"
        "R- Remove Contact\n"
        "Q- Quit\n"
        " Enter choice: ";
    cin >> choice;

    return choice;
}

int main()
{
    char choice{};  // <--- [b][i]ALWAYS initialize all your variables.[/b]
    vector<contact>mycontacts
    {
        { "bob", "240-740-1509", "[e-mail]" },        
        { "joe", "240-740-1527","[e-mail]" },
        { "paul", "240-740-1527", "[e-mail]"}
    };

    sort(mycontacts);  // <--- Used for testing.

    while (choice != 'Q')  // <--- What about 'q'?
    {
        choice = menu();

        choice = static_cast<char>(std::toupper(static_cast<unsigned char>(choice)));

        if (choice == 'D')  // <--- What about 'd'?
        {
            displayContacts(mycontacts);
        }
        else if (choice == 'A')  // <--- What about 'a'?
        {
            addContact(mycontacts);
        }
    }
}

In the "sort" function, use a better name than just "sort" as there is a "std::sort", the outer for loop should go to mycontacts.size() - 1. The inner for loop is fine.

In the morning I will cover any questions that you have.

Andy

[jlb]

You may want to consider using std::multiset instead of std::vector since the multiset will keep the set sorted properly if you overload the proper operator< for the structure.

You should also get used to using initialization lists with your constructors, and you should also consider passing complicated classes like std::string via reference to avoid unnecessary copying.

[dhayden]

                string first = mycontacts[j].name; // save the old name, but you don't do anything with it.
                mycontacts[i] = mycontacts[j];     // Replaces the old contents of mycontacts[i], throwing away whatever was there.
                mycontacts[j] = mycontacts[i];     // Since you copied mycontacts[j] to mycontacts[i], this just copies the same data back, resulting in no change. 

You want to do:

contact tmp = mycontacts[i];  // save the entire contact, not just the name
contact[i] = contact[j];
contact[j] = tmp;  // copy old contents of contact[i] to contact[j] 

Finally, in the real world, you would do:

#include <algorithm>
...
std::swap(contacts[i], contacts[j]);  // The std library does it very efficiently 

[Handy Andy]

Hello coder0101,

After working on your code and figuring out the sort function this is what I have come up with.

As a start you need more header files than just "iostream". Your IDE may be letting you define string name, from the struct, but when you get to a "cout" statement the tries to pint the string it does not know what to do without the "string" class defined in the "string" header file. Also "std::getline" will generate an error as being undefined.

I find that these includes tend to be needed by most programs:

#include <iostream>
//#include <iomanip>  // <--- For "std::fixed", "std::setprecision()" and "std::setw()" + others.
#include <limits>
#include <string>  // <--- Missing "<string>".
#include <vector>
#include <cctype>  // <--- For "std::tolower() and std::toupper()" + others.

using namespace std;  // <--- Best not to use. 

Also the using statement normally comes after all the includes not in between, but really makes no difference, just looks better. But as the comment says you should avoid this using statement. Better to learn how to qualify what is in the "std" name space now then all at once later.

For your struct:

struct Contact
{
    string sName;
    string sPhone;
    string sEmail;

    Contact() {}
    Contact(string name, string phone, string email) : sName(name), sPhone(phone), sEmail(email) {}
};

Starting the name of the struct is not mandatory, but very helpful. This also applies to classes.

Defining the variable starting with an "s" is not necessary, but also can be very helpful as demonstrated in the overloaded ctor.

Based on jlb's suggestion I used the initialization list, but either way works. This also demonstrates how a good variable name, something more than a single letter, can make the code easier to read and understand.

Notice there is no (;) after the {}s.

In the "displayContacts" function you do not need a "cout" statement for every line. That is what the insertation operator, (<<), is for. The code would be:

void displayContacts(const vector<Contact>& mycontacts)  // <--- Changed. Added the "const".
{
    std::cout << '\n';

    for (size_t i = 0; i < mycontacts.size(); i++)
    {
        cout 
            << " Contact " << i + 1 << '\n'
            << "  Name: " << mycontacts[i].sName << '\n'
            << " Phone: " << mycontacts[i].sPhone << '\n'
            << " Email: " << mycontacts[i].sEmail << "\n\n";
    }
}

I added the "const" here because the vector is better passed by reference. The "const" means you can use the the vector, but can not accidentally change anything.

This also has the advantage of the code looking more like what is displayed on the screen. To that end adding an extra space before "name" lines up the (:)s for a better looking display. Adding line 3 means that your output will not all run together line after line.

The "sort" function is the biggest problem. Consider calling the function "SortVec". Not only is it more descriptive, but less chance of it being confused with "std::sort". Since you are using the line: using namespace std; the compiler will look for "std::sort" first. If it finds it you could have a problem.

Inside your function you have:

int a = mycontacts.size();
string added = mycontacts[a].name;
for (int i = 0; i < mycontacts.size(); i++)
{
    string first = mycontacts[i].name;
    for (int j = i + 1; i < mycontacts.size(); j++)
    {
        if (mycontacts[j].name < first)
        {
            string first = mycontacts[j].name;
            mycontacts[i] = mycontacts[j];
            mycontacts[j] = first;
        }
    }

Lines 1 and 2 make no sense. And line 2 is causing the program to crash because the ".size" function returns the number of elements in the vector. The returned number is of type "size_t" which is an "unsigned implementation-defined" integer type. Depending on your header files this could be an "int" or a "long". See https://en.cppreference.com/w/c/types/size_t

This means that mycontacts[a] the value of "a" is 1 past the end of the vector.

In your for loops, since the ".size" function returns a "size_t", both "i" and "j" should be defined as "size_t". This will eliminate the warnings from the compiler, but even if you do not change it the "int" will still work.

In the outer loop you do not want i < mycontacts.size(). This would end up making "j" start 1 past the end of the vector. The condition on the inner for loop is fine.

Inside the inner loop the if statement should be making use of both "i" and "j" since "i" would start at element (0)zero and "j" would start at element (1). Using that to compare 2 names is what you want.

Inside the if statement you want to swap complete objects. Line 10 has no place or use in the part of the code. And I believe you changed line 12 from what you originally posted because I remember line 12 trying to set an object to a "std::string", but you code no longer reflects this.

If you have changes to your code put them in another post. Otherwise your original post will look as if nothing is wrong then it makes it hard to understand what the thread is about.

You did make the proper correction to line 12.

Your sort should start something like this:

void SortVec(vector<Contact>& mycontacts)  // <--- Notice the space after the &.
{
    //int a = mycontacts.size() - 1;

    //string added = mycontacts[a].sName;  // <--- A is the total elements of the vector. "mycontacts[a]" is 1 past the end of the vector.

    for (size_t i = 0; i < mycontacts.size() - 1; i++)
    {
        Contact first;

        for (size_t j = i + 1; j < mycontacts.size(); j++)  // <--- Changed. "i < mycontacts"
        {
            if (mycontacts[j].sName < mycontacts[i].sName)

The commented lines are never used in the function.

Then inside the if statement you need to deal with objects not a combination of objects and "std::string"s.

In "addContact" it actually works down to mycontacts.resize(mycontacts.size() + 1);. Resizing the vector will make it 1 larger, but that last element will be an empty struct then the "push_back" will add 1 more to the end. Not what you want.

I ended up with this:

//Adding Contact alphabetically
//mycontacts.resize(mycontacts.size() + 1);
//mycontacts.push_back(Contact(name, phone, email));
mycontacts.emplace_back(name, phone, email);  // <--- Will construct an object of the struct and add it to the vector. 

The only other thing I would suggest is after you enter a name you should check that each element of the string is converted to a lower case letter. Otherwise the sort may not give you what you want.

In the "menu" function I added 1 line of code:

cin >> choice;

choice = static_cast<char>(std::toupper(static_cast<unsigned char>(choice)));  // <--- Added. 

This way when you return to "main" you will have a capital letter. to work with.

In "main" instead of the if/else if statements, BTW you are missing a final else to deal with any letter that does not match, a switch would be a better choice here.

Andy

[Handy Andy]

@dhayden,

Out of this:

string first = mycontacts[j].name; // save the old name, but you don't do anything with it.
mycontacts[i] = mycontacts[j];     // Replaces the old contents of mycontacts[i], throwing away whatever was there.
mycontacts[j] = mycontacts[i];     // Since you copied mycontacts[j] to mycontacts[i], this just copies the same data back, resulting in no change.  

the original last line was mycontacts[j] = first; later changed with an edit.

I do like the suggestion of the "std::swap" That would really be something with the "sort" function OP has.

Andy

Edit: type