Prompting the user to enter Name value pairs and then printing it out using a Name_value class

Question

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;
}

• yuri: std_lib_facilities.h in case anyone wants to look at it.
• Incomputable: @VladarAkos, how the program terminates? Is there a way to gracefully exit rather than pressing X on the window or ctrl+C to kill it?
• Vladar Akos: @Incomputable Well after printing the result there is a break which prompts the user to enter any key to exit the console window. Is that a good option or should I do something else?
• Incomputable: @VladarAkos, oh, sorry, missed it. It is totally fine.

Answer 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)
   : a{a}
   , b{b}
  {}
  

• 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.

• Vladar Akos: Thank you for all your suggestions. They are very much appreciated. By omitting braces you mean when I have one declaration after an if then I should put braces as well? I haven't even heard of prefix over postfix until now so thank you for that. I don't really get your point about whitespace. So I should use more for readability? I know that using namespace std is bad but it is in the header provided so I cannot do much about until. The exception is indeed wonky. It is from the header aswell. The author provided it for some early exception handling. There a chapter on exceptions coming soon.
• Vladar Akos: Can I maybe edit my code above with your suggestions and ask for your feedback again? (if that isn't a problem of course)
• yuri: @VladarAkos No. Editing a question invalidates answers so you may not edit your question. You can however rewrite your program based on input from CR and then ask another question.
• Vorac: @VladarAkos about exceptions, you are just re-implementing the default behaviour. Try running int main() {const auto a = new int[99999999999999999];}