4
\$\begingroup\$

I have created a contact management class for practicing, and I want to know your opinion about it. I know there are a lot of example of contact management class, but I tried to make this one a bit different by adding some functionality to avoid initializing a contact with wrong data. For example, if you try to initialize the phone number of a contact with "asdf" a warning or error will arise. Do you think the method I used for verification of the input data is good? If no, what would you do.

I put an example of using the class(demonstration) and also the header and the definition of the class data member and member functions respectively.

I am very open to any suggestion about my implementation, and thanks in advance.

Here is an example or demonstration of my class:


///IMANIPOUR Meysam
/// comment lines start with 3 forward slash.(///).
/// if the line contian 2 forward slash it could be run.

#include <iostream>
#include "Contacts_Manager.h"

int main(int argc, char* argv[])
{

    if(argc < 2)
    {
        std::cerr << "phonebook path has not specified. please specify it as second argument. " << "\n";
        return EXIT_FAILURE;
    }
    if(argc > 2)
    {
        std::cerr << "extra command argument is provided\n";
        return EXIT_FAILURE;
    }
    ///creating the object of the class.
    //Contacts_Manager cm(argv[1]);
   
    ///4 different ways of initializing new contact.
    //cm.newcontact("ali","12345","ali@mail.com");
    //cm.newcontact(NPE("hassan","23456","hassan@hotmail.com"));
    //cm.newcontact("hossein","34567");
    //cm.newcontact(NPE("sadjad","45678"));
    
    ///wrong data input and warning handling.
    //cm.newcontact("@name","09876");// wrong name warning will arise.
    //cm.newcontact("name","j9382");// wrong phone number warning will arise.
    //cm.newcontact("name","84989","test"); // wrong email warning will arise.
    //cm.newcontact("ali","28913","anemail@gmail.com");// duplicated contact info warning will arise.

    ///removeing an existing member of phone book.
    //cm.removecontact("sadjad"); // it could be by phone number like -> cm.removecontact("45678") 
    
    ///warning for non existing data.
    //cm.removecontact("notexistdata"); // warning will arise saying this info doesn't exist to be removed.
   
    /// edit contacts of the phone book. 
    //cm.edit("ali","Ali");//will edit contact with name ali to be Ali.
    //cm.edit("Ali","11111");//will edit will name Ali to have a new phone number like"11111".
    //cm.edit("sadjad","NewSadjad","99999");//editing contact named sadjad to a new neme and new phone number
    //cm.edit("23456","newemail@hotmail.com");//editing a contact with the given phone numbe to have the new email(2nd arugment).
    //cm.edit("hossein","newname","98989","Newemail@server.com");
    
    ///if the given new data for editing are not correct(for example phone without digits) then there will be warning.
    
    ///print the info of a contact with giving one data of that contact. 
    //cm.printinfo("NewSadjad");
   
    ///remove all the contact 
    //cm.removeall();
    return 0;
}   

Here is the declaration(*.h file) :


#ifndef CONTACTS_MANAGER_H
#define CONTACTS_MANAGER_H

#include <iostream>
#include <string>
#include <set>
#include <fstream>
#include <sstream>
#include <locale>

struct NPE{ // stand for Name Phone Email
    NPE(std::string _Name, std::string _Phone, std::string _Email = "not defined");
    std::string Name, Phone, Email;
};

class Contacts_Manager
{
public:
    typedef const std::string& strp;
    Contacts_Manager(strp _Dir);
    void newcontact(const NPE& _NPE);
    void newcontact(strp _Name, strp _Phone, strp _Email = "not defined");
    void removecontact(strp _CurrentData);//remove the contact which include _CurrentData
    void edit(strp _CurrentData, strp _NewData);//edit info of a contact which include _CurrentData with replacing _NewData(s).
    void edit(strp _CurrentData, strp _NewData1, strp _NewData2);
    void edit(strp _CurrentData, strp _NewData1, strp _NewData2, strp _NewData3);
    void printinfo(strp _SearchData);//it fill find a contact based _SearchData and print its info.
    void removeall();
private:
    std::string Dir;// path to phonebook, where we store contacts info.
    std::fstream Fio;
    std::set<std::string> tempo_phonebook;
    char whatdata(strp _Data);//return 'n' if _Data is name, return 'p' if _Data is Phone and 'e' if _Data is email. 
    int find(strp _Dataitem);//return the number of the line containing _Dataitem.(_Dataitem could be name, phone or email).
    
};

#endif

Here is the definition of function (*.cpp file)

#include "Contacts_Manager.h"

