problem with array and classes

[Fredda]

Hello!

I have problem with the following:

From the class 'Transaktion':

------------------------------------------------------------------------------------------------------
..
..
..

Transaktion :: Transaktion(const Transaktion &t)
: typ(t.typ), datum(t.datum), namn(t.namn), belopp(t.belopp),
ant_kompisar(t.ant_kompisar)
{
if (ant_kompisar > 0) {
kompisar = new string[ant_kompisar];
for (int i = 0; i < ant_kompisar; i++)
kompisar = t.kompisar;
}
else
kompisar = 0;

}

..
..
..

bool Transaktion :: finnsKompis(string namnet) {
for (int i = 0; i < ant_kompisar; i++)
if (kompisar == namnet)
return true;
return false;
}
..
..
..

From the class 'TransaktionsLista':

------------------------------------------------------------------------------------------------------
..
..
..
double TransaktionsLista :: aerSkyldig(string namnet) {
double belopp = 0.0;
for (int i = 0; i < antalTrans; i++) {
Transaktion temp = Transaktion(trans);
if ( temp.finnsKompis(namnet) )
belopp += ( trans.haemta_belopp() /
(trans.haemta_ant_kompisar() + 1) );
}
return belopp;
}
..
..
..

I want to use the method finnsKompis(string namnet) from the class
'Transaktion' on a Transaktion-object in the class 'TransaktionsLista'.
The code compiles fine but generates an error on runtime. I suspect it
has
something to do with the matrix 'kompisar[]' in the
'Transaktion'-class but I don't understand what's wrong. I do use a
copy-constructor in the class 'Transaktion' but it doesn't seem to
help.

I'm very greatful for any suggestions!
Thanks in advance!
Fredrik

PS. I'm sorry that the code is in swedish...

 

[Mike Wahler]

Hello!

I have problem with the following:

From the class 'Transaktion':

------------------------------------------------------------------------------------------------------
.
.
.

Transaktion :: Transaktion(const Transaktion &t)
: typ(t.typ), datum(t.datum), namn(t.namn), belopp(t.belopp),
ant_kompisar(t.ant_kompisar)
{
if (ant_kompisar > 0) {
kompisar = new string[ant_kompisar];
for (int i = 0; i < ant_kompisar; i++)
kompisar = t.kompisar;
}
else
kompisar = 0;

}

.
.
.

bool Transaktion :: finnsKompis(string namnet) {
for (int i = 0; i < ant_kompisar; i++)
if (kompisar == namnet)
return true;
return false;
}
.
.
.

From the class 'TransaktionsLista':

------------------------------------------------------------------------------------------------------
.
.
.
double TransaktionsLista :: aerSkyldig(string namnet) {
double belopp = 0.0;
for (int i = 0; i < antalTrans; i++) {
Transaktion temp = Transaktion(trans);
if ( temp.finnsKompis(namnet) )
belopp += ( trans.haemta_belopp() /
(trans.haemta_ant_kompisar() + 1) );
}
return belopp;
}
.
.
.

I want to use the method finnsKompis(string namnet) from the class
'Transaktion' on a Transaktion-object in the class 'TransaktionsLista'.
The code compiles fine but generates an error on runtime. I suspect it
has
something to do with the matrix 'kompisar[]' in the
'Transaktion'-class but I don't understand what's wrong. I do use a
copy-constructor in the class 'Transaktion' but it doesn't seem to
help.

I'm very greatful for any suggestions!
Thanks in advance!
Fredrik

PS. I'm sorry that the code is in swedish...

You have not supplied enough information. We need at least
the definitions of your 'Transaktion' and ''TransaktionsLista'
classes.

Best would be a small complete program which we can compile. But
one thing: have you observed the 'rule of 3'? (This states that
if your class needs any one of: copy constructor, assignment
operator (operator=()) or destructor, it very likely needs all
three.) I suspect this could be your problem. But I'm only
guessing without seeing more code.

