Database of names and scores

Question

Continuing the exercises in Chapter 4 of Programming Principles and Practice Using C++, speaking specifically about the exercises ranging from 4-19 to 4-21, we have:

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.

20. Modify the program from exercise 19 so that when you enter a name, the program will output the corresponding score or name not found.

21. Modify the program from exercise 19 so that when you enter an integer, the program will output all the names with that score or score not found.

Doing the exercises up to 21, since were requested changes to the code of exercise 19, I completely rewrote my original monolithic code, giving it a cleaner structure (I hope) that included all the required tasks. I just changed one thing from 19 that asks to generate an error for duplicate names. I preferred to add the score value to the first one entered and deleted the duplicates.

Following the advice of some of you given other previous posts, I provided a check for invalid values ​​acquired through the input (I did some research on the mechanism).

Some functions are derived from the function extraction refactoring automation of the IDE. They are very referenced, I liked them and left them like that, adapting them better to code also deleting the resulting header file.

It has to be said that parameterization, reference pointers so on are covered chapters much later in the book and also I would like to point out that as I have said other occasion I am not an expert, I do this for passion, it's not my job...I have a basic vision that comes from a course I did previously on C++, which started with a description of C with some elementary exercises with the addition of C++ implementations....so I've heard a little bit about these topics... Putting the exercises here at least, to avoid excessive insults.. it pushes me to try to do a little better.

#include <iostream>
#include <vector>
#include <string>
#include <limits>
// cin flushing function
void fluxInputError() 
{
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
}
// function for controlling exit from submenus
void checkExit(bool &stay) // exit function for internal menus
{
 while (true) 
 {
 std::cout << "\nDo you want to continue?(y/n)\n";
 std::string check{ 0 };
 if (std::cin >> check) // check if cin is valid
 {
 std::cout << "\nI'm inside\n";
 if (check == "y" || check == "Y")
 {
 stay = true; // 
 break;
 }
 else if (check == "n" || check == "N") 
 {
 stay = false; // this value is used to exit from evoking, not from here 
 break; // that use break
 }
 }
 else
 {
 fluxInputError();
 continue;
 }
 }
}
//data insertion function in the two vectors 
void insertData(std::string &name, int &score, std::vector<std::string> &names, std::vector<int> &scores)
{
 while (true) 
 {
 std::cout << "\nEnter a name and a score (enter 'NoName 0' when finished)" << '\n';
 if (std::cin >> name >> score) // check if cin is valid
 {
 if (name == "NoName" && score == 0) // as required by the exercise. I didn't use my custom checkExit function.
 {
 break;
 }
 names.push_back(name);
 scores.push_back(score);
 for (int i = 0; i < names.size(); ++i)
 {
 for (int x = 0; x < names.size(); ++x)
 {
 if (i != x && x < names.size() && names[i] == names[x])
 {
 //std::cout << "\nError! " << names[x] << " is entered twice\n\n";
 //names.erase(names.begin() + x);
 //scores.erase(scores.begin() + x);
 // instead of giving an error message for repeated names as asked from exercixe, 
 // I prefered add the score to the first name entered and remove duplicates
 names.erase(names.begin() + x);
 scores[i] += scores[x];
 scores.erase(scores.begin() + x);
 }
 }
 }
 }
 else
 {
 std::cout << "\nWrong input!\n";
 fluxInputError();
 continue;
 }
 }
}
// selection function
void askingWhatToDo(char& selection)
{
 
 std::cout << "\nWhat do you want to do now?\n\n";
 std::cout << "1) I want to insert some data into the database?\n";
 std::cout << "2) I want to search for a name in the database?\n";
 std::cout << "3) I want to search for a score in the database\n";
 std::cout << "4) I want to see all names and scores\n";
 std::cout << "5) I finished\n\n";
 std::cin >> selection;
}
// function used to search for values ​​by name
void lookinFornName(bool&stay, std::string &name, std::vector<std::string> &names, std::vector<int> &scores) //*
{
 while (stay)
 {
 std::cout << "\nEnter a name\n";
 if (std::cin >> name) 
 {
 bool isNotFound{ true };
 for (int i = 0; i < names.size(); ++i)
 {
 if (name == names[i])
 {
 std::cout << "The score of " << name << " is " << scores[i] << "\n";
 isNotFound = false;
 }
 }
 if (isNotFound) std::cout << "\nName not found\n";
 }
 else
 {
 std::cout << "\nWrong input!\n"; 
 fluxInputError();
 continue;
 }
 checkExit(stay); 
 }
}
// function used to search for values ​​by score
void lookingForScore(bool& stay, int& score, std::vector<std::string> &names, std::vector<int> &scores) //**
{ 
 
 while (stay)
 {
 
 std::cout << "\nEnter a score\n";
 if (std::cin >> score)
 {
 bool isNotFound{ true };
 for (int i = 0; i < scores.size(); ++i)
 {
 if (score == scores[i])
 {
 std::cout << names[i] << '\n';
 isNotFound = false;
 }
 }
 if (isNotFound) std::cout << "\nScore not found\n";
 
 }
 else
 {
 std::cout << "\nWrong input!\n"; 
 fluxInputError();
 continue;
 }
 checkExit(stay);
 }
}
// this function shows all the (name, score) pairs present in the two vectors.
void allNamesAndScores(std::vector<std::string> &names, std::vector<int> &scores)
{
 if (names.size() > 0)
 {
 std::cout << "\nNames and scores are; " << "\n\n";
 for (int i = 0; i < names.size(); i++)
 {
 std::cout << names[i] << ',' << scores[i] << '\n';
 }
 }
 else
 { 
 std::cout << "\nThe database is empty\n";
 }
}
int main()
{
 //4 - Exercise - 21)
 
 std::vector<std::string> names;
 std::vector<int> scores;
 std::string name{ 0 };
 int score{ 0 };
 bool isFinished{ false };
 while (!isFinished)
 {
 bool stay{ true };
 char selection{ 0 };
 askingWhatToDo(selection);
 switch (selection)
 {
 case '1':
 insertData(name, score, names, scores);
 break;
 case '2':
 lookinFornName(stay, name, names, scores);
 break;
 case '3':
 lookingForScore(stay, score, names, scores);
 break;
 case '4':
 allNamesAndScores(names, scores);
 break;
 case '5':
 isFinished = true;
 break;
 }
 }
 return 0;
}