NPE::NPE(std::string _Name, std::string _Phone, std::string _Email)
{
    Name  = _Name;
    Phone = _Phone;
    Email = _Email;
};

Contacts_Manager::Contacts_Manager(strp _Dir)
{
    Dir = _Dir;
}

char Contacts_Manager::whatdata(strp _Data)
{
    
    if(_Data.find('@')!= std::string::npos||_Data == "not defined")
    {
        return 'e';
    }
    else if (std::isdigit(_Data[0]) == 1)
    {
        return 'p';
    }
    else
    {
         return 'n'; 
    }
}

int Contacts_Manager::find(strp _Dataitem)
{
    char datatype = this->whatdata(_Dataitem);
    int fline = 0;
    Fio.close();
    Fio.open(Dir,std::ios::in);
    std::string line;
    while(std::getline(Fio,line))
    {
        size_t firstcomma = line.find(',',0);
        size_t secondcomma= line.find(',',firstcomma+1);
        size_t res = line.find(_Dataitem);
        if(res != std::string::npos)
        {
            switch(datatype)
            {
                case 'n':
                    {
                        if(!line.compare(0,firstcomma,_Dataitem))
                        {
                             Fio.close();
                             return fline;
                        }
                    }break;
                case 'p':
                    {
                        if(!line.compare(firstcomma+1,secondcomma-firstcomma-1,_Dataitem))
                        {
                             Fio.close();
                             return fline; 
                        }

                    }break;
                case 'e':
                    {
                        if(!line.compare(secondcomma+1,line.length()-secondcomma-1,_Dataitem))
                        {
                             Fio.close();
                             return fline;                            
                        }
                    }break;
            }

        }
        fline++;
    }
    fline = -1;
    Fio.close();
    return fline;
}

void Contacts_Manager::newcontact(const NPE& _NPE)
{
    Fio.close();
    if (this->find(_NPE.Name) != -1)
    {
        std::cout << "this name is already exist." << "\n";
        return;
    }
    else if (this->whatdata(_NPE.Email) != 'e')
    {
        std::cout << "wrong email, basically email contiain '@' character." << "\n";
        return;
    }
    else if(this->whatdata(_NPE.Phone) != 'p')
    {
        std::cout << "wrong number, basically phone number should contain digits not letter" << "\n"; 
        return;
    }
    else if(this->whatdata(_NPE.Name) == 'e')
    {
        std::cout << "wrong name, basically name shouldn't contain '@' character." << "\n";
        return;
    }
    else
    {
        Fio.open(Dir, std::ios::app);
        Fio << _NPE.Name << "," << _NPE.Phone << "," << _NPE.Email << "\n";
    }
    return;
}

void Contacts_Manager::newcontact(strp _Name, strp _Phone, strp _Email)
{
    Fio.close();
    if(this->find(_Name) != -1)
    {
        std::cout << "this name is already exist." << "\n";
        return;
    }
    else if (this->whatdata(_Email) != 'e')
    {
        std::cout << "wrong email, basically email contiain '@' character." << "\n";
        return;
    }
    else if(this->whatdata(_Phone) != 'p')
    {
        std::cout << "wrong number, basically phone number should contain digits not letter" << "\n"; 
        return;
    }
    else if(this->whatdata(_Name) == 'e')
    {
        std::cout << "wrong name, basically name shouldn't contain '@' character." << "\n";
        return;
    }
    else
    {
        Fio.open(Dir,std::ios::app);
        Fio << _Name << "," << _Phone << "," << _Email << "\n";
    }
    return; 
}

void Contacts_Manager::removecontact(strp _CurrentData)
{
    int fline = this->find(_CurrentData);
    if(fline == -1)
    {
        std::cout << "can't find a contact with given info.\n";
        return;
    }
    Fio.close();
    Fio.open(Dir,std::ios::in);
    tempo_phonebook.clear();
    bool condition = true;
    std::string line;
    int cnt = 0;
    while(std::getline(Fio,line))
    {
        if(line == "")
           condition = false; 
        if(cnt != fline && condition)
        {
            tempo_phonebook.insert(line);
        }
        cnt++;
        condition = true;
    }
    Fio.close();
    Fio.open(Dir,std::ios::out|std::ios::trunc);
    for(auto i : tempo_phonebook)
    {
        Fio << i << "\n";
    }
    Fio.close();
}

