trjhtr

I am learning from Bjarne Stroustrup's book for beginners and I on Chapter 6 Exercise 4 as seen below. I am studying it alone and I need some reviews.

Are my comments any good?

Can you recommend me some improvements regarding comments or overusing functions?

The writer of the book has some solutions for his exercise and he doesn't use the put_in function for example.

Are global variables that bad? I used it when declaring vector<Name_value>pairs.

/*
4. Define a class Name_value that holds a string and a value. Rework exercise 19 in Chapter 4 to use a
vector<Name_value> instead of two vectors.

Chapter 4 : 19. Write a program where you first enter a set of name-and-value pairs, such as Joe 17 and Barbara 22. For each pair, add
the name to a vector called names and the number to a vector called scores (in corresponding positions, so that if
names[7]=="Joe" then scores[7]==17). Terminate input with NoName 0. Check that each name is unique and
terminate with an error message if a name is entered twice. Write out all the (name,score) pairs, one per line.
*/

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

const string print_names = "NoName";
const int print_val = 0;

//class of the name and corresponding value
class Name_value 
public:
 string Name; //the name of the person
 int value; //their respective score
 Name_value(string Na, int val) 
 :Name(Na), value(val) 
;
vector<Name_value>pairs; //the list of names and their corresponding values

//put the pairs in the vector
vector<Name_value> put_in(string x, int y) 
 pairs.push_back(Name_value(x,y));
 return pairs;


int main()
try

 string names;
 int scores;
 cout << "Enter 'NoName' and score '0' when you want to print out the results!n";
 cout << "Please enter some names and their scores (Ex.: Joe 22 Anne 53): ";
 while (cin) 
 cin >> names >> scores;
 if (!cin) error("You have not entered a string and then a value!n");
 if (names == print_names && scores == print_val) 
 cout << "The list of pairs entered: n";
 for (int i = 0; i < pairs.size(); i++) 
 cout << pairs[i].Name << 't' << pairs[i].value << 'n';
 
 break;
 
 for (int i = 0; i < pairs.size(); i++)
 
 if (names == pairs[i].Name) error("Duplicates found!n");
 
 pairs = put_in(names, scores); //put the values into the vector list
 

catch (exception&e)

 cerr << "error: " << e.what() << 'n';
 return 1;

• Yes, global variables really are that bad. Do try to avoid them.



• 
  

  Personally I find it easier to read if you add some vertical space to your member initializer list like so:

  
  
  

  Foo(int a, int b)
   : aa
   , bb
  
  

  
  



• Don't omit braces as that can lead to hard to find bugs (For example Apple's goto fail bug would have been much easier to spot if braces had been used.




• Prefer prefix over postfix



• 
  

  It's okay to use whitespace more leniently

  
  
  

  // This might be debatable
  pairs.push_back(Name_value(x,y));
  
  // But this just looks strange
  catch (exception&e)
  

  
  
  

  Considering that you add a space everywhere else in the code you might as well add one in those cases.

  
  



• Your naming could be improved. Namely put_in, x and y are not the greatest variable names. Also why is Name capitalized? Why names and scores when they only hold one name/score? Remember that variable names should always reflect intent as your goal is to write self-documenting code.



• 
  

  This code is confusing

  
  
  

  pairs = put_in(names, scores); //put the values into the vector list
  

  
  
  

  First get rid of that comment. Second, why are you returning the vector after you already added the values to it? Why even use a function for this in the first place? Simply doing:

  
  
  

  pairs.push_back(Name_value(names, scores));
  

  
  
  

  Has the same effect.

  
  



• While you didn't write it, you still use using namespace std included via the header file provided by Bjarne(?).
  
  Just a reminder that this is considered bad
  but hopefully it will be explained further into the book as well.




• I'm no expert on exceptions but the way you use them seems wonky, maybe someone else can chime in here.