Simple address book in C++

Question

I am trying to increase my understanding of classes, so I tried to implement a simple address book program that holds ten names with that names corresponding address. The program works correctly and I am proud of what I have done so far, but I am looking to see if there is any aspect that can be made better, and or needs to be changed.

Class:

#pragma once
#include <iostream>
#include <string>

class Person
{
    friend std::istream &read(std::istream &is, Person add_book[10]);
    friend std::ostream &print(std::ostream &os, const Person add_book[10]);

public:
    Person& Person::sort_addbook(Person add_book[10]);
    Person();
    ~Person();
private:
    std::string name;
    std::string address;
};

Member functions:

#include "Person.h"

std::istream &read(std::istream &is,Person add_book[10]) {
    for (int count = 0; count < 10; count++) {
        is >> add_book[count].name; // get name
        std::getline(is,add_book[count].address); // get address from input
    }
    return is;
}

std::ostream &print(std::ostream &os, const Person add_book[10]) {
    for (int count = 0; count < 10; count++) {
        os << "Name: " << add_book[count].name << std::endl << "Adress: "
        << add_book[count].address << std::endl << std::endl; //output contents of address book
}
    return os;
}

//use bubble sort to sort address book alphabetically by name
Person& Person::sort_addbook(Person add_book[10]) {
    for (int pass = 0; pass < 10; pass++) {
        for (int i = 0; i < 9; i++) {
            if (add_book[pass].name < add_book[i].name)
                swap(add_book[pass].name, add_book[i].name);
        }
    }
    return *this;
}

Person::Person()
{
}


Person::~Person()
{
}

Main function:

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

/*create address book that stores name + address,
sort that adress into alphabetical order, print address book.*/

int main() {

    Person address_book[10]; //stores name in column 1, address in column 2

    std::cout << "Enter name, and then address: " << std::endl;
    read(std::cin,address_book); //get user input

    address_book[10].sort_addbook(address_book); //sort addressbook
    std::cout << std::endl;

   print(std::cout, address_book); //print contents of addressbook

   system("pause");
}

Answer 1

It's a good effort; the important thing is to keep practicing and keep learning. I see a number of things that may help you improve your program.

Don't overspecify member functions

The declaration of the sort_address member function in the header file is currently this:

Person& Person::sort_addbook(Person add_book[10]);

However, the Person:: is neither needed nor wanted here. Instead, it can and should be written like this:

Person& sort_addbook(Person add_book[10]);

That make it both easier to read and satisfies the compiler when you turn up the warning level.

Let the compiler generate code where it can

The Person constructor and destructor do nothing. The compiler could generate that for you, so you should let it. You can simply omit both the declaration and the implementations for both of those.

Eliminate "magic numbers"

This code includes "magic numbers," that is, unnamed constants such as 10. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "10" and then trying to determine if this particular 10 means the length of the array or some other constant that happens to have the same value.

Don't use #pragma once

Although it is supported by some compilers, code which is intended to be reused should avoid non-standard extensions. By definition, all #pragma are non-standard. For portable code, you should use the standard include guards. Even if you are only ever using one compiler at the moment, you will want to know the portable way of accomplishing this.

Don't use system("pausse")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. One simple replacement for that might be

void pause() {
    std::string nothing;
    std::cin >> nothing;
}

Prefer resizeable containers to fixed size

Rather than using a fixed size of 10, it may be preferable to use something like a std::vector.

Think carefully about object responsibilities

A Person is not an address_book. A Person is an individual, while an address_book represents a collection of Person objects. For that reason, the sort_addbook shouldn't really be a member function of the Person object. I'd recommend implementing an AddressBook object instead and having it include a sort method.

Use a stream operators where practical

As with the point above, I'd recommend that the Person object use the << and >> operators rather than read and print. For example, they might be implemented like this:

friend std::istream& operator>>(std::istream& in, Person &p) {
    in >> p.name; 
    std::getline(in, p.address); 
    return in;
}
friend std::ostream& operator<<(std::ostream& out, const Person &p) {
    return out << "Name: " << p.name 
        << "\nAdress: " << p.address << '\n';
}

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Prefer standard algorithms

Instead of writing your own sort, you could use the std::sort instead. To do that, you'll need to implement a comparison operator. Here's one way to do that, as a member function of the Person class:

bool operator<(const Person& other) const {
    return name < other.name;
}

Putting it together

Here's a way that this could be implemented. Note that this code does not use std::vector and keeps the fixed size, but it does use all of the other suggestions:

int main() {
    constexpr unsigned ADDRESS_COUNT{10};
    Person address_book[ADDRESS_COUNT]; 

    std::cout << "Enter name, and then address: " << std::endl;
    for (size_t i = 0; i < ADDRESS_COUNT; ++i) {
        std::cin >> address_book[i];
    }

    std::sort(&address_book[0], &address_book[ADDRESS_COUNT]);
    for (size_t i = 0; i < ADDRESS_COUNT; ++i) {
        std::cout << address_book[i] << '\n';
    }
}

Answer 2

Good start.

Your next step should be to separate Person and Address Book. These are completely different types and should be handled separately. Currently you are mixing the two into a single class and it causes some difficulties for expansion.

Some things you can try:

Place the array of People in its own class.

class AddressBook
{
    int       noOfEntries;
    People    Data[10];
    public:
        // STUFF (like adding/removing people)
 };

Validate that the read worked.

is >> add_book[count].name

This operation is allowed to fail. You can test for that by doing:

 if (is >> add_book[count].name)
 {
     // It worked
 }
 else
 {
      // It Failed
 }

Why is sort returning a value?

Person& Person::sort_addbook(Person add_book[10])

I personally would not expect sort to return a value.

Next Step:

Try again splitting into the two separate classes. A lot of the other problems I see will just go away. Post your code again for another review.