• Chris: In insertData you have continue as the last statement evaluated in a while loop. This is extraneous as the loop was going to move to the next iteration anyway.
• RedM3tal: #Chris: you're right it is irrelevant. I absentmindedly inserted it similarly to the functions above where however I wanted to avoid the checkExit call (also there) . Careful observer! thanks much
• pacmaninbw: Please do not edit the question, especially the code, after an answer has been posted. Changing the question may cause answer invalidation. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered.
• RedM3tal: @pac: ah ok sorry. go Looking at the original I had left a debug message and badly copied the original on a line not covered in subsequent answers.

Answer 1

• void checkExit(bool &stay)

  A void function returning a value (via reference) is anti-idiomatic. Prefer returning a value explicitly: bool checkExit(void). This way, you don't need a strange looking stay variable, eg

   if (check == "y" || check == "Y") {
   return true;
   } else if (check == "n" || check == "N") {
   return false;
   }
  

• insertData works too hard. It pushes name and score only to erase them later on if a duplicate is found. Looking for the duplicate first make things easier:

   int x = indexByName(names, name);
   if (x == names.size()) {
   names.push_back(name);
   scores.push_back(0);
   }
   scores[x] += score;
  

  In any case, a nested loop is a big overkill. You know that only the newly entered name could be a duplicate. Testing for names[i] == names[x] for all pairs of names is a waste of time. And, once a dupe is found you may safely stop looking.

• The above bullet point hints that indexByName does deserve to be a function. It allow refactoring lookinFornName into

   int x = indexByName(names, name);
   if (x < names.size()) {
   std::cout << "Name not found\n";
   } else {
   std::cout << "The score of " << name << " is " << scores[x] << "\n";
   }
  

  Nitpick: it should be lookingForName.