-Mike

 

[Fredda]

Hi again!

Here comes a compilable program with parts of the code.
I've had quite a few problems whith when to use a destructor and when
not to.
It shouldn't be a problem to delete a NULL-pointer but sometimes I just
get the
same error as in the problem below. Thats why I've inactivated the
vital parts
of the destructors.

Thanks for your help!
Fredrik

#include <iostream>
#include <fstream>
#include <string>

using namespace std;

class Transaktion
{
private:
string datum;
string typ;
string namn;
double belopp;
int ant_kompisar;
string *kompisar;

public:
Transaktion();
Transaktion(const Transaktion &t);
~Transaktion();
//string haemta_namn();
double haemta_belopp();
int haemta_ant_kompisar();
bool finnsKompis( string namnet );
//bool laesEnTrans( istream &is );
void skrivEnTrans( ostream & os );
}; //End Transaktion

// Standardkonstruktor
Transaktion :: Transaktion()
: datum("060118"), typ("Food"), namn("Mike"), belopp(10.0),
ant_kompisar(3)
{
kompisar = new string[ant_kompisar];
kompisar[0] = "Fredrik";
kompisar[1] = "Karl";
kompisar[2] = "Richard";
kompisar[3] = "Linn";
}

// Kopieringskonstruktor
Transaktion :: Transaktion(const Transaktion &t)
: typ(t.typ), datum(t.datum), namn(t.namn), belopp(t.belopp),
ant_kompisar(t.ant_kompisar)
{
if (ant_kompisar > 0) {
kompisar = new string[ant_kompisar];
for (int i = 0; i < ant_kompisar; i++)
kompisar = t.kompisar;
}
else
kompisar = 0;

}

// Destruktor
Transaktion :: ~Transaktion() {
//delete [] kompisar;
}

double Transaktion :: haemta_belopp() {
return belopp;
}

int Transaktion :: haemta_ant_kompisar() {
return ant_kompisar;
}

void Transaktion :: skrivEnTrans(ostream &os) {
if (ant_kompisar > 0) {
if (os) {
os << datum << "\t" << typ << "\t" << namn << "\t" << belopp
<< "\t" << ant_kompisar;
for (int i = 0; i < ant_kompisar; i++)
os << "\t" << kompisar;
os << endl;
}
else cout << "output stream error";
}
}

bool Transaktion :: finnsKompis(string namnet) {
for (int i = 0; i < ant_kompisar; i++)
if (kompisar == namnet)
return true;
return false;
}


class TransaktionsLista
{
private:
int antalTrans;
Transaktion *trans;

public:
TransaktionsLista();
~TransaktionsLista();
//void laesin( istream & is );
void skrivut( ostream & os );
void laggTill( Transaktion & t );
//double totalkostnad();
//double liggerUteMed( string namnet );
double aerSkyldig( string namnet );
//PersonLista FixaPersoner();
};// End TransaktionsLista

TransaktionsLista :: TransaktionsLista()
: antalTrans(0), trans(0)
{ ; }

TransaktionsLista :: ~TransaktionsLista() {
//delete [] trans;
}

void TransaktionsLista :: laggTill(Transaktion &t) {
if (antalTrans > 0) {
Transaktion *temp = 0;
temp = new Transaktion[antalTrans+1];
for (int i = 0; i < antalTrans; i++)
temp = trans;
temp[antalTrans] = t;
delete [] trans;
trans = temp;
antalTrans++;
}
else {
trans = new Transaktion[1];
trans[0] = t;
antalTrans++;
}
}

// This one doesn't work!
double TransaktionsLista :: aerSkyldig(string namnet) {
double belopp = 0.0;
for (int i = 0; i < antalTrans; i++) {
Transaktion temp = Transaktion(trans);
if ( temp.finnsKompis(namnet) )
belopp += ( trans.haemta_belopp() /
(trans.haemta_ant_kompisar() + 1) );
}
return belopp;
}