void Contacts_Manager::edit(strp _CurrentName, strp _NewData)
{
    int fline = this->find(_CurrentName);
    if(fline == -1)
    {
        std::cout << "can't find a contact with given info." << "\n";
        return;
    }
    tempo_phonebook.clear();
    Fio.open(Dir,std::ios::in);
    std::string line;
    int cnt = 0;
    while(std::getline(Fio,line))
    {
        if(cnt == fline)
        {
            size_t firstcomma = line.find(',',0);
            size_t secondcomma= line.find(',',firstcomma+1);
            char datatype = this->whatdata(_NewData);
            switch(datatype)
            {
                case 'n':
                    {
                        line.replace(0,firstcomma,_NewData);
                    }break;

                case 'p':
                    {
                        line.replace(firstcomma+1,secondcomma-firstcomma-1,_NewData);
                    }break;
                case 'e':
                    {
                        line.replace(secondcomma+1,line.length()-secondcomma-1,_NewData);
                    }      
            }
            tempo_phonebook.insert(line);
        }
        else
        {
            tempo_phonebook.insert(line);
        }
        cnt++;
    }
    Fio.close();
    Fio.open(Dir,std::ios::out|std::ios::trunc);
    for(auto i : tempo_phonebook)
    {
        Fio << i << "\n";
    }
    Fio.close();
    return;
}
//I was thinking if I could use multiprocessing then I would invok two times edit function with _NewData 1 and 2.
void Contacts_Manager::edit(strp _CurrentData, strp _NewData1, strp _NewData2)
{
    if(this->whatdata(_NewData1) == this->whatdata(_NewData2))
    {
       std::cout << "Please enter two different types of data.\n";
       std::cout << "More detial -> " << _NewData1 << " and " << _NewData2 << "are both the same type.\n";
       return;
    }
    if(this->whatdata(_NewData1) != this->whatdata(_CurrentData))
    {
        this->edit(_CurrentData,_NewData1);
        this->edit(_CurrentData,_NewData2);
        return;
    }
    else if(this->whatdata(_NewData2) != this->whatdata(_CurrentData))
    {
        this->edit(_CurrentData,_NewData2);
        this->edit(_CurrentData,_NewData1);
        return;
    }
    else 
    {
        this->edit(_CurrentData,_NewData2);
        this->edit(_CurrentData,_NewData1);
        return;
    }
}

void Contacts_Manager::edit(strp _CurrentData, strp _NewData1, strp _NewData2, strp _NewData3)
{
    char input_types[3] = {this->whatdata(_NewData1),this->whatdata(_NewData2),this->whatdata(_NewData3)};
    if(input_types[0] == input_types[1] || input_types[0] == input_types[2] || input_types[1] == input_types[2])
    {
       std::cout << "Please enter different types of new data.\n";
       return;
    }

    if(input_types[0] != this->whatdata(_CurrentData))
    {
        this->edit(_CurrentData,_NewData1);
        if(input_types[1] != this->whatdata(_CurrentData))
        {
            this->edit(_CurrentData,_NewData2);
            this->edit(_CurrentData,_NewData3);
            return;
        }
        else if(input_types[2] != this->whatdata(_CurrentData))
        {
            this->edit(_CurrentData,_NewData3);
            this->edit(_CurrentData,_NewData2);
            return;
        }
    }

    if(input_types[1] != this->whatdata(_CurrentData))
    {
        this->edit(_CurrentData,_NewData2);
        if(input_types[0] != this->whatdata(_CurrentData))
        {
            this->edit(_CurrentData,_NewData1);
            this->edit(_CurrentData,_NewData3);
            return;
        }
        else if(input_types[2] != this->whatdata(_CurrentData))
        {
            this->edit(_CurrentData,_NewData3);
            this->edit(_CurrentData,_NewData1);
            return;
        }
    }
    
    if(input_types[2] != this->whatdata(_CurrentData))
    {
        this->edit(_CurrentData,_NewData3);
        if(input_types[0] != this->whatdata(_CurrentData))
        {
            this->edit(_CurrentData,_NewData1);
            this->edit(_CurrentData,_NewData2);
            return;
        }
        else if(input_types[1] != this->whatdata(_CurrentData))
        {
            this->edit(_CurrentData,_NewData2);
            this->edit(_CurrentData,_NewData1);
            return;
        }
    }

}


void Contacts_Manager::printinfo(strp _SearchData)
{
    int fline = this->find(_SearchData);
    if(fline == -1)
    {
        std::cout << "No contact has been found" << "\n";
        return;
    }
    Fio.close();
    Fio.open(Dir,std::ios::in);
    int cnt = 0;
    std::string line;
    while(std::getline(Fio,line))
    {
        if(cnt == fline)
        {
            size_t firstcomma = line.find(',',0);
            size_t secondcomma= line.find(',',firstcomma+1);
            std::cout << "Name  : " << line.substr(0,firstcomma) << "\n";
            std::cout << "Phone : " << line.substr(firstcomma+1,secondcomma-firstcomma-1) << "\n";
            std::cout << "Email : " << line.substr(secondcomma+1,line.length()-secondcomma-1) << "\n";
            return;
        }
        cnt++;
    }
}

