Contact management program

Question

I have made a simple contact management in C++.

It's my second project; I'm pretty sure it can be done more better and I could decrease its runtime, but I just joined the C++ community this year. So can you review my code?

It's a CUI-based contact management system through which you can add, delete, edit or search contacts.

#include <iostream>
#include <fstream>
#include <vector>
#include <sstream>
#include <conio.h>

//Used For Replacing The Line
void ReplaceLine(const std::string& ToReplace,const std::string&& Main){
    std::vector<std::string>Content;
    std::string s;
    std::ifstream File("contacts.txt");
    while(getline(File,s)){
        if(s==ToReplace){
            s=Main;
        }
        Content.push_back(s);
    }
    std::ofstream writer("contacts.txt",std::ostream::trunc);
    for(std::string& s:Content){
        writer<<s<<std::endl;
    }
}

//Editing The Contact
void EditContact(const std::string& ID){
    std::string NewName,NewNumber;
    std::cout<<"Enter A New Name : ";
    std::cin>>NewName;
    std::cout<<"Enter A New Number : ";
    std::cin>>NewNumber;
    NewName+=" ";
    ReplaceLine(ID,NewName+NewNumber);
}

//Adding New Contact
void AddContact(const std::string&& ID){
    std::ofstream File("contacts.txt",std::ios_base::app);
    std::string s;
    std::ifstream reader("contacts.txt");
    char Confirmation;
    while(getline(reader,s)){
            if(ID==s){
            std::cout<<"Contact Already Exist!\n"<<"Do You Want To OverWrite?[Y/N]"<<std::endl<<">>";
            std::cin>>Confirmation;
            if(Confirmation=='Y'){
                system("clear");
                EditContact(ID); 
                std::cout<<"Contacts Overriden"<<std::endl;
                system("pause");
                File.close();
                return;
            }
            else{
                std::cout<<"Contacts Aren't Touched"<<std::endl;
                system("pause");
                File.close();
                return;
            }
        }
    }
    File<<ID<<std::endl;
    std::cout<<"New Contact Has Been Added"<<std::endl;
    system("pause");
    File.close();
}

//Deleting A Contact
void DeleteContact(const std::string&& ToDlt){
    std::vector<std::string>Contact;
    std::string s;
    bool Deleted=false;
    std::ifstream Reader("contacts.txt");
    while(getline(Reader,s)){
        if(s==ToDlt){
            Deleted=true;
            continue;
        }
        Contact.push_back(s);
    }
    std::ofstream Writer("contacts.txt",std::ios_base::trunc);
    for(std::string&k : Contact){
        Writer<<k<<std::endl;
    }
    if(!Deleted){
        std::cout<<"Contact Didn't Existed!"<<std::endl;
    }
    else{
        std::cout<<"Contact Has Been Deleted"<<std::endl;
    }
}

//Searching A Contact This Wont Be Called Directly
int Search(const std::string& Query){
    int Count=0;
    bool IsNum=false;
    if(isdigit(Query[0])){
        IsNum=true;
    }
    std::ifstream reader("contacts.txt");
    std::string Name,Number;
    std::string s;
    while(getline(reader,s)){
        std::stringstream Extractor(s);
        Extractor>>Name>>Number;
        if(IsNum==true){
            if(Number.find(Query)!=std::string::npos){
                Count++;
                std::cout<<"NAME : "<<Name<<"   "<<"NUMBER : "<<Number<<std::endl;
            }
        }
        else{
            if(Name.find(Query)!=std::string::npos){
                Count++;
                std::cout<<"NAME : "<<Name<<"  "<<"NUMBER : "<<Number<<std::endl;
            }
        }
    }
    return Count;
}