// This one doesn't work either! - Same problem??
void TransaktionsLista :: skrivut(ostream &os) {
for (int i = 0; i < antalTrans; i++) {
Transaktion temp = Transaktion(trans);
temp.skrivEnTrans(os);
}
}

int main () {
Transaktion a;
Transaktion b;
Transaktion c;
TransaktionsLista tl;
tl.laggTill(a);
tl.laggTill(b);
tl.laggTill(c);
tl.aerSkyldig("Karl"); // <-- This doesn't work
ofstream os("temp.txt");
tl.skrivut(os); // <-- Nor this...
os.close();
return (0);
}

 

[=?ISO-8859-1?Q?Stefan_N=E4we?=]

Hi again!

Here comes a compilable program with parts of the code.
I've had quite a few problems whith when to use a destructor and when
not to.
It shouldn't be a problem to delete a NULL-pointer but sometimes I just
get the
same error as in the problem below. Thats why I've inactivated the
vital parts
of the destructors.

[code snipped]

Just two things:
1. learn to use std::vector
2. learn to use std::vector<std::string>

/S.

 

[=?iso-8859-1?q?Stephan_Br=F6nnimann?=]

Hi again!

Here comes a compilable program with parts of the code.
I've had quite a few problems whith when to use a destructor and when
not to.
It shouldn't be a problem to delete a NULL-pointer but sometimes I just
get the
same error as in the problem below. Thats why I've inactivated the
vital parts
of the destructors.

Thanks for your help!
Fredrik

I have snipped away the parts of your code that are not relevant
for my discussion ...

#include <iostream>
#include <fstream>
#include <string>

using namespace std;

class Transaktion
{
private:
string datum;
string typ;
string namn;
double belopp;
int ant_kompisar;
string *kompisar;

Use std::vector instead.

public:
Transaktion();
Transaktion(const Transaktion &t);
~Transaktion();

Rule of 3: the assignment operator is missing,
but the pointer `kompisar' is managed by the class.

//string haemta_namn();
double haemta_belopp();
int haemta_ant_kompisar();
bool finnsKompis( string namnet );
//bool laesEnTrans( istream &is );
void skrivEnTrans( ostream & os );

All methods above that do not modify the data members should
be declared as const (see also TransaktionsLista::skrivut() below).
Also consider returning a const reference for "complex" types:
const string& haemta_namn() const;

}; //End Transaktion

// Standardkonstruktor
Transaktion :: Transaktion()
: datum("060118"), typ("Food"), namn("Mike"), belopp(10.0),
ant_kompisar(3)
{
kompisar = new string[ant_kompisar];
kompisar[0] = "Fredrik";
kompisar[1] = "Karl";
kompisar[2] = "Richard";
kompisar[3] = "Linn";

Undefined behavior (runtime error): `kompisar' has only 3 elements.

}

// Kopieringskonstruktor
Transaktion :: Transaktion(const Transaktion &t)
: typ(t.typ), datum(t.datum), namn(t.namn), belopp(t.belopp),
ant_kompisar(t.ant_kompisar)
{
if (ant_kompisar > 0) {
kompisar = new string[ant_kompisar];
for (int i = 0; i < ant_kompisar; i++)
kompisar = t.kompisar;
}
else
kompisar = 0;

}

// Destruktor
Transaktion :: ~Transaktion() {
//delete [] kompisar;

If you implement the assignment operator the delete will work.

} [snipped]

void Transaktion :: skrivEnTrans(ostream &os) {
if (ant_kompisar > 0) {
if (os) {
os << datum << "\t" << typ << "\t" << namn << "\t" << belopp
<< "\t" << ant_kompisar;
for (int i = 0; i < ant_kompisar; i++)
os << "\t" << kompisar;
os << endl;
}
else cout << "output stream error";

Hint: error handling should be done at
an upper level and write to std::cerr.

}
}
[snip]