void Contacts_Manager::removeall()
{
    Fio.close();
    Fio.open(Dir,std::ios::out|std::ios::trunc);
    Fio.close();
    return;
}

\$\endgroup\$
1

2 Answers 2

5
\$\begingroup\$

Names, phone numbers and email addresses are not unique

A lot of your code hinges on names, phone numbers and email addresses all being unique identifiers for a contact. However, none of these things are actually unique identifiers in the real world. There are many people who have exactly the same name, land lines are often shared by a family (but even mobile phones could be shared amongst a group of people), and even a single email address might be used by multiple people.

There are several ways this can be dealt with. One solution would be to create a unique identifier for contacts. That could be something as simple as a serial number that you create yourself. Another option would be to treat the combination of name, phone number and email address as a unique identifier, as it is extremely unlikely to have two distinct people with the exact same name, phone number and email address.

Regardless, consider that if you call find(), there might actually be multiple lines in your database that will match. Instead of just returning the first one, consider returning a list of matches.

Turn struct NPE into class Contact

NPE is now just a simple struct that holds three strings, it doesn't do anything else. All the logic surrounding it is in Contacts_Manager. However, consider putting all the logic for parsing, formatting and validating contact information into that struct. In fact, make it deal with everything regarding a single contact, and in that case it makes sense to rename it from NPE to Contact. This reduces the responsibilities of Contacts_Manager; the latter's task is just to manage a file containing Contacts. Consider:

class Contact
{
    std::string name;
    std::string phone;
    std::string email;

public:
    // Constructors
    Contact() = default;
    Contact(const std::string& name, const std::string& phone, const std::string& email = "not defined");

    // Setters
    void set_name(const std::string& name);
    void set_phone(const std::string& phone);
    void set_email(const std::string& email);

    // Getters
    std::string get_name();
    std::string get_phone();
    std::string get_email();

    // Compare it against something
    bool matches(const std::string& search_term);

    // I/O operations
    friend std::ostream& operator<<(std::ostream& os, const Contact& contact);
    friend std::istream& operator>>(std::istream& is, Contact& contact);
};

The actual strings are now private, and can only be set via the constructor or the setters. This allows these functions to validate that the name, phone number and email address provided are valid, and if not you can return an error in some way (for example, by throwing an exception). You can overload operator>>() and operator<<() to allow Contacts to be directly read from and written to a file. Now that all this logic is in Contact, Contacts_Manager's find() would be greatly simplified, possibly like so:

std::vector<Contact> Contacts_Manager::find(const std::string& search_term)
{
    std::vector<Contact> results;
    std::ifstream file(Dir);
    Contact contact;

    while (file >> contact) {
        if (contact.matches(search_term)) {
            results.push_back(contact);
        }
    }

    if (!file.eof()) {
        throw std::runtime_error("Error reading contacts");
    }

    return results;
}

Temporary variables should not be class members

Fio and tempo_phonebook are just temporary variables, they should not be member variables of Contacts_Manager. Instead, just declare them in functions where they are used. Member variables should only be used for things that need to be remembered between function calls.

Avoid creating aliases for standard types

You created the type alias strp. However, someone who is not familiar with your code might have a hard time figuring out what that means, unless they search your code to find how it is defined. The name is also misleading; a p usually means something is a pointer, however your strp is actually a reference, which is a big difference. It also doesn't hint that it is a const reference. I recommend just writing const std::string& fully everywhere you need it.

Unnecessary use of this->

You almost never have to write this-> in C++, I recommend that you just omit it.

Avoid repeating code

There is a lot of code repetition going on, especially in newcontact() and edit(). I would try to find some ways to avoid that. There are several ways to go about it. For example, in edit(), especially the overload that takes four parameters, you could write:

void Contacts_Manager::edit(strp _CurrentData, strp _NewData1, strp _NewData2, strp _NewData3)
{
    for (auto& NewData: {_NewData1, _NewData2, _NewData3}) {
         edit(_CurrentData, _NewData);
    }
}

Of course, that omits the check that all data items are of a different type, but there should be a similar way to do that check without too much repetition. However, in my opinion it would be even better to avoid creating these overly helpful functions, and just have explicit functions to change either the name, phone number or email address, like the set_name(), set_phone() and set_email() functions I added to the example class Contact above.