//This Is Used To Take Inputs For Searching
void Query(){
    std::string Query="";
    std::cout<<"Contact Search"<<std::endl;
    std::cout<<">>";
    while(true){
        char s=getche();
        //If User Presses Enter Case It Worked Atleast for mine
        if(int(s)==10){
            return;
        }
        //Handling BackSpace Case Worked In Mine :)
        if(int(s)==127){
           if(Query.length()==0){
              Query=" ";
              std::cout<<"\b"<<"\b";
           }
           Query.erase(Query.end()-1);
        }
        //Else  Adding Character To Query
        else{
           Query.push_back(s);
        }
        system("clear");
        //The Contacts Get Printed In Search Itself
        int Searched=0;
        Searched=Search(Query);
        if(Searched==0){
            std::cout<<"No Results Found!!"<<std::endl;
        }
        std::cout<<">>"<<Query;
    }
    system("pause");
    system("clear");
}
void EditContact(const std::string&& ID){
    system("clear");
    std::ifstream Reader("contacts.txt");
    std::string S;
    bool Exist=false;
    std::vector<std::string>NewBlocks;
    while(getline(Reader,S)){
        if(S==ID){
            std::string Name,Num;
            std::cout<<"Enter The New Name : ";
            std::cin>>Name;
            std::cin.ignore();
            std::cout<<"Enter The New Number : ";
            std::cin>>Num;
            Name+=" ";
            NewBlocks.push_back(Name+Num);
            Exist=true;
            continue;
        }
        NewBlocks.push_back(S);
    }
    if(!Exist){
        std::cout<<"The Contact You Want To Edit Didn't Exist!!"<<std::endl;
        system("pause");
        return;
    }
    std::ofstream Writer("contacts.txt",std::ios_base::trunc);
    for(std::string& Val:NewBlocks){
        Writer<<Val<<std::endl;
    }
    std::cout<<"Contacts Has Been Edited!!"<<std::endl;
    system("pause");
}

int main()
{
    char Cmnd;
    while(true){
        std::cout<<"~Add A Conatct [1]\n~Delete A Contact [2]\n~Edit A Contact [3]\n~Search A Contact [4]\n>>";
        std::cin>>Cmnd;
        std::cin.ignore();
        if(Cmnd=='1'){
            system("clear");
            std::string Name,Number;
            std::cout<<"Enter Name : ";
            std::cin>>Name;
            std::cin.ignore();
            std::cout<<"Enter Number : ";
            std::cin>>Number;
            Name+=" ";
            AddContact(Name+Number);
            system("clear");
        }
        else if(Cmnd=='2'){
            system("clear");
            std::string Name,Num;
            std::cout<<"Enter The Number : ";
            std::cin>>Num;
            std::cin.ignore();
            std::cout<<"Enter Users Name : ";
            std::cin>>Name;
            Name+=" ";
            DeleteContact(Name+Num);
            system("clear");
        }
        else if(Cmnd=='3'){
            system("clear");
            std::string Name,Num;
            std::cout<<"Enter The Name : ";
            std::cin>>Name;
            std::cin.ignore();
            std::cout<<"Enter The Number : ";
            std::cin>>Num;
            Name+=" ";
            EditContact(Name+Num);
            system("clear");
        }
        else if(Cmnd=='4'){
            system("clear");
            Query();
            system("clear");
        }
        else{
            std::cout<<"Invalid Option!!";
            system("pause");
            system("clear");
        }
    }
}

Please let me know anything I can improve.

Answer 1

Your C++ code is very C-like. This is one of the cases where a class would be an obvious choice to structure your code. However, before that, I would like to point out a few things in your current code.

---

1. The const std::string&& Main should be const std::string& Main. A const rvalue reference is almost always semantically useless. Better yet, if you're using C++17 or newer, you can in most cases pass a std::string_view instead of const std::string&.

---

2. Use '\n' over std::endl. It can potentially impact performance (especially when performing file I/O). Here's a SO thread on that

---

3. Check whether a file stream is valid before writing or reading to it.

std::ifstream file("contacts.txt");
if(!file)
{
  // something went wrong, handle the error!
}

---

4. As a general rule, avoid system. It's can be inefficient, but more notably, is not portable. Your code will not work on Linux, for example, since pause is not a valid shell command in Linux.

---

5. You don't need to manually call File.close(). The file will be closed when the object fstream object is destroyed.

---

6. Follow the Single Responsibility Principle. In simple terms, it means a function should only do one thing. For example, your EditContact() function is responsible for a) opening the file, b) parsing the data, c) asking the user input and d) writing the data to the file. Break your functions into logical chunks.

---

7. You can use a switch statement, instead of if inside your main function. Also, as mentioned above, your main function is doing too much. You can easily move a lot of the statements inside their own functions.

---

8. As mentioned above, your code is pretty inefficient, since it's opening, parsing and writing a file almost every operation. File I/O is not cheap, and since you're using C++, you obviously care about performance.

So what options do you have?

a) Open, parse, update and close the file every time you want to do an operation on the file (this is what you're doing now). As mentioned above, this is not a very performant solution.

b) Open at the start, update the file every operation, and finally close the file at the end of the program. Slightly better, but writing to the file every time is still not very good.

