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;
}
1 Answer 1
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.
-
\$\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– chris3602016年04月02日 06:03:43 +00:00Commented 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– Edward2016年04月02日 13:33:06 +00:00Commented Apr 2, 2016 at 13:33