class TransaktionsLista
{
private:
int antalTrans;
Transaktion *trans; Why not use std::vector?

public:
TransaktionsLista();
~TransaktionsLista();

Again: pointer `trans' is managed by the class, but this time
both the copy constructor and assignment operator are missing.
If they do not make sense, declare them private and
never implement them:
private:
// prevent copying until needed
TransaktionsLista(const TransaktionsLista&);
// prevent assignment until needed
TransaktionsLista& operator=(const TransaktionsLista&);

//void laesin( istream & is );
void skrivut( ostream & os );
void laggTill( Transaktion & t );
//double totalkostnad();
//double liggerUteMed( string namnet );
double aerSkyldig( string namnet );
//PersonLista FixaPersoner();
};// End TransaktionsLista

TransaktionsLista :: TransaktionsLista()
: antalTrans(0), trans(0)
{ ; }

No need for `;'.

TransaktionsLista :: ~TransaktionsLista() {
//delete [] trans;

Memory leak: its now save to delete `trans'.

}

void TransaktionsLista :: laggTill(Transaktion &t) {
if (antalTrans > 0) {
Transaktion *temp = 0;
temp = new Transaktion[antalTrans+1];
for (int i = 0; i < antalTrans; i++)
temp = trans;
temp[antalTrans] = t;
delete [] trans;
trans = temp;
antalTrans++;
}
else {
trans = new Transaktion[1];
trans[0] = t;
antalTrans++;
}
}

// This one doesn't work!
double TransaktionsLista :: aerSkyldig(string namnet) {
double belopp = 0.0;
for (int i = 0; i < antalTrans; i++) {
Transaktion temp = Transaktion(trans);
if ( temp.finnsKompis(namnet) )
belopp += ( trans.haemta_belopp() /
(trans.haemta_ant_kompisar() + 1) );
}
return belopp;
}

// This one doesn't work either! - Same problem??
void TransaktionsLista :: skrivut(ostream &os) {
for (int i = 0; i < antalTrans; i++) {
Transaktion temp = Transaktion(trans);
temp.skrivEnTrans(os);

There's no need for a temporay, in particular with the folloing
declaration:
void Transaktion::skrivEnTrans(ostream& os) const;

}
}

int main () {
Transaktion a;
Transaktion b;
Transaktion c;
TransaktionsLista tl;
tl.laggTill(a);
tl.laggTill(b);
tl.laggTill(c);
tl.aerSkyldig("Karl"); // <-- This doesn't work
ofstream os("temp.txt");
tl.skrivut(os); // <-- Nor this...
os.close();
return (0);
}

Regards, Stephan

 

[Fredda]

Thanks a lot.
But I don't quite get it to work.
How do I implement the assignment operator?
Can you please give me a brief example of the "rule of 3".

Regards Fredrik

 

[=?iso-8859-1?q?Stephan_Br=F6nnimann?=]

Thanks a lot.
But I don't quite get it to work.
How do I implement the assignment operator?

Transaktion& Transaktion:perator=(const Transaktion &t)
{
// prevent self assignment: return
if (this == &t) return *this;

delete [] kompisar;
kompisar = 0;
typ = t.typ;
datum = t.datum;
namn = t.namn;
belopp = t.belopp;
ant_kompisar = t.ant_kompisar;

if (ant_kompisar > 0) {
kompisar = new string[ant_kompisar];
for (int i = 0; i < ant_kompisar; i++) {
kompisar = t.kompisar;
}
}
return *this;
}

Can you please give me a brief example of the "rule of 3".

Your class Transaktion and the problems your experiencing
with it is an excellent example. The rule is:

If you should always declare the
+ destructor,
+ copy constructor and
+ assignment operator
if your class defines one of them.

I hear your question "But when do I need one of the 3 functions?"
As a rule of thumb you should always implement the destructor
if the class manages memory itself, i.e., contains a pointer data
member.

Regards Fredrik

Again: consider how simple Transaktion gets if you use

#include <vector>
class Transaktion {
private:
std::vector<std::string> kompisar;
};

No destructor, copy constructor and assignment operator is needed now
and you can concentrate on the business logic.

Regards, Stephan
(e-mail address removed)
Open source rating and billing engine for communication networks.

 

[Earl Purple]

Thanks a lot.
But I don't quite get it to work.
How do I implement the assignment operator?
Can you please give me a brief example of the "rule of 3".

Regards Fredrik

How to implement the assignment using swap. This is what you should
prefer to use:

class Transaktion
{
private:
string datum;
string typ;
string namn;
double belopp;
int ant_kompisar;
string *kompisar;

void swap( Transaktion & other ); // make this public if you
really want
public:
Transaktion();
Transaktion(const Transaktion &t);

Transaktion& operator=( const Transaktion & );

~Transaktion();
//string haemta_namn();
double haemta_belopp();
int haemta_ant_kompisar();
bool finnsKompis( string namnet );
//bool laesEnTrans( istream &is );
void skrivEnTrans( ostream & os );
}; //End Transaktion

#include <algorithm>

Transaktion& Transaktion:perator=( const Transaktion & t )
{
Transaktion temp( t );
swap( temp );
return *this;
}

void Transaktion::swap( Transaktion & t )
{
std::swap( datum, t.datum );
std::swap( typ, t.typ );
std::swap( namn, t.typ );
std::swap( belopp );
std::swap( ant_kompisar );
std::swap( kompisar, t.kompisar );
}

Should you implement these changes? No. Change kompisar to be
std::vector< std::string >
and you won't need to implement any of the 3 as the compiler will
generate them already to
behave the way you already want.

 

[David Harmon]

On 18 Jan 2006 02:38:43 -0800 in comp.lang.c++, "Fredda"

Transaktion :: Transaktion()
: datum("060118"), typ("Food"), namn("Mike"), belopp(10.0),
ant_kompisar(3)
{
kompisar = new string[ant_kompisar];
kompisar[0] = "Fredrik";
kompisar[1] = "Karl";
kompisar[2] = "Richard";
kompisar[3] = "Linn";
}

The most important thing (as others have said) is that kompisar
should be made into std::vector<std::string>. Perhaps I will
elaborate on that in another post.

But let me point out just one error above: you allocate three array
members and then use four of them. Oops.

 

[David Harmon]

On 18 Jan 2006 02:38:43 -0800 in comp.lang.c++, "Fredda"

Here comes a compilable program with parts of the code.

I changed kompisar to vector<string> and a few other things.
the constructor and destructor are now unnecessary, but I left them
in anyway just for example.
See if you like it?


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

using namespace std;

class Transaktion
{
private:
string datum;
string typ;
string namn;
double belopp;
vector<string> kompisar;

public:
Transaktion();
Transaktion(const Transaktion &t);
~Transaktion();
//string haemta_namn();
double haemta_belopp() const;
int haemta_ant_kompisar() const;
bool finnsKompis( const string& ) const;
//bool laesEnTrans( istream &is );
void skrivEnTrans( ostream & os );
}; //End Transaktion

// Standardkonstruktor
Transaktion :: Transaktion()
: datum("060118"), typ("Food"), namn("Mike"), belopp(10.0)
{
kompisar.push_back("Fredrik");
kompisar.push_back("Karl");
kompisar.push_back("Richard");
kompisar.push_back("Linn");
}

// Kopieringskonstruktor
Transaktion :: Transaktion(const Transaktion &t)
: typ(t.typ), datum(t.datum), namn(t.namn),
belopp(t.belopp), kompisar(t.kompisar)
{
}

// Destruktor
Transaktion :: ~Transaktion() { }

double Transaktion :: haemta_belopp() const {
return belopp;
}

int Transaktion :: haemta_ant_kompisar() const {
return kompisar.size();
}

void Transaktion :: skrivEnTrans(ostream &os) {
os << datum << "\t" << typ << "\t" << namn << "\t" << belopp
<< "\t" << kompisar.size() << "\t";
copy(kompisar.begin(), kompisar.end(),
ostream_iterator<string>(os, "\t"));
os << endl;
}

bool Transaktion :: finnsKompis(const string& namnet) const {
return kompisar.end() !=
find(kompisar.begin(), kompisar.end(), namnet);
}

class TransaktionsLista
{
private:
int antalTrans;
Transaktion *trans;

public:
TransaktionsLista();
~TransaktionsLista();
//void laesin( istream & is );
void skrivut( ostream & os );
void laggTill( Transaktion & t );
//double totalkostnad();
//double liggerUteMed( string namnet );
double aerSkyldig( string namnet );
//PersonLista FixaPersoner();
};// End TransaktionsLista

TransaktionsLista :: TransaktionsLista()
: antalTrans(0), trans(0)
{ ; }

TransaktionsLista :: ~TransaktionsLista() {
//delete [] trans;
}

void TransaktionsLista :: laggTill(Transaktion &t) {
if (antalTrans > 0) {
Transaktion *temp = 0;
temp = new Transaktion[antalTrans+1];
for (int i = 0; i < antalTrans; i++)
temp = trans;
temp[antalTrans] = t;
delete [] trans;
trans = temp;
antalTrans++;
}
else {
trans = new Transaktion[1];
trans[0] = t;
antalTrans++;
}
}

double TransaktionsLista :: aerSkyldig(string namnet) {
double belopp = 0.0;
for (int i = 0; i < antalTrans; i++) {
Transaktion const & temp = trans;
if ( temp.finnsKompis(namnet) )
belopp += temp.haemta_belopp() /
(temp.haemta_ant_kompisar() + 1);
}
return belopp;
}

void TransaktionsLista :: skrivut(ostream &os) {
for (int i = 0; i < antalTrans; i++) {
trans.skrivEnTrans(os);
}
}

int main () {
Transaktion a;
Transaktion b;
Transaktion c;
TransaktionsLista tl;
tl.laggTill(a);
tl.laggTill(b);
tl.laggTill(c);
ofstream os("temp.txt");
os << tl.aerSkyldig("Karl") << '\n';
tl.skrivut(os);
os.close();
return (0);
}

 

[Fredda]

Thanks a lot Stephan!
This last post really made it all clear to me and the program is now
working great!
I noticed that most people think it's preferrable to use the
'std::vector' instead of 'the rule of 3' in my case but I think it's
important to understand how it really works.

Thanks again!
Fredrik

 

[Fredda]

Thanks all for your help!
The program is now working and I've learnt a lot about this issue.

Regards, Fredrik

 

[Mike Wahler]

Thanks a lot Stephan!
This last post really made it all clear to me and the program is now
working great!
I noticed that most people think it's preferrable to use the
'std::vector' instead of 'the rule of 3' in my case but I think it's
important to understand how it really works.

Let me try to stop your misconception right here.
'std::vector' (or any other container or feature of C++)
is not a 'replacement for using the rule of three'.

The 'rule of three' is to help ensure that objects of
user-defined types that do memory management, do it
properly.

The advice to use a container often obviates the need for
explicit memory management. This memory management
isn't necessarily part of the implementation of a user-
defined type. (note that by its very definition, the rule
of three only applies to class/struct types).


void f()
{
int *i = new int[100]; // not recommended

std::vector<int> v(100); // preferred, safer, easier.
// more maintainable

// 25 lines of other stuff

// Oops, forgot to 'delete[] i;' -- memory leak
// The vector's memory will be properly freed automatically
}

-Mike

 

[Fredda]

Sorry, but that's about what I meant...
Maybe I did express myself a little bit unclear.

/Fredrik

 

[Fredda]

I DO understand now! ;-)