cannot compile the following example code (person-pointer)

[=?ISO-8859-1?Q?Martin_J=F8rgensen?=]

Hi,

I don't understand these errors I get:

g++ Persort.cpp
Persort.cpp: In function 'int main()':
Persort.cpp:43: error: name lookup of 'j' changed for new ISO 'for' scoping
Persort.cpp:37: error: using obsolete binding at 'j'


- - - - -
#include <iostream>
#include <string> //for string class
using namespace std;
////////////////////////////////////////////////////////////////
class person //class of persons
{
protected:
string name; //person's name
public:
void setName() //set the name
{ cout << "Enter name: "; cin >> name; }
void printName() //display the name
{ cout << endl << name; }
string getName() //return the name
{ return name; }
};
////////////////////////////////////////////////////////////////
int main()
{
void bsort(person**, int); //prototype
person* persPtr[100]; //array of pointers to persons
int n = 0; //number of persons in array
char choice; //input char

do { //put persons in array
persPtr[n] = new person; //make new object
persPtr[n]->setName(); //set person's name
n++; //count new person
cout << "Enter another (y/n)? "; //enter another
cin >> choice; // person?
}
while( choice=='y' ); //quit on 'n'

cout << "\nUnsorted list:";
for(int j=0; j<n; j++) //print unsorted list
{ persPtr[j]->printName(); }

bsort(persPtr, n); //sort pointers

cout << "\nSorted list:";
for(j=0; j<n; j++) //print sorted list
{ persPtr[j]->printName(); }
cout << endl;
return 0;
} //end main()
//--------------------------------------------------------------
void bsort(person** pp, int n) //sort pointers to persons
{
void order(person**, person**); //prototype
int j, k; //indexes to array

for(j=0; j<n-1; j++) //outer loop
for(k=j+1; k<n; k++) //inner loop starts at outer
order(pp+j, pp+k); //order the pointer contents
}
//--------------------------------------------------------------
void order(person** pp1, person** pp2) //orders two pointers
{ //if 1st larger than 2nd,
if( (*pp1)->getName() > (*pp2)->getName() )
{
person* tempptr = *pp1; //swap the pointers
*pp1 = *pp2;
*pp2 = tempptr;
}
}

- - - - -

How to fix them?

With visual studio, I could go to msdn.com and look up all kinds of
different errors. Is there something like that for g++?


Best regards / Med venlig hilsen
Martin Jørgensen

 

[Phlip]

Persort.cpp: In function 'int main()':
Persort.cpp:43: error: name lookup of 'j' changed for new ISO 'for'
scoping
Persort.cpp:37: error: using obsolete binding at 'j'

Next time copy parts of the error message into Google - it should easily
find this one.

for(int j=0; j<n; j++) //print unsorted list ....
for(j=0; j<n; j++) //print sorted list

The ISO C++ Standard changed the scope of 'j'. It formerly lived in the
block around the first for loop. Now it lives only in this for-loop and its
controlled statement or block.

The second for loop now sees no 'j'. Instead of just barfing "unknown
identifier" or something, the g++ maintainers intercepted this particular
situation and gave you a complete error message describing the difference
between the old and new compiler behavior.

 

[Bob Hairgrove]

Hi,

I don't understand these errors I get:

g++ Persort.cpp
Persort.cpp: In function 'int main()':
Persort.cpp:43: error: name lookup of 'j' changed for new ISO 'for' scoping
Persort.cpp:37: error: using obsolete binding at 'j'

j is only in the scope of the first "for" loop. Declare it outside of
the loop or redeclare it within the second loop.

[snip]

for(int j=0; j<n; j++) //print unsorted list
{ persPtr[j]->printName(); }

j is now out of scope.

bsort(persPtr, n); //sort pointers

cout << "\nSorted list:";
for(j=0; j<n; j++) //print sorted list

for (int j=0; // etc.

{ persPtr[j]->printName(); }
cout << endl;
return 0;
} //end main()
//--------------------------------------------------------------
void bsort(person** pp, int n) //sort pointers to persons
{
void order(person**, person**); //prototype
int j, k; //indexes to array

These work OK because both counters are visible outside the loop
scopes.

for(j=0; j<n-1; j++) //outer loop
for(k=j+1; k<n; k++) //inner loop starts at outer
order(pp+j, pp+k); //order the pointer contents
}
//--------------------------------------------------------------
void order(person** pp1, person** pp2) //orders two pointers
{ //if 1st larger than 2nd,
if( (*pp1)->getName() > (*pp2)->getName() )
{
person* tempptr = *pp1; //swap the pointers
*pp1 = *pp2;
*pp2 = tempptr;
}
}

- - - - -

How to fix them?

With visual studio, I could go to msdn.com and look up all kinds of
different errors. Is there something like that for g++?

Have you tried:
man gcc

or:
man g++

?

 

[Alf P. Steinbach]

* Martin Jørgensen:

I don't understand these errors I get:

g++ Persort.cpp
Persort.cpp: In function 'int main()':
Persort.cpp:43: error: name lookup of 'j' changed for new ISO 'for' scoping
Persort.cpp:37: error: using obsolete binding at 'j'