c) Open at the start, store the data in memory, update the data that is in memory every time, and write to the file and close it at the very end. This is seemingly the best solution because memory access is orders of magnitude faster that file I/O.

Okay, so we go with option (c). How do we store the data in memory? You already did in one of your functions. Store it inside a std::vector. Now any operation you want to do is done on the strings in the vector.

We can create a class called CustomerManagementSystem.

(in pseudocode)

class CustomerManagementSystem
{
   CustomerManagementSystem(const std::string& str)
   {
         std::ifstream file(str)
         if(file is invalid)
         {
            // handle error
         }
         parseFile(file);
   }
}

We can provide a constructor that takes in file name, opens it and parses the file and store the data into a std::vector<std::string>. In fact, a better approach would be define a struct such as

struct CustomerInfo
{
   std::string name;
   int number;
};

and create a std::vector<CustomerInfo> to store the data.

When the constructor ends, the file is destroyed and the file is automatically closed.

Now every operation you want to do can be a member function of the class.

void AddContact(const std::string& ID)
{
    auto id = FindIdInVector(ID);
    if(id already exists)
    {
        UpdateID();
    }
    else
    {
       data.push_back(ID);
    }
}

So, how do we update the file in the end? We use a powerful C++ feature called Resource Acquisition Is Initialization. It's a scary name, but in very simple terms: your resource (for e.g. file handles, memory, sockets, et cetera) must be created (or "acquired") in the constructor and the resource is deleted (or "freed") (for e.g. calling delete on some memory allocated by using new, or closing a file object) inside the destructor.

This ensures that the resource is 'acquired' when the object is created, and 'freed' when the object goes out of scope. Not only that, but it also ensures clean up of all your resource if ever your program throws an exception.

So, how do we utilize RAII inside our class?

We already fulfilled the first half of the contract; in our constructor, we opened the file, and stored the data in memory.

So the second half of the contract is: in the destructor, we open the file and write the stored data back into it.

So, destructor will look like this:

~CustomerManagementSystem()
{
    std::ofstream file(filename);
    if(!file)
    {
       // file wasn't opened! handle error here
    } 
    writeToFile(file, data);
}

Answer 2

I would at least consider re-structuring the program a little, to read the data from a file into memory when it starts up, then manipulating the data in memory, without reading the file again (until the next time the user starts the program).

If you were going to do that, when you read it into memory, you could store the data into a std::map<std::string, std::string>. This would make it really quick and easy to look up a contact by name (probably the most common case).

std::map also sorts all the contents, so it would be trivial to add a few things like "list all my contacts in alphabetical order".

The other thing that seems like kind of a problem to me is that virtually all your code knows all about everything. Even the code in main knows all the details of things like exactly how the data is formatted in the file.

I'd rather define something like a structure that knows how to read and write the data in the file, and that's the only part that knows or cares about the file format.

Answer 3

This looks like it could be a great program, but it's needlessly platform-specific:

#include <conio.h>

If you could replace this with something more portable, then it would be much better. I recommend learning how to use the Curses library if you want to make CUI programs; see What is the equivalent to getch() & getche() in Linux?. That would also allow you to replace the (also non-portable) system() calls with more reliable functions.

I note that system hasn't even been declared - in C++ programs, you should include <cstdlib> and call it as std::system.

---

    if(Cmnd=='1'){
        ⋮
    }
    else if(Cmnd=='2'){
        ⋮
    }
    else if(Cmnd=='3'){
        ⋮
    }
    else if(Cmnd=='4'){
        ⋮
    }
    else{
        ⋮
    }

It's clearer to write this using a switch statement:

    switch (command) {
    case '1':
        ⋮
        break;
    case '2':
        ⋮
        break;
    case '3':
        ⋮
        break;
    case '4':
        ⋮
        break;
    default:
        ⋮
        break;
    }

Answer 4

You have a lot of duplicated code to open, read, and (for modifying operations) re-write your file. What happens if you update your file format? You'll have to change just about everything!

Others have said that you should suck in the entire file, and operate in memory. That would indeed give you a single place to Load and Save. On a modern machine, with the expected size of a contact file, that makes sense. Historically, reading one record at a time made sense, and might still be true if this were the contact list for a large company or something like that. But then again, they would be using a database system today.

If you want to keep the unit-record batch processing flow, you should at least isolate the record reading and writing functions for reuse.

It looks like you're reading in the whole file anyway, to rewrite it after a change. But I shouldn't have to read through the Delete function to figure that out!