Constructive Feedback Needed

[Nathaniel10]

I did exercise 2) of chapter 9 in Stroustrup's book. That chapter is about classes, its members, enumerations, constructors, etc. This is my first attempt at writing a program with classes containing public and private members that are both data variables and functions. It compiled and ran correctly,BUT.

The exercise was a continuation of another exercise in an earlier chapter. That exercise only required writing functions, no classes. I thought I could cut and paste that program into this one and just adapt the class. I couldn't and that's where I need feedback. I was getting many compile errors saying that the subscript needed to be used with an array type. My IDE (MS VC++ Express) underlines errors and gives hints as to the solution. The solution was to write

vector<string>name;
function(vector<string>name);

instead of

vector<string>name;
function(string name);

In other words, my program seems to work correctly but I don't understand why.
This is the program.

/*
This program solves an exercise.  The requirement is to first create a class holding a vector of names of individuals and a vector 
of their ages as members.  Then write a function that reads the names into the vector, a function that reads their ages into the 
vector, a function that sorts the names alphabetically and also sorts the ages to remain matched to the names, and finally a 
function that prints out the sorted names and ages.  These functions should be members of the class.
*/

#include "../../std_lib_facilities.h"

class Name_Age {
   private:
	   vector<string>Names;
           vector<string>Names2;
 	   vector<double>Ages;
	   vector<double>Ages2;
	   string name;
	   string name2;
	   double age;
	   double age2;

   public:
	   void Read_names(void)
{
	   cout <<"Enter the names of individuals.  One name per line.\n";
	   cout <<"Enter 'last' to signal the end of the list.\n";
 
	   while (cin >> name) {
		 if (name == "last") {
			Read_ages();
			break;
		 } else {
		 Names.push_back(name);
		 }
	   }
}

	   void Read_ages(void)
{
	   cout <<"Enter the ages of the individuals.  One age per line.\n";
	   for (int i = 0; i < Names.size(); ++i) {
		   cin >> age;
		   Ages.push_back(age);
	   }
	   Sort(Names, Ages);
}

	   void Sort(vector<string>Names, vector<double>Ages)  
{
	   for (int k = 0; k < Names.size(); ++k) {       // A copy of Names, called Names2, is created in order that the unsorted 
		   name2 = Names[k];                      // order of the names can be used to match the ages to the sorted names.
		   Names2.push_back(name2);
	   }

  	   sort(Names2.begin(), Names2.end());

	   for (int i = 0; i < Names2.size(); ++i) {
		   for (int j = 0; j < Names2.size(); ++j) {
			   if (Names2[i] == Names[j]) {
				   age2 = Ages[j];
				   Ages2.push_back(age2);
			   }
		   }
           }
	   Print(Names2, Ages2);
}

	   void Print(vector<string>Names2, vector<double>Ages2)
{
	   for (int i = 0; i < Names.size(); ++i) {
  	      cout << i+1 <<"  "  << Names2[i] <<"  "  << Ages2[i] <<endl;
	   }
}
};

int main () 
{
	Name_Age Test;

	Test.Read_names();

        keep_window_open();
        return 0;
}

Thanks in advance for your comments.

[mike_2000_17]

In general it looks quite alright. Of course, I assume there will be future iterations on that problem in your book, because the instructions stated at the beginning will certainly be changed later to a more clever design. But I don't want to spoil it for you.

One mistake that is very important here is that you don't need to pass the vectors Names and Ages to all the functions you have. Since Sort and Print are member functions (called methods) they have access to the data inside the object of type Name_Age, so they can access the vectors Names and Ages directly without having to pass them as parameters.

Another improvement is to realize that those variables name, name2, age and age2 are used only locally inside the methods (to read in the values from cin). So you don't need those a data members, simply have them as temporary local variables inside the methods.

So a simple improvement on your code will result in this:

/*
This program solves an exercise.  The requirement is to first create a class holding a vector of names of individuals and a vector 
of their ages as members.  Then write a function that reads the names into the vector, a function that reads their ages into the 
vector, a function that sorts the names alphabetically and also sorts the ages to remain matched to the names, and finally a 
function that prints out the sorted names and ages.  These functions should be members of the class.
*/

#include "../../std_lib_facilities.h"

class Name_Age {
  private:
    vector<string>Names;
    vector<string>NamesSorted; //use more sensible names
    vector<double>Ages;
    vector<double>AgesSorted;

  public:
    void Read_names() //no need for (void), that looks a bit retarded.
    {
      cout <<"Enter the names of individuals.  One name per line.\n";
      cout <<"Enter 'last' to signal the end of the list.\n";
 
      Names.clear(); //Before filling a vector, you should clear it first.
      string name = "";
      cin >> name;
      while (name != "last") {
        Names.push_back(name);
        cin >> name;
      }
      Read_ages();
    }

    void Read_ages()
    {
      cout <<"Enter the ages of the individuals.  One age per line.\n";
      double age = 0.0;
      Ages.clear(); //Before filling a vector, you should clear it first.
      for (int i = 0; i < Names.size(); ++i) {
        cin >> age;
        Ages.push_back(age);
      }
      Sort();
    }

    void Sort()  
    {
      NamesSorted.clear(); //Before filling a vector, you should clear it first.
      for (int k = 0; k < Names.size(); ++k) // A copy of Names, called NamesSorted, is created in order that the unsorted            
         NamesSorted.push_back(Names[k]);       // order of the names can be used to match the ages to the sorted names.

      sort(NamesSorted.begin(), NamesSorted.end());

      AgesSorted.clear(); //Before filling a vector, you should clear it first.
      for (int i = 0; i < NamesSorted.size(); ++i) {
        for (int j = 0; j < Names.size(); ++j) {
          if (NamesSorted[i] == Names[j]) {
            AgesSorted.push_back(Ages[j]);
          }
        }
      }
      Print();
    }

    void Print()
    {
      for (int i = 0; i < NamesSorted.size(); ++i) {
        cout << i+1 <<"  "  << NamesSorted[i] <<"  "  << AgesSorted[i] << endl;
      }
    }
};

int main () 
{
  Name_Age Test;

  Test.Read_names();

  keep_window_open();
  return 0;
}

[Nathaniel10]

@mike,

Thank you very, much for your detailed comments. I carefully compared your code to mine and I think I understand what you called my mistake.

One mistake that is very important here is that you don't need to pass the vectors Names and Ages to all the functions you have. Since Sort and Print are member functions (called methods) they have access to the data inside the object of type Name_Age, so they can access the vectors Names and Ages directly without having to pass them as parameters.

When I called the sort function in line 44, I included arguments Names and Ages. Therefore, when I defined it in line 47, I needed to include those arguments in its definition. Hence the error message when my argument was a string rather than a vector string.

However, if I had not included an arguments when I called Sort or Print, the way I had when I called Read_ages in line 29, then I would not have needed to include any arguments when I defined those functions, as I did not have to do when I defined Read_ages in line 37. I don't remember why I decided to do that. But now I know and understand better.