- - - - -
int main()
{
for(int j=0; j<n; j++) //print unsorted list
{ persPtr[j]->printName(); }

bsort(persPtr, n); //sort pointers

cout << "\nSorted list:";
for(j=0; j<n; j++) //print sorted list
{ persPtr[j]->printName(); }

Here you need to write 'for( int j=0; ...', the scope of the previous
declaration of j ended at the end of that for-loop.

How to fix them?

Why not implement the program in C++ instead of C? <g>

E.g., preserving the program logic exactly (or as best I could see),


#include <algorithm> // std::sort
#include <iostream> // std::cin, std::cout
#include <istream> // operator>>
#include <ostream> // operator<<, std::endl
#include <string> // std::string
#include <vector> // std::vector

class Person
{
protected:
std::string myName;
public:
Person( std::string const& name ): myName( name ) {}
std::string name() const { return myName; }
};

std::string lineFrom( std::istream& stream )
{
std::string line;
std::getline( stream, line );
return line;
}

std::string nameFromUser()
{
std::cout << "Enter name: "; return lineFrom( std::cin );
}

bool userAffirms( std::string const& question )
{
char choice;

std::cout << question;
std::cin >> choice;
std::cin.ignore( INT_MAX, '\n' );
return (choice != 'n'); // Can be Really Improved.
}

void printNameOf( Person const& person )
{
// Original code's newline at front preserved, but why choose that?
std::cout << std::endl << person.name();
}

void printList( std::vector<Person> const& persons )
{
for( std::size_t i=0; i < persons.size(); ++i )
{
printNameOf( persons );
}
}

bool order( Person const& a, Person const& b )
{
return a.name() < b.name();
}

int main()
{
std::vector<Person> persons;

do
{
persons.push_back( nameFromUser() );
}
while( userAffirms( "Enter another (y/n)? " ) );

std::cout << "\nUnsorted list:"; printList( persons );
std::sort( persons.begin(), persons.end(), order );
std::cout << "\nSorted list:"; printList( persons );
std::cout << std::endl;
}

 

[=?ISO-8859-1?Q?Martin_J=F8rgensen?=]

* Martin Jørgensen: -snip-

cout << "\nSorted list:";
for(j=0; j<n; j++) //print sorted list
{ persPtr[j]->printName(); }

Here you need to write 'for( int j=0; ...', the scope of the previous
declaration of j ended at the end of that for-loop.
Thanks.

How to fix them?

Why not implement the program in C++ instead of C? <g>

E.g., preserving the program logic exactly (or as best I could see),


#include <algorithm> // std::sort
#include <iostream> // std::cin, std::cout
#include <istream> // operator>>
#include <ostream> // operator<<, std::endl
#include <string> // std::string
#include <vector> // std::vector

This is perhaps a newbie question, but why do you like to write std:: in
front of many of the commands?

class Person
{
protected:
std::string myName;

Why not private here?

public:
Person( std::string const& name ): myName( name ) {}
std::string name() const { return myName; }
};

std::string lineFrom( std::istream& stream )
{
std::string line;
std::getline( stream, line );
return line;
}

std::string nameFromUser()
{
std::cout << "Enter name: "; return lineFrom( std::cin );
}

What is this istream all about? You're saying something like:
std::getline ( (type istream), (type string) )?

bool userAffirms( std::string const& question )
{
char choice;

std::cout << question;
std::cin >> choice;
std::cin.ignore( INT_MAX, '\n' );
return (choice != 'n'); // Can be Really Improved.
}

What's that INT_MAX-thing doing? It's not defined anywhere here... Still
the program works.

void printNameOf( Person const& person )
{
// Original code's newline at front preserved, but why choose that?
std::cout << std::endl << person.name();
}

void printList( std::vector<Person> const& persons )
{
for( std::size_t i=0; i < persons.size(); ++i )
{
printNameOf( persons );
}
}

I haven't learned about vector's but will soon learn about it, so this
is an excellent tutorial code for me, in just 2-3 weeks

bool order( Person const& a, Person const& b )
{
return a.name() < b.name();
}

int main()
{
std::vector<Person> persons;

do
{
persons.push_back( nameFromUser() );
}
while( userAffirms( "Enter another (y/n)? " ) );

std::cout << "\nUnsorted list:"; printList( persons );
std::sort( persons.begin(), persons.end(), order );
std::cout << "\nSorted list:"; printList( persons );
std::cout << std::endl;
}

Looks very nice - thanks for posting it...


Best regards / Med venlig hilsen
Martin Jørgensen

 

[=?ISO-8859-1?Q?Martin_J=F8rgensen?=]

On Sat, 25 Mar 2006 17:37:20 +0100, Martin Jørgensen



Have you tried:
man gcc

or:
man g++

?

Come on. It looks like shit, with that formatting and the search
capabilites are poor. So much information and so hard to find what is
relevant.


Best regards / Med venlig hilsen
Martin Jørgensen

 

[Daniel T.]

Hi,

I don't understand these errors I get:

g++ Persort.cpp
Persort.cpp: In function 'int main()':
Persort.cpp:43: error: name lookup of 'j' changed for new ISO 'for' scoping
Persort.cpp:37: error: using obsolete binding at 'j'

Everyone else told you what would fix the error. I'm going to critique
your program.

In general, you have done an excellent job. The impression I get is that
your assignment was to write a bubble sort routine, and test it by
allowing the user to input some names, then sort and output them. If
this was not your assignment, then you probably still have some work to
do.

#include <iostream>
#include <string> //for string class
using namespace std;
////////////////////////////////////////////////////////////////
class person //class of persons
{
protected:
string name; //person's name
public:
void setName() //set the name
{ cout << "Enter name: "; cin >> name; }
void printName() //display the name
{ cout << endl << name; }
string getName() //return the name
{ return name; }
};

The invariant that a concrete class is supposed to maintain is the
single most important aspect of it.

You see, for a concrete class to be useful it must either (a) contain
more than one member-variable and maintain some sort of invariant
(guaranteed relationship) between them or (b) contain only one
member-variable and restrict its allowed values in some way (the
invariant would be that the member-variable will never be certain
values.)

Do you know what the invariant you created for this class is? If so,
express it in a comment.

////////////////////////////////////////////////////////////////
int main()
{
void bsort(person**, int); //prototype

Prototypes inside functions isn't a great idea IMO. Every module that
wants to use this bsort function should have a single place to go to get
the prototype, usually a header file. In this case, the prototype should
be outside of, and above main, but below class person.

person* persPtr[100]; //array of pointers to persons
int n = 0; //number of persons in array

Here is a perfect example for a class. There is an invariant between 'n'
and 'persPtr' such that n defines how many of the values in persPtr are
valid. Both of these variables should be in a class that has the job of
maintaining that invariant.

What if the user enters over 100 names? Whenever you use an array, you
have to insure that it's kept within bounds. Another job for the class
mentioned above?

Of course, if a class already exists that can do the job with minimal
effort, use it rather than writing your own. Consider using std::vector

char choice; //input char

do { //put persons in array
persPtr[n] = new person; //make new object

You create new objects here, but never delete them. Although the system
will clean them up upon exit, get in the habit of deleting every object
you new when you are done with it. In this case, you should go through
the array and delete all the persons before returning from main.

persPtr[n]->setName(); //set person's name
n++; //count new person
cout << "Enter another (y/n)? "; //enter another
cin >> choice; // person?
}
while( choice=='y' ); //quit on 'n'

The 'choice' variable is only used in the two lines above, yet its scope
is all of main. It's good practice to limit the scope of variables to
only the part of the program where they are actually used. This helps
insure that they don't accidentally get used incorrectly. In this case,
a function that gets the users choice and returns true if that choice is
'y' would be ideal. Then you can simply call that function inside the
while condition.

cout << "\nUnsorted list:";
for(int j=0; j<n; j++) //print unsorted list
{ persPtr[j]->printName(); }

bsort(persPtr, n); //sort pointers

cout << "\nSorted list:";
for(j=0; j<n; j++) //print sorted list
{ persPtr[j]->printName(); }
cout << endl;
return 0;
} //end main()

I want to talk for a second about code comments... Anyone who has
bothered to look at the definition of "bsort" will know that it sorts
pointers (after all you have a comment to that effect. So the "sort
pointers" comment above is superfluous. The other two, however are
somewhat handy because they explain what several lines of code are
doing. Note however the duplication between "print unsorted list" and
"print sorted list". Ideally when you have two blocks of code alike, you
should make a function that contains the block just once, and call it
twice. On top of that, if you call this function "print_list", it will
render these comments superfluous as well. You see, comments aren't bad,
but code that requires comments to be understood often is.

//--------------------------------------------------------------
void bsort(person** pp, int n) //sort pointers to persons
{
void order(person**, person**); //prototype
int j, k; //indexes to array

for(j=0; j<n-1; j++) //outer loop
for(k=j+1; k<n; k++) //inner loop starts at outer
order(pp+j, pp+k); //order the pointer contents
}
//--------------------------------------------------------------
void order(person** pp1, person** pp2) //orders two pointers
{ //if 1st larger than 2nd,
if( (*pp1)->getName() > (*pp2)->getName() )
{
person* tempptr = *pp1; //swap the pointers
*pp1 = *pp2;
*pp2 = tempptr;
}
}

Much like the comment I made about classes above, if a function already
exists that can do what you want, then consider using it rather than
writing your own.

In both cases (with functions and classes,) there is often something
that *almost* does what you want but not quite. You are then left with
the decision as to wether you should write something that does exactly
what you want, or write something that adapts your problem to fit its
solution. Your choice should be guided by several factors including ease
of understanding, amount of code required, as well as the performance of
the solution.

In this particular case, there are a couple general-purpose sort
routines already available in the standard library. Unless your
assignment was to write a bubble sort routine, your code would probably
be better served if you adapted one of the standard sort routines to
your purposes rather than writing your own. (a) The code needed to adapt
their solution to your purposes would be less than the code required to
create a new solution. (b) Less code (if written clearly) tends to be
easer to understand, (c) the general solution would likely still fall
within what performance requirements your program has and may even be
better. (After all the library implementors have spent much more time
perfecting their sort routines than you have.)

 

[=?ISO-8859-1?Q?Martin_J=F8rgensen?=]

Do you know what the invariant you created for this class is? If so,
express it in a comment.

I don't know what you mean by the word "invariant".

Prototypes inside functions isn't a great idea IMO. Every module that
wants to use this bsort function should have a single place to go to get
the prototype, usually a header file. In this case, the prototype should
be outside of, and above main, but below class person.

Why below class person?

person* persPtr[100]; //array of pointers to persons
int n = 0; //number of persons in array

Here is a perfect example for a class. There is an invariant between 'n'
and 'persPtr' such that n defines how many of the values in persPtr are
valid. Both of these variables should be in a class that has the job of
maintaining that invariant.

What if the user enters over 100 names? Whenever you use an array, you
have to insure that it's kept within bounds. Another job for the class
mentioned above?

I know, but it's not that important here... Besides that, I'm a c++
beginner so I wouldn't know how to do it better (in C however I think I
could do it now, although I'm not a C-expert).

Of course, if a class already exists that can do the job with minimal
effort, use it rather than writing your own. Consider using std::vector

char choice; //input char

do { //put persons in array
persPtr[n] = new person; //make new object

You create new objects here, but never delete them. Although the system
will clean them up upon exit, get in the habit of deleting every object
you new when you are done with it. In this case, you should go through
the array and delete all the persons before returning from main.
Ok.

persPtr[n]->setName(); //set person's name
n++; //count new person
cout << "Enter another (y/n)? "; //enter another
cin >> choice; // person?
}
while( choice=='y' ); //quit on 'n'

The 'choice' variable is only used in the two lines above, yet its scope
is all of main. It's good practice to limit the scope of variables to
only the part of the program where they are actually used. This helps
insure that they don't accidentally get used incorrectly. In this case,
a function that gets the users choice and returns true if that choice is
'y' would be ideal. Then you can simply call that function inside the
while condition.

Good idea.

cout << "\nUnsorted list:";
for(int j=0; j<n; j++) //print unsorted list
{ persPtr[j]->printName(); }

bsort(persPtr, n); //sort pointers

cout << "\nSorted list:";
for(j=0; j<n; j++) //print sorted list
{ persPtr[j]->printName(); }
cout << endl;
return 0;
} //end main()

I want to talk for a second about code comments... Anyone who has
bothered to look at the definition of "bsort" will know that it sorts
pointers (after all you have a comment to that effect. So the "sort
pointers" comment above is superfluous. The other two, however are
somewhat handy because they explain what several lines of code are
doing. Note however the duplication between "print unsorted list" and
"print sorted list". Ideally when you have two blocks of code alike, you
should make a function that contains the block just once, and call it
twice. On top of that, if you call this function "print_list", it will
render these comments superfluous as well. You see, comments aren't bad,
but code that requires comments to be understood often is.

Ok, good point. I understand what you mean, but I wouldn't know how to
change it now, since I'm a c++ beginner...

Much like the comment I made about classes above, if a function already
exists that can do what you want, then consider using it rather than
writing your own.

In both cases (with functions and classes,) there is often something
that *almost* does what you want but not quite. You are then left with
the decision as to wether you should write something that does exactly
what you want, or write something that adapts your problem to fit its
solution. Your choice should be guided by several factors including ease
of understanding, amount of code required, as well as the performance of
the solution.

In this particular case, there are a couple general-purpose sort
routines already available in the standard library. Unless your
assignment was to write a bubble sort routine, your code would probably
be better served if you adapted one of the standard sort routines to
your purposes rather than writing your own. (a) The code needed to adapt
their solution to your purposes would be less than the code required to
create a new solution. (b) Less code (if written clearly) tends to be
easer to understand, (c) the general solution would likely still fall
within what performance requirements your program has and may even be
better. (After all the library implementors have spent much more time
perfecting their sort routines than you have.)

Actually it was part of this code example not to use any standard
library functions... But nice comments...


Best regards / Med venlig hilsen
Martin Jørgensen

 

[Daniel T.]

I don't know what you mean by the word "invariant".

Something that is guaranteed to always be true. Whatever the invariant
is, the class's principal job is to ensure that it stays true no matter
what member-functions are called, and no matter what parameters are sent
to those functions. For example:

class Range {
// invariant: low() < high()
public:
int low() const;
int high() const;
// other member-functions
};

The class above has the invariant that the value returned by low() will
always be less than the value returned by high(). This means that *every
other member-function* must work to make sure that no matter what
happens, low() < high() will always be true.

Why below class person?

Because bsort(person**, int) is supposed to work with persons. Declaring
it after person makes sense. In a larger program, both class person and
the bsort algorithm would be in a header file together.

person* persPtr[100]; //array of pointers to persons
int n = 0; //number of persons in array

Here is a perfect example for a class. There is an invariant between 'n'
and 'persPtr' such that n defines how many of the values in persPtr are
valid. Both of these variables should be in a class that has the job of
maintaining that invariant.

What if the user enters over 100 names? Whenever you use an array, you
have to insure that it's kept within bounds. Another job for the class
mentioned above?

I know, but it's not that important here...

It's *always* important to make sure the program can *never* go over the
bounds of its arrays.

Besides that, I'm a c++
beginner so I wouldn't know how to do it better (in C however I think I
could do it now, although I'm not a C-expert).

This is a great time to learn. The most obvious solution is to break out
of the loop that creates person objects if the array is full...

do {
persPtr[n] = new person;
persPtr[n]->setName();
++n;
if ( n < 100 ) {
cout << "Enter another (y/n)? ";
cin >> choice;
}
else {
cout << "Memory full.\n";
choice = 'n';
}
}
while ( choice == 'y' )

The obvious way to test this is to try reducing the size of the array
for a run or two. Of course to do that, you need to change two values (
the two "100"s in the code.) Better would be to create a constant that
is used in both places...

const int max_names = 100;
//...
person* persPtr[max_names];
//...
if ( n < max_names ) {
//...

cout << "\nUnsorted list:";
for(int j=0; j<n; j++) //print unsorted list
{ persPtr[j]->printName(); }

bsort(persPtr, n); //sort pointers

cout << "\nSorted list:";
for(j=0; j<n; j++) //print sorted list
{ persPtr[j]->printName(); }
cout << endl;
return 0;
} //end main()

I want to talk for a second about code comments... Anyone who has
bothered to look at the definition of "bsort" will know that it sorts
pointers (after all you have a comment to that effect. So the "sort
pointers" comment above is superfluous. The other two, however are
somewhat handy because they explain what several lines of code are
doing. Note however the duplication between "print unsorted list" and
"print sorted list". Ideally when you have two blocks of code alike, you
should make a function that contains the block just once, and call it
twice. On top of that, if you call this function "print_list", it will
render these comments superfluous as well. You see, comments aren't bad,
but code that requires comments to be understood often is.

Ok, good point. I understand what you mean, but I wouldn't know how to
change it now, since I'm a c++ beginner...

Sure you do:

void print_persons( person** persPtr, int n )
{
for ( int j = 0; j < n; ++j )
persPtr[j]->printName();
}

Then your code becomes:

cout << "\nUnsorted list:";
print_persons( persPtr, n );

bsort( persPtr, n );

cout << "\nSorted list:";
print_persons( persPtr, n );

Actually it was part of this code example not to use any standard
library functions... But nice comments...

Yet you used operator<< for input, operator>> for output and operator
new to create person objects. Also, of course, several of the functions
defined in class string were used as well.

You see, it's very hard to write anything useful without using any of
the standard library functions.

 

[=?ISO-8859-1?Q?Martin_J=F8rgensen?=]

Something that is guaranteed to always be true. Whatever the invariant
is, the class's principal job is to ensure that it stays true no matter
what member-functions are called, and no matter what parameters are sent
to those functions. For example:

class Range {
// invariant: low() < high()
public:
int low() const;
int high() const;
// other member-functions
};

The class above has the invariant that the value returned by low() will
always be less than the value returned by high(). This means that *every
other member-function* must work to make sure that no matter what
happens, low() < high() will always be true.
Ok.



Because bsort(person**, int) is supposed to work with persons. Declaring
it after person makes sense. In a larger program, both class person and
the bsort algorithm would be in a header file together.

Yes, I have something to learn here....

person* persPtr[100]; //array of pointers to persons
int n = 0; //number of persons in array


Here is a perfect example for a class. There is an invariant between 'n'
and 'persPtr' such that n defines how many of the values in persPtr are
valid. Both of these variables should be in a class that has the job of
maintaining that invariant.

What if the user enters over 100 names? Whenever you use an array, you
have to insure that it's kept within bounds. Another job for the class
mentioned above?

I know, but it's not that important here...

It's *always* important to make sure the program can *never* go over the
bounds of its arrays.

Besides that, I'm a c++
beginner so I wouldn't know how to do it better (in C however I think I
could do it now, although I'm not a C-expert).

This is a great time to learn. The most obvious solution is to break out
of the loop that creates person objects if the array is full...

do {
persPtr[n] = new person;
persPtr[n]->setName();
++n;
if ( n < 100 ) {
cout << "Enter another (y/n)? ";
cin >> choice;
}
else {
cout << "Memory full.\n";
choice = 'n';
}
}
while ( choice == 'y' )

The obvious way to test this is to try reducing the size of the array
for a run or two. Of course to do that, you need to change two values (
the two "100"s in the code.) Better would be to create a constant that
is used in both places...

Oh, yeah. I didn't think of that.... That was easy enough

const int max_names = 100;
//...
person* persPtr[max_names];
//...
if ( n < max_names ) {
//...
Agreed.

cout << "\nUnsorted list:";
for(int j=0; j<n; j++) //print unsorted list
{ persPtr[j]->printName(); }

bsort(persPtr, n); //sort pointers

cout << "\nSorted list:";
for(j=0; j<n; j++) //print sorted list
{ persPtr[j]->printName(); }
cout << endl;
return 0;
} //end main()


I want to talk for a second about code comments... Anyone who has
bothered to look at the definition of "bsort" will know that it sorts
pointers (after all you have a comment to that effect. So the "sort
pointers" comment above is superfluous. The other two, however are
somewhat handy because they explain what several lines of code are
doing. Note however the duplication between "print unsorted list" and
"print sorted list". Ideally when you have two blocks of code alike, you
should make a function that contains the block just once, and call it
twice. On top of that, if you call this function "print_list", it will
render these comments superfluous as well. You see, comments aren't bad,
but code that requires comments to be understood often is.

Ok, good point. I understand what you mean, but I wouldn't know how to
change it now, since I'm a c++ beginner...

Sure you do:

void print_persons( person** persPtr, int n )
{
for ( int j = 0; j < n; ++j )
persPtr[j]->printName();
}

Then your code becomes:

cout << "\nUnsorted list:";
print_persons( persPtr, n );

bsort( persPtr, n );

cout << "\nSorted list:";
print_persons( persPtr, n );

Oh... This is something with pointers to pointers... Still a bit tricky
for me, so thanks for showing it so I can study the example code... I
think pointers to int/double/whatever is something I can handle easily
but pointers to pointers are a bit tricky yet...

Yet you used operator<< for input, operator>> for output and operator
new to create person objects. Also, of course, several of the functions
defined in class string were used as well.

You see, it's very hard to write anything useful without using any of
the standard library functions.

I meant: this code had to show how to sort without using general-purpose
sort-routines. But you knew that, based on your smiley I think


Best regards / Med venlig hilsen
Martin Jørgensen

 

[Daniel T.]

Daniel T. wrote:

cout << "\nUnsorted list:";
for(int j=0; j<n; j++) //print unsorted list
{ persPtr[j]->printName(); }

bsort(persPtr, n); //sort pointers

cout << "\nSorted list:";
for(j=0; j<n; j++) //print sorted list
{ persPtr[j]->printName(); }
cout << endl;
return 0;
} //end main()


I want to talk for a second about code comments... Anyone who has
bothered to look at the definition of "bsort" will know that it sorts
pointers (after all you have a comment to that effect. So the "sort
pointers" comment above is superfluous. The other two, however are
somewhat handy because they explain what several lines of code are
doing. Note however the duplication between "print unsorted list" and
"print sorted list". Ideally when you have two blocks of code alike, you
should make a function that contains the block just once, and call it
twice. On top of that, if you call this function "print_list", it will
render these comments superfluous as well. You see, comments aren't bad,
but code that requires comments to be understood often is.

Ok, good point. I understand what you mean, but I wouldn't know how to
change it now, since I'm a c++ beginner...

Sure you do:

void print_persons( person** persPtr, int n )
{
for ( int j = 0; j < n; ++j )
persPtr[j]->printName();
}

Then your code becomes:

cout << "\nUnsorted list:";
print_persons( persPtr, n );

bsort( persPtr, n );

cout << "\nSorted list:";
print_persons( persPtr, n );

Oh... This is something with pointers to pointers... Still a bit tricky
for me, so thanks for showing it so I can study the example code... I
think pointers to int/double/whatever is something I can handle easily
but pointers to pointers are a bit tricky yet...

But that is exactly wha tyou did with your bsort below, so you must have
at least some understanding of it.

Also, this further emphasizes that persPtr and n belong in a class
together. We have an invariant (that I've already discussed) and now two
functions that need both variable to do there job.

We've also created a 'max_names' constant that directly relates to the
persPtr... All this screams "make me a class!"

I suggest your next exorcise should be to create a PersonArray class.
Finish this program before you go on... I'll get you started:

class PersonArray
{
// invariants: if n > 0 then persPtr[0] to persPtr[n-1] point to valid
// persons.
// persPtr[n] to persPtr[max_names] == 0
// n <= max_names
static const int max_names = 100;
person* persPtr[max_names];
int n;
public:
// add methods here...
};

I suggest you build the class slowly. First create:

class PersonArray
{
int n;
public:
int get_n() const { return n; }
void set_n( int v ) { n = v; }
};

Then in your code replace:

int n = 0;

with

PersonArray persons;

and change the rest of the code to use "persons.set_n(int)" wherever n
is modified and "persons.get_n()" wherever the value of n is needed.

Once you've made that small change (and tested to make sure the code
still works,) do the same with persPtr, move it out of main and into the
PersonArray class, create a get/set pair and adjust main to use the new
get/sets (remember, persPtr is actually a series of variables that you
are moving in, the getter and setter will need an index and will get/set
a person*.

After that, your code that a calls to one of the setters and breaks the
class invariant needs to be moved into the PersonArray class such that
main can call one function that doesn't return until the invariant is
true again. Once you get this code moved, you should be able to remove
the two sets you created earlier.

I meant: this code had to show how to sort without using general-purpose
sort-routines. But you knew that, based on your smiley I think

But "how you sort" is by using the general purpose routines. Just like
"how you get data from the user" is by using a general purpose routine.

 

[=?ISO-8859-1?Q?Martin_J=F8rgensen?=]

But that is exactly wha tyou did with your bsort below, so you must have
at least some understanding of it.

Well, I have a book that I has some good example code in it, and I feel
pretty comfortable with working with pointers having a single
redirection (*)... So, hopefully I'll soon be comfortable with pointers
to pointers, although I'm not completely at this moment.

-snip-

After that, your code that a calls to one of the setters and breaks the
class invariant needs to be moved into the PersonArray class such that
main can call one function that doesn't return until the invariant is
true again. Once you get this code moved, you should be able to remove
the two sets you created earlier.

Thanks for the instructions... Was it something like below you thought
of? Took me about 1-2 hours

- - - - - - - - - - - - - - - - -
#include <iostream>
#include <string> //for string class


#define max_names 100

using namespace std;
////////////////////////////////////////////////////////////////
class person //class of persons
{
protected:
string name; //person's name

public:
void setName() //set the name
{ cout << "Enter name: "; cin >> name; }
void printName() //display the name
{ cout << endl << name; }
string getName() //return the name
{ return name; }
};

class PersonArray
{
private:
int n;
string name; //person's name

public:
/* I would like to make the array below private, but couldn't... */
person* persPtr[max_names]; //pointer to each person

PersonArray() : n(0) { } /* constructor */
int get_n() const { return n; }
void set_n( int v ) { n = v; }
void print_persons( person** persPtr )
{
if ( n >= 0 && n < max_names)
for ( int j = 0; j < n; ++j )
persPtr[j]->printName();
}
void addperson()
{
if( n>= 0 && n < max_names)
{
persPtr[n] = new person;
persPtr[n] -> setName();
n++;
}
else
cout << "No room for additional persons!" << endl;
}

};


////////////////////////////////////////////////////////////////
int main()
{
void bsort(person**, int); //prototype

/* contains current number n + pointers up to n-1 */
PersonArray persons;

char choice; //input char

do {
persons.addperson();
cout << "Enter another (y/n)? ";
cin >> choice;
}
while ( choice != 'n' );

cout << "\n\nUnsorted list:";
persons.print_persons( persons.persPtr );

bsort(persons.persPtr, persons.get_n() ); //sort pointers

cout << "\n\n\nSorted list:";
persons.print_persons( persons.persPtr );

cout << endl;
return 0;
} //end main()

//--------------------------------------------------------------
void bsort(person** pp, int n) //sort pointers to persons
{
void order(person**, person**); //prototype
int j, k; //indexes to array

for(j=0; j<n-1; j++) //outer loop
for(k=j+1; k<n; k++) //inner loop starts at outer
order(pp+j, pp+k); //order the pointer contents
}

//--------------------------------------------------------------
void order(person** pp1, person** pp2) //orders two pointers
{ //if 1st larger than 2nd,
if( (*pp1)->getName() > (*pp2)->getName() )
{
person* tempptr = *pp1; //swap the pointers
*pp1 = *pp2;
*pp2 = tempptr;
}
}

- - - - - - - - - - - - - - - - -



Best regards / Med venlig hilsen
Martin Jørgensen

 

[Daniel T.]

class PersonArray
{
private:
int n;
string name; //person's name

public:
/* I would like to make the array below private, but couldn't... */
person* persPtr[max_names]; //pointer to each person

PersonArray() : n(0) { } /* constructor */
int get_n() const { return n; }
void set_n( int v ) { n = v; }
void print_persons( person** persPtr )
{
if ( n >= 0 && n < max_names)
for ( int j = 0; j < n; ++j )
persPtr[j]->printName();
}
void addperson()
{
if( n>= 0 && n < max_names)
{
persPtr[n] = new person;
persPtr[n] -> setName();
n++;
}
else
cout << "No room for additional persons!" << endl;
}

};

That's a start, but you need to work harder at making persPtr private.
It *can* be done.

A couple of other points:
1) loose the "string name" in this class. It makes no sense and isn't
used.
2) loose the "set_n" function. Remember, no function in this class is
allowed to break the class invariant. If someone called
myPersonArray.set_n( 1000 ), that would be a problem. None of your
code uses this function anyway.
3) Addperson can be cleaned up some.
a) we know that n >= 0 is always true, because once you loose the
"set_n" function there is no way to make n negative except from
within this class, and this class would never do that. You can use
an assert if you are paranoid enough.
b) If addperson fails, it prints a message to the user, but the
function calling it never finds out, thus it will continue to ask
the user for more names. That makes no sense. Have addperson
return a bool result indicating success.
4) You had the foresight to bring print_persons into your class, but you
have it using the persPtr passed in rather than the persPtr it
already owns. Fix that.
5) print_persons is in the class because it uses persPtr and n, but what
about bsort? It uses the same variables...

 