Consider also that Contacts_Manager should just deal with maintaining the file containing contact information, and should not have other responsibilities. Consider changing edit() with:

void Contacts_Manager::replace(const Contact& old_contact, const Contact &new_contact) {
    std::string tmp_filename = Dir + ".tmp";
    std::ifstream old_file(Dir);
    std::ofstream new_file(tmp_filename);
    Contact contact;

    while (file >> contact) {
        if (contact == old_contact) {
            new_file << new_contact;
        } else {
            new_file << contact;
        }
    }

    if (!old_file.eof()) {
        throw std::runtime_error("Error reading contacts");
    }

    new_file.close();

    if (new_file.fail()) {
        throw std::runtime_error("Error writing contacts");
    }

    // This requires C++17, but it's portable:
    std::filesystem::rename(tmp_filename, Dir);
}

However, the above requires two distinct Contact objects, even though they would refer to the same person. Ideally, you want one function that changes some aspect of a Contact and updates the database. Either you could have class Contact track which Contact_Manager it is part of, and have it call replace() whenever you call a set_*() function, or keep using the edit() functions of Contacts_Manager, but now write them like so:

void Contacts_Manager::edit_name(Contact& contact, const std::string& new_name) {
    Contact old_contact = contact;
    contact.set_name(new_name);
    replace(old_contact, contact);
}

Consider using SQLite to manage the file

The format of the file you are managing is very simple, but also quite efficient if you have lots of contacts. You have to scan linearly through the file to find something, and editing the file means rewriting it completely. You could improve performance yourself in various ways, but I recommend you use some library to manage the file for you. In particular, use SQLite. This will also teach you a bit about managing a database, and will force you to address issues such as the uniqueness of names, phone numbers and email addresses.

\$\endgroup\$
3
  • \$\begingroup\$ Is it really a good idea for the geters to return string on value? Returning a const reference would be beter. \$\endgroup\$
    – convert
    Feb 23 at 17:09
  • \$\begingroup\$ @convert For performance it would definitely be better, but it would also result in a stronger coupling between the public API and the private member functions. \$\endgroup\$
    – G. Sliepen
    Feb 23 at 20:09
  • \$\begingroup\$ By const reference I meaned const string&, which is very comon. \$\endgroup\$
    – convert
    Feb 23 at 21:03
4
\$\begingroup\$

Interface review

I would say that recompiling the program for every use case is not usable in the long term. It might be fun to do it once or twice, but it gets annoying quickly. The usual suggestion would be to create a command line based menus, but I have a different idea. Accept the contacts file path by environment variable, so that people could set it in their terminal's profile and not type it again as long as the path is valid. Then accept commands which follow very strict format, which you could later pass with regular expressions. If there was no match, just error out.

User Experience (UX) is a very important part of engineering. Most of the performance comes from optimizing the application from top to bottom (making common actions straightforward, allowing a save point before error prone actions, dividing complex actions into step by step process). That applies to code too - we can make correct usages easier and more intuitive than error prone ones, have concrete idea of what problem is solved and optimize for that, provide error reporting mechanism to facilitate bug hunting.

Code Review

struct NPE{ // stand for Name Phone Email
    NPE(std::string _Name, std::string _Phone, std::string _Email = "not defined");
    std::string Name, Phone, Email;
};

This could be inside the Contacts_Manager class. Although NPE makes sense standalone, its functionality is severely limited when considered without manager class. Also, I think Contact or ContactsEntry would be a better name to NPE.

To make an entry extensible, the code could use a std::map<std::string, std::string> instead of fixed fields. In constructors and modifying functions, the contacts entry object could verify correctness and integrity before saving. Whats more - you could actually save your contacts file in json or yaml formats so that you would no longer need to parse it by hand. There are a lot of libraries that provide support for parsing and formatting json and yaml.


whatdata is a bit weird. It has only three possible return values, yet the return type says there are lot more possible return values. It would be better to just have an enum:

enum class FieldType {
    Email,
    Phone,
    Name
};

That way, it is clear that there are only three possible values and nobody needs to know the constants ('n', 'p', 'e') for them.


There are also little things that together have serious consequences. One of them is manual file opening and closing. It is better to create a new object each time and save the needed one via moving it.

The other little thing is unclear overloads. The names of arguments of edit overloads don't make it clearer either. It is better to provide a proxy object that has overloaded operator= and operator std::string() so that the field could verified for correctness.

There are probably some more little things.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.