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;
}
Edward
67.2k4 gold badges120 silver badges284 bronze badges
asked Mar 31, 2016 at 17:52
\$\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.

answered Mar 31, 2016 at 22:23
\$\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\$ Commented 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\$ Commented Apr 2, 2016 at 13:33

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.