• RedM3tal: @ vnp: First of all, thanks for your time. I thought the critical point was the nested for loop of InsertData. I got out of it like this but I knew it was incorrect and quite expensive. Instead, analyzing indexByName(names, name); what exactly does it do? is it a single for loop that finds name in names? in this case wouldn't names.size() returning the length of names always be an out of bounds index value of the vector?
• RedM3tal: Good to know that A void function returning a value via reference is anti-idiomatic. Speaking of the other functions present, regardless of their correctness, the IDE (VB) when I used the refactoring extraction automation created them like this, that is, the void and reference pair as return parameters. In this case, what was preferable to do? Use this technique only when you also have an explicit return value? Thanks again.
• vnp: @RedM3tal indexByName indeed finds name in names and returns its index if found. Otherwise, it returns names.size(), which is the invalid index, so the caller knows that the name was not found.
• vnp: @RedM3tal Regarding reference pair as a return parameter, I'd prefer to return a struct, or perhaps an std::pair. Since c++17, structured binding wins hands down.
• RedM3tal: @vpn: nice technique to use the vector size() to validate any elements found. To be extended to all similar cases. Thanks for sharing!

Answer 2

Parallel data structures

The approach you're using right now is "parallel" vectors of data representing linked pieces of data. This is both fragile and not very scalable. You have to ensure that any changes in one vector are reflected in the parallel vector, but the data structure does nothing to assist you with that. You're doing all of the heavy lifting.

Inverting this

The first approach to improve this is to use a single vector of a struct that combines both linked pieces of data.

struct Record {
 std::string name;
 int score;
};

Now you can search for a record where the name matches an input and you'll have the score without having to index into a second vector which we hope is actually parallel. This is sometimes referred to as an "association list."

Maps

But, if we know that names must be unique, we can actually use them as the key in a map data structure like std::unordered_map which does even more of the work for you.

A single, simple example:

void insertData(
 std::string &name, int &score, 
 std::unordered_map<std::string, int> &database
)
{
 // Test to see if the entry already exists and
 // avoid overwriting existing data.
 if (database.find(name) != database.end()) return;
 database[name] = score;
}

An additional example demonstrating a range-based for loop with a structured binding.

void allNamesAndScores(const std::unordered_map<std::string, int>& database)
{
 // Early return if there's no data to print.
 if (database.empty()) return;
 std::cout << "\nNames and scores are; " << "\n\n";
 for (auto &[name, score] : database) {
 std::cout << name << ',' << score << '\n';
 }
}

Other concerns

You also want to separate input of information from the user from functions like insertData. Let each function do one job. In several places you take name as an argument, then immediately read into it from standard input. If you didn't care about the value of name, why take it as an argument?

Worse, because name is a reference your function is modifying the state of a variable outside of the scope of your function. This is going to make debugging much harder.

C vs. C++

Among the many differences between C and C++ is a larger standard library full of versatile data structures. Learning to use it is critical.

• G. Sliepen: Good point about std::unordered_map being the right tool for the job. I guess the problem specifically calls for a vector to be used, and the goal might be to teach students about working with vectors, but it would have been much better if the problem would actually be written such that a vector would be the right data type. Now students will be left thinking "what a waste of time learning about std::vector when there are so much better types".
• RedM3tal: @ G.Slieprn. You nailed it perfectly. The exercise asked you to modify another one that originally planned to use two vectors. As I specified at this level of the text, the tools made available are barely the basic types, vectors, conditionals (if, switch) and iterations and little more. Having used references I would already be off topic. I also believe that the intention is to make you untangle the indices of multiple vectors through for loops while maintaining control of the data.
• RedM3tal: @ Chirs: you are right both name and score being variables that are consumed inside the functions, could have been simply local variables. Wanting to use references as arguments, the only two were just the two vectors with whose content you deal with in all the functions present. The refactoring automation had created the functions so referenced. Thanks for pointing this out to me, I realized to be more careful when using this tool and work better with the variables without waste