5
\$\begingroup\$

This will be my third post of this program, I have received a lot of great feedback and have learned a lot from the community. I have edited this program a lot, and am here for another review to see if I got it correctly. I had a hard time implementing one minor fix with output stream delimiter which was suggested in an earlier answer, but I believe I fixed the inheritance issues. Any other fixes would be very appreciated:

Person.h

#ifndef PERSON
#define PERSON
#pragma once

#include <iostream>
#include <string>
#include <vector>
#include <algorithm>

class Person
{
    friend std::istream& operator>>(std::istream&,Person&);
    friend std::ostream& operator<<(std::ostream&, const Person&);
public:
    //comparison operator
    bool operator<(const Person&) const;
private:
    std::string name;
    std::string address;
};

#endif // !PERSON

Person.cpp

#include "Person.h"

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

std::istream& operator>>(std::istream &in, Person &p){
    in >> p.name;
    getline(in, p.address);
    return in;
}

std::ostream& operator<<(std::ostream& os, const Person &p) {
    os << "Name: " << p.name << "\nAddress: " << p.address;
    return os;
}

Address_book.h

#ifndef ADDRESS_BOOK
#define ADDRESS_BOOK
#pragma once

#include "Person.h"

class Address_book
{
    friend std::istream& operator>>(std::istream&, Address_book&);
    friend std::ostream& operator<<(std::ostream&, Address_book&);
public:
    //set vector size to match appropriate number of entries
    void Address_book::resize(size_t entries);
    void Address_book::sort();

private:
    std::vector<Person> add_book;
};

#endif

Address_book.cpp

#include "Address_book.h"

//fills address book
std::istream& operator>>(std::istream& in, Address_book& abook) {
    for (int i = 0; i < abook.add_book.size(); ++i) {
        std::cin >> abook.add_book[i];
    }
    return in;
}

//sort address book
void Address_book::sort() {
    std::sort(this->add_book.begin(), this->add_book.end());
}
//prints contents of address book
std::ostream& operator<<(std::ostream& os, Address_book& abook) {
    for (int i = 0; i < abook.add_book.size(); ++i) {
        std::cout << abook.add_book[i] << "\n";
    }
    return os;
}

void Address_book::resize(size_t entries) {
    add_book.resize(entries);
}

main.cpp

#include <iostream>
#include <vector>
#include <string>
#include "Address_book.h"
#include "Person.h"

void pause();

int main() {

    size_t entries = 0;

    std::cout << "Enter number of entries to go in addressbook: ";
    std::cin >> entries;

    Address_book abook1;
    abook1.resize(entries);

    //input into addressbook
    std::cout << "Enter the name, followed by the address:\n";
    std::cin >> abook1;
    //sort addressbook
    abook1.sort();

    //output addressbook
    std::cout << "\n";
    std::cout << abook1;

    pause();
}

void pause() {
    std::string pause;
    std::cout << "Press any key followed by enter to continue...";
    std::cin >> pause;
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

This code is greatly improved! Well done. Still, there are some things that may help you improve your code.

Don't overspecify member functions

Within the Address_book.h file, the member functions are declared like this

void Address_book::resize(size_t entries);
void Address_book::sort();

But they should not include the class prefix in this context. Instead, write them like this:

void resize(size_t entries);
void sort();

Be aware of signed vs. unsigned

Within the Address_book.cpp file, both functions include a line like this:

for (int i = 0; i < abook.add_book.size(); ++i) {

However, size() often returns an unsigned number, so it would be prudent to make i unsigned as well. I'd recommend std::size_t as in

for (std::size_t i = 0; i < abook.add_book.size(); ++i) {

Use const where practical

Usually, an extractor (operator <<) does not alter the underlying object. This is true of your code as well, which is good. But to make it explicit, declare the parameter as const. That is, the friend function should be this instead:

std::ostream& operator<<(std::ostream& os, const Address_book& abook);

Avoid the use of this in member functions

The code current contains this function:

void Address_book::sort() {
    std::sort(this->add_book.begin(), this->add_book.end());
}

However, it's usually not a good idea to explictly write this unless you have to. I'd recommend shortening to this:

void Address_book::sort() {
    std::sort(add_book.begin(), add_book.end());
}

Think of the user

It's not so convenient to count the addresses manually first and then enter them. Why not have the computer count them? One simple way to do that would be to have a blank line or end of file signal the end of the input, avoiding the need for the user to manually count addresses first.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your second look at my code, your time in answering is very appreciated! I have made the suggested adjustments, and I am working on simplifying it for the user as suggested. Thanks again for all your help! \$\endgroup\$
    – chris360
    Apr 2, 2016 at 6:03
  • \$\begingroup\$ If you found it helpful, you can upvote the answer as well as marking it "accepted." Personally, I find it gratifying to see code get improved. You're definitely on the right track! \$\endgroup\$
    – Edward
    Apr 2, 2016 at 13:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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