[Old Wolf]

What is this istream all about? You're saying something like:
std::getline ( (type istream), (type string) )?

Yes, the function std::getline takes an istream and a string as
parameters by reference, and it reads characters from the stream
and puts them into the string, until it gets to '\n'. There are
various online references about C++ standard library functions,
where you can look up std::getline.

What's that INT_MAX-thing doing? It's not defined anywhere here...
Still the program works.

It's defined in <climits>, which Alf forgot to include. His compiler
must have automatically included it as part of one of the other
header files (which is permitted).
Its value is the maximum possible value of an 'int'.

 

[=?ISO-8859-1?Q?Martin_J=F8rgensen?=]

Yes, the function std::getline takes an istream and a string as
parameters by reference, and it reads characters from the stream
and puts them into the string, until it gets to '\n'. There are
various online references about C++ standard library functions,
where you can look up std::getline.

I'll try that.

It's defined in <climits>, which Alf forgot to include. His compiler
must have automatically included it as part of one of the other
header files (which is permitted).
Its value is the maximum possible value of an 'int'.

Ok, thanks...


Best regards / Med venlig hilsen
Martin Jørgensen

 

[=?ISO-8859-1?Q?Martin_J=F8rgensen?=]

That's a start, but you need to work harder at making persPtr private.
It *can* be done.

A couple of other points:
1) loose the "string name" in this class. It makes no sense and isn't
used.
2) loose the "set_n" function. Remember, no function in this class is
allowed to break the class invariant. If someone called
myPersonArray.set_n( 1000 ), that would be a problem. None of your
code uses this function anyway.
3) Addperson can be cleaned up some.
a) we know that n >= 0 is always true, because once you loose the
"set_n" function there is no way to make n negative except from
within this class, and this class would never do that. You can use
an assert if you are paranoid enough.
b) If addperson fails, it prints a message to the user, but the
function calling it never finds out, thus it will continue to ask
the user for more names. That makes no sense. Have addperson
return a bool result indicating success.
4) You had the foresight to bring print_persons into your class, but you
have it using the persPtr passed in rather than the persPtr it
already owns. Fix that.
5) print_persons is in the class because it uses persPtr and n, but what
about bsort? It uses the same variables...

Oooh... I feel I'm really getting somewhere

How do you like my code now?
- - - - - - - - - - - - - - - - - - - - -
#include <iostream>
#include <string> //for string class


#define max_names 4

using namespace std;
////////////////////////////////////////////////////////////////
class person //class of persons
{
protected:
string name; //person's name

public:
void setName() //set the name
{ cout << "Enter name: "; cin >> name; }
void printName() //display the name
{ cout << endl << name; }
string getName() //return the name
{ return name; }
};


////////////////////////////////////////////////////////////////

class PersonArray
{
private:
int n;
person* persPtr[max_names]; //pointer to each person


void order(person** pp1, person** pp2) //orders two pointers
{ //if 1st larger than 2nd,
if( (*pp1)->getName() > (*pp2)->getName() )
{
person* tempptr = *pp1; //swap the pointers
*pp1 = *pp2;
*pp2 = tempptr;
}
}
public:
PersonArray() : n(0) { } /* constructor */
int get_n() const { return n; }

void print_persons();
bool addperson();
void bsort(); //sort pointers to persons

};


void PersonArray:rint_persons()
{
if ( n <= max_names)
for ( int j = 0; j < n; ++j )
persPtr[j]->printName();
}

bool PersonArray::addperson()
{
if( n < max_names-1)
{
persPtr[n] = new person;
persPtr[n] -> setName();
n++;
return(1);
}
else /* finish last person and quit loop */
{
persPtr[n] = new person;
persPtr[n] -> setName();
n++; /* n = max_names (else print_persons doesn't work
correctly)... */
cout << endl << "No room for more persons than " << max_names <<
" persons!" << endl;
return(0);
}
}
void PersonArray::bsort() //sort pointers to persons
{
int j, k; //indexes to array

for(j=0; j<n-1; j++) //outer loop
for(k=j+1; k<n; k++) //inner loop starts at outer
order(persPtr+j, persPtr+k); //order the pointer contents
}


////////////////////////////////////////////////////////////////
int main()
{
void bsort(person**, int); //prototype
PersonArray persons; //persons-array...
char choice; //input char

while(persons.addperson() )
{
cout << "Enter another (y/n)? ";
cin >> choice;
if(choice == 'n')
break;
}

cout << "\n\nUnsorted list:";

persons.print_persons();

persons.bsort();
cout << "\n\n\nSorted list:";

persons.print_persons();

cout << endl;
return 0;
}

- - - - - - - - - - - - - - - - - - - - -

It still took me too long time (about 1-2 hours again)... But next time,
I can hopefully do it much quicker


Best regards / Med venlig hilsen
Martin Jørgensen

 

[Daniel T.]

Oooh... I feel I'm really getting somewhere

How do you like my code now?
- - - - - - - - - - - - - - - - - - - - -
#include <iostream>
#include <string> //for string class


#define max_names 4

max_names is only used inside the PersonArray class so you should put it
there. However, you can't do that if its a define. Change it to a
"static const int".

This is one of the reasons that everyone told you not to use #define.

using namespace std;
////////////////////////////////////////////////////////////////
class person //class of persons
{
protected:
string name; //person's name

public:
void setName() //set the name
{ cout << "Enter name: "; cin >> name; }
void printName() //display the name
{ cout << endl << name; }
string getName() //return the name
{ return name; }
};


////////////////////////////////////////////////////////////////

class PersonArray
{
private:
int n;
person* persPtr[max_names]; //pointer to each person


void order(person** pp1, person** pp2) //orders two pointers
{ //if 1st larger than 2nd,
if( (*pp1)->getName() > (*pp2)->getName() )
{
person* tempptr = *pp1; //swap the pointers
*pp1 = *pp2;
*pp2 = tempptr;
}
}
public:
PersonArray() : n(0) { } /* constructor */
int get_n() const { return n; }

void print_persons();
bool addperson();
void bsort(); //sort pointers to persons

};


void PersonArray:rint_persons()
{
if ( n <= max_names)
for ( int j = 0; j < n; ++j )
persPtr[j]->printName();
}

You have some code duplication in your addperson function below. I
expect that you can get rid of it with a modicum of effort, especially
if you take the output out of here and into main.

bool PersonArray::addperson()
{
if( n < max_names-1)
{
persPtr[n] = new person;
persPtr[n] -> setName();
n++;
return(1);

It's a bool try returning "true" or "false" as in:

return true;

}
else /* finish last person and quit loop */
{
persPtr[n] = new person;
persPtr[n] -> setName();
n++; /* n = max_names (else print_persons doesn't work
correctly)... */
cout << endl << "No room for more persons than " << max_names <<
" persons!" << endl;

If you take the line above out of this function. You can use your class
in programs that don't use cout for output (for example a GUI program.)
It's generally a good idea to separate your "View" (output) from your
"Model" (computation.)

return(0); return false;
}
}
void PersonArray::bsort() //sort pointers to persons
{
int j, k; //indexes to array

for(j=0; j<n-1; j++) //outer loop
for(k=j+1; k<n; k++) //inner loop starts at outer
order(persPtr+j, persPtr+k); //order the pointer contents
}

You still never delete your person objects. Give PersonArray a
destructor and delete them in there.

////////////////////////////////////////////////////////////////
int main()
{
void bsort(person**, int); //prototype
PersonArray persons; //persons-array...
char choice; //input char

while(persons.addperson() )
{
cout << "Enter another (y/n)? ";
cin >> choice;
if(choice == 'n')
break;
}

I'm not a big fan of the break statement. You need to pull the
persons.addperson() out of the while so that you can handle the output
if it fails.

cout << "\n\nUnsorted list:";

persons.print_persons();

persons.bsort();
cout << "\n\n\nSorted list:";

persons.print_persons();

cout << endl;
return 0;
}

- - - - - - - - - - - - - - - - - - - - -

It still took me too long time (about 1-2 hours again)... But next time,
I can hopefully do it much quicker

It will. Once you learn how and where you can trust the compiler to
catch your errors, you'll be able to go much faster. Some say, the speed
of a programmer increases with the square of his skill. Right now you
are still in the "less than one" range on your skill.

 

[=?ISO-8859-1?Q?Martin_J=F8rgensen?=]

max_names is only used inside the PersonArray class so you should put it
there. However, you can't do that if its a define. Change it to a
"static const int".

This is one of the reasons that everyone told you not to use #define.

Oh, sorry. I forgot to change it. It's changed now.

using namespace std;
////////////////////////////////////////////////////////////////
class person //class of persons
{
protected:
string name; //person's name

public:
void setName() //set the name
{ cout << "Enter name: "; cin >> name; }
void printName() //display the name
{ cout << endl << name; }
string getName() //return the name
{ return name; }
};


////////////////////////////////////////////////////////////////

class PersonArray
{
private:
int n;
person* persPtr[max_names]; //pointer to each person


void order(person** pp1, person** pp2) //orders two pointers
{ //if 1st larger than 2nd,
if( (*pp1)->getName() > (*pp2)->getName() )
{
person* tempptr = *pp1; //swap the pointers
*pp1 = *pp2;
*pp2 = tempptr;
}
}
public:
PersonArray() : n(0) { } /* constructor */
int get_n() const { return n; }

void print_persons();
bool addperson();
void bsort(); //sort pointers to persons

};


void PersonArray:rint_persons()
{
if ( n <= max_names)
for ( int j = 0; j < n; ++j )
persPtr[j]->printName();
}

You have some code duplication in your addperson function below. I
expect that you can get rid of it with a modicum of effort, especially
if you take the output out of here and into main.

I removed the code duplication...

bool PersonArray::addperson()
{
if( n < max_names-1)
{
persPtr[n] = new person;
persPtr[n] -> setName();
n++;
return(1);

It's a bool try returning "true" or "false" as in:

return true;
Ok...

}
else /* finish last person and quit loop */
{
persPtr[n] = new person;
persPtr[n] -> setName();
n++; /* n = max_names (else print_persons doesn't work
correctly)... */
cout << endl << "No room for more persons than " << max_names <<
" persons!" << endl;

If you take the line above out of this function. You can use your class
in programs that don't use cout for output (for example a GUI program.)
It's generally a good idea to separate your "View" (output) from your
"Model" (computation.)

Ok. Done.

return false;



You still never delete your person objects. Give PersonArray a
destructor and delete them in there.

Ok. I added a destructor. But don't understand: What is it that should
be deleted? See my new code, below.

I'm not a big fan of the break statement. You need to pull the
persons.addperson() out of the while so that you can handle the output
if it fails.

I can handle it outside now...

It will. Once you learn how and where you can trust the compiler to
catch your errors, you'll be able to go much faster. Some say, the speed
of a programmer increases with the square of his skill. Right now you
are still in the "less than one" range on your skill.

Yep... New code:

- - - - - - - - - - - - - - - - - -
#include <iostream>
#include <string> //for string class

const int max_names =4;

using namespace std;
////////////////////////////////////////////////////////////////
class person //class of persons
{
protected:
string name; //person's name

public:
void setName() //set the name
{ cout << "Enter name: "; cin >> name; }
void printName() //display the name
{ cout << endl << name; }
string getName() //return the name
{ return name; }
};


////////////////////////////////////////////////////////////////

class PersonArray
{
private:
int n;
person* persPtr[max_names]; //pointer to each person


void order(person** pp1, person** pp2) //orders two pointers
{ //if 1st larger than 2nd,
if( (*pp1)->getName() > (*pp2)->getName() )
{
person* tempptr = *pp1; //swap the pointers
*pp1 = *pp2;
*pp2 = tempptr;
}
}
public:
PersonArray() : n(0) { } /* constructor */
~PersonArray() { cout << "PersonArray object destroyed" << endl; }
int get_n() const { return n; }

void print_persons();
bool addperson();
void bsort(); //sort pointers to persons

};


void PersonArray:rint_persons()
{
if ( n <= max_names)
for ( int j = 0; j < n; ++j )
persPtr[j]->printName();
}

bool PersonArray::addperson()
{
/* add person */
persPtr[n] = new person;
persPtr[n] -> setName();
n++;

/* last name ? */
if( n => max_names )
{
return(false);
}

/* else finish and return normally */
return(true);
}

void PersonArray::bsort() //sort pointers to persons
{
int j, k; //indexes to array

for(j=0; j<n-1; j++) //outer loop
for(k=j+1; k<n; k++) //inner loop starts at outer
order(persPtr+j, persPtr+k); //order the pointer contents
}


////////////////////////////////////////////////////////////////
int main()
{
void bsort(person**, int); //prototype
PersonArray persons; //persons-array...
char choice; //input char
bool success;

while( success = persons.addperson() )
{
cout << "Enter another (y/n)? ";
cin >> choice;
if(choice == 'n')
break;
}

/* is the array full ? */
if(success == false)
cout << endl << "No room for more persons than "
<< max_names << " persons!" << endl;

cout << "\n\nUnsorted list:";

persons.print_persons();

persons.bsort();
cout << "\n\n\nSorted list:";

persons.print_persons();

cout << endl;
return 0;
}

- - - - - - - - - - - - - - - - - -


Best regards / Med venlig hilsen
Martin Jørgensen

 

[Daniel T.]

- - - - - - - - - - - - - - - - - -
#include <iostream>
#include <string> //for string class

const int max_names =4;

Now that max_names is a proper type, you can put it directly in
PersonArray. After all, that is the only class that uses it. See below.

using namespace std;
////////////////////////////////////////////////////////////////
class person //class of persons
{
protected:
string name; //person's name

public:
void setName() //set the name
{ cout << "Enter name: "; cin >> name; }
void printName() //display the name
{ cout << endl << name; }
string getName() //return the name
{ return name; }
};


////////////////////////////////////////////////////////////////

class PersonArray
{
private:

static const int max_names = 4;

The 'static' keyword above tells the compiler that *all* PersonArrays
use the same 'max_names'. IE every PersonArray will only be able to hold
that many names each.

int n;
person* persPtr[max_names]; //pointer to each person


void order(person** pp1, person** pp2) //orders two pointers
{ //if 1st larger than 2nd,
if( (*pp1)->getName() > (*pp2)->getName() )
{
person* tempptr = *pp1; //swap the pointers
*pp1 = *pp2;
*pp2 = tempptr;
}
}
public:
PersonArray() : n(0) { } /* constructor */
~PersonArray() { cout << "PersonArray object destroyed" << endl; }
int get_n() const { return n; }

void print_persons();
bool addperson();
void bsort(); //sort pointers to persons

};


void PersonArray:rint_persons()
{
if ( n <= max_names)
for ( int j = 0; j < n; ++j )
persPtr[j]->printName();
}

What happens if someone calls add person after it returns false?
Remember, the functions of the class are supposed to make sure that the
invariant is never broken.

Also, the idiom is to return false only if the addperson function failed
to add a person, you are returning false when the addperson succeeds but
will fail if called again.

Also, you were asking what you should do in the destructor? Anything you
new must be deleted manually, the system doesn't take care of it for you
until program exit. Waiting until program exit in this case is OK but it
isn't always. In fact usually it causes problems. Your PersonArray
destructor needs delete every valid persPtr.

bool PersonArray::addperson()
{
/* add person */
persPtr[n] = new person;
persPtr[n] -> setName();
n++;

/* last name ? */
if( n => max_names )
{
return(false);
}

/* else finish and return normally */
return(true);
}

void PersonArray::bsort() //sort pointers to persons
{
int j, k; //indexes to array

for(j=0; j<n-1; j++) //outer loop
for(k=j+1; k<n; k++) //inner loop starts at outer
order(persPtr+j, persPtr+k); //order the pointer contents
}


////////////////////////////////////////////////////////////////
int main()
{
void bsort(person**, int); //prototype
PersonArray persons; //persons-array...
char choice; //input char
bool success;

while( success = persons.addperson() )
{
cout << "Enter another (y/n)? ";
cin >> choice;
if(choice == 'n')
break;
}

This is something of a religious issue so I'm going to tread carefully
here...

There was a time when I used break & continue in blocks, and multiple
returns in functions. Hell I even had the backing of several acknowledge
experts all scoffing at the "single-entry/single-exit" style of
programming (strictly speaking they believe in single-entry but have no
problem with multiple exits.)

Then about 4 years ago, I spent quite a bit of time trying to debug
others' code. It was the first time the chore fell to me, to that point
I always wrote new code or debugged my own stuff. Suddenly, I had to go
through functions line by line trying to figure out why my break point
was never reached and it was a real pain. From then on, I listened to
Dijkstra. One entry, one exit for each block of code.

 

[=?ISO-8859-1?Q?Martin_J=F8rgensen?=]

Now that max_names is a proper type, you can put it directly in
PersonArray. After all, that is the only class that uses it. See below.

Ok, thanks for telling me since I got some errors while trying to do it...

static const int max_names = 4;

The 'static' keyword above tells the compiler that *all* PersonArrays
use the same 'max_names'. IE every PersonArray will only be able to hold
that many names each.

Yep, I've learned about that...

int n;
person* persPtr[max_names]; //pointer to each person


void order(person** pp1, person** pp2) //orders two pointers
{ //if 1st larger than 2nd,
if( (*pp1)->getName() > (*pp2)->getName() )
{
person* tempptr = *pp1; //swap the pointers
*pp1 = *pp2;
*pp2 = tempptr;
}
}
public:
PersonArray() : n(0) { } /* constructor */
~PersonArray() { cout << "PersonArray object destroyed" << endl; }
int get_n() const { return n; }

void print_persons();
bool addperson();
void bsort(); //sort pointers to persons

};


void PersonArray:rint_persons()
{
if ( n <= max_names)
for ( int j = 0; j < n; ++j )
persPtr[j]->printName();
}

What happens if someone calls add person after it returns false?
Remember, the functions of the class are supposed to make sure that the
invariant is never broken.

Also, the idiom is to return false only if the addperson function failed
to add a person, you are returning false when the addperson succeeds but
will fail if called again.

Yep, that's a clear mistake... I've modified the program again...

Also, you were asking what you should do in the destructor? Anything you
new must be deleted manually, the system doesn't take care of it for you
until program exit. Waiting until program exit in this case is OK but it
isn't always. In fact usually it causes problems. Your PersonArray
destructor needs delete every valid persPtr.

Ok... hmmm. Could be after having printed them to the screen...

bool PersonArray::addperson()
{
/* add person */
persPtr[n] = new person;
persPtr[n] -> setName();
n++;

/* last name ? */
if( n => max_names )
{
return(false);
}

/* else finish and return normally */
return(true);
}

void PersonArray::bsort() //sort pointers to persons
{
int j, k; //indexes to array

for(j=0; j<n-1; j++) //outer loop
for(k=j+1; k<n; k++) //inner loop starts at outer
order(persPtr+j, persPtr+k); //order the pointer contents
}


////////////////////////////////////////////////////////////////
int main()
{
void bsort(person**, int); //prototype
PersonArray persons; //persons-array...
char choice; //input char
bool success;

while( success = persons.addperson() )
{
cout << "Enter another (y/n)? ";
cin >> choice;
if(choice == 'n')
break;
}

This is something of a religious issue so I'm going to tread carefully
here...

There was a time when I used break & continue in blocks, and multiple
returns in functions. Hell I even had the backing of several acknowledge
experts all scoffing at the "single-entry/single-exit" style of
programming (strictly speaking they believe in single-entry but have no
problem with multiple exits.)

Then about 4 years ago, I spent quite a bit of time trying to debug
others' code. It was the first time the chore fell to me, to that point
I always wrote new code or debugged my own stuff. Suddenly, I had to go
through functions line by line trying to figure out why my break point
was never reached and it was a real pain. From then on, I listened to
Dijkstra. One entry, one exit for each block of code.

hmmm. Yeah, I see what you mean but I can't think of anything that would
improve the program considerable because the good thing about the
following is that I think the while loop is very simple and pretty easy
to understand... So I can't really think of anything that is much better
here...?

New code with your recent suggestions implemented:
- - - - - - - - - - - - - - - - - - - - - - - -
#include <iostream>
#include <string> //for string class

//const int max_names =4;

using namespace std;
////////////////////////////////////////////////////////////////
class person //class of persons
{
protected:
string name; //person's name

public:
void setName() //set the name
{ cout << "Enter name: "; cin >> name; }
void printName() //display the name
{ cout << endl << name; }
string getName() //return the name
{ return name; }
~person() { cout << endl << endl << "Person class object
destroyed!" << endl; }
};


////////////////////////////////////////////////////////////////

class PersonArray
{
private:
static const int max_names = 4;
int n;
person* persPtr[max_names]; //pointer to each person

void order(person** pp1, person** pp2) //orders two pointers
{ //if 1st larger than 2nd,
if( (*pp1)->getName() > (*pp2)->getName() )
{
person* tempptr = *pp1; //swap the pointers
*pp1 = *pp2;
*pp2 = tempptr;
}
}
public:
PersonArray() : n(0) { } /* constructor */
~PersonArray() { cout << endl << endl << "PersonArray object
destroyed" << endl; }
int get_n() const { return n; }

void print_persons();
bool addperson();
void bsort(); //sort pointers to persons
void delete_them();

};


void PersonArray:rint_persons()
{
if ( n <= max_names)
for ( int j = 0; j < n; ++j )
persPtr[j]->printName();
}

bool PersonArray::addperson()
{
/* last name ? */
if( n == max_names ) /* or >= perhaps = */
{
return(false);
}

/* add person */
persPtr[n] = new person;
persPtr[n] -> setName();
n++;

/* else finish and return normally */
return(true);
}

void PersonArray::bsort() //sort pointers to persons
{
int j, k; //indexes to array

for(j=0; j<n-1; j++) //outer loop
for(k=j+1; k<n; k++) //inner loop starts at outer
order(persPtr+j, persPtr+k); //order the pointer contents
}

void PersonArray::delete_them()
{
for ( int j = 0; j < n; ++j )
delete persPtr[j];
}


////////////////////////////////////////////////////////////////
int main()
{
void bsort(person**, int); //prototype
PersonArray persons; //persons-array...
char choice; //input char
bool success;

while( success = persons.addperson() )
{
cout << "Enter another (y/n)? ";
cin >> choice;
if(choice == 'n')
break;
}

/* is the array full ? */
if(success == false)
cout << endl << "No room for more persons." << endl;

cout << "\n\nUnsorted list:";

persons.print_persons();

persons.bsort();
cout << "\n\n\nSorted list:";

persons.print_persons();

persons.delete_them();

cout << endl;
return 0;
}


- - - - - - - - - - - - - - - - - - - - - - - -


Best regards / Med venlig hilsen
Martin Jørgensen