Help me out to correct logical error in this code

[anchitgood]

I have tried a lot but unable to correct just 1 logical error in the
following code. This error is in the for loop, when fine and minutes
are printed as the garbage values. Please check it out:
#include<iostream>
#include<string>

using namespace std;

int m=0, min, k[10];

class parkedcar
{
string make;
int model;
string color;
string licnumber;
int minutesparked;
public:
parkedcar() //default constructor//
{
make=" ";
model=0;
licnumber=" ";
color=" ";
minutesparked=0;
}

void setmake(string mk)
{
make=mk;
}

void setmodel(int ml)
{
model=ml;
}

void setcolor(string c)
{
color=c;
}

void setlicnumber(string l)
{
licnumber=l;
}

void setminutesparked(int mnp)
{
minutesparked=mnp;
}

string getmake()
{
return (make);
}

int getmodel()
{
return (model);
}

string getcolor()
{
return (color);
}

string getlicnumber()
{
return (licnumber);
}

int getminutesparked()
{
return (minutesparked);
}

void print();
};

void parkedcar::print() //outside class definition//
{
cout<<"Car Make :"<<getmake()<<endl;
cout<<"Car Model :"<<getmodel()<<endl;
cout<<"Car License Number :"<<getlicnumber()<<endl;
cout<<"Car Color :"<<getcolor()<<endl;
}
class parkingticket: public parkedcar
{
int fine;
int minutes;
void calfine();

public:
void showfine();
void getticket (int min)
{
minutes=min;
calfine();
cout<<endl;
}
void setmin(int mnl)
{
minutes=mnl;
}
int getfine()
{
return(fine);
}
};

void parkingticket::calfine()
{
if(minutes/60<=0)
{
fine = 125;
}
else
{
int mn;
mn=minutes - 60;
fine = 125 + 100 + 100*(mn/60);
}
}

void parkingticket::showfine()
{
cout<<"ILLEGAL PARKING"<<endl;
cout<<"Time in violation is "<<minutes/60<<" hours and "<<minutes
%60<<" minutes"<<endl;
cout<<"Fine : Rs. "<<fine<<endl;
}
class parkingmeter
{
int minpurchased;
public:
parkingmeter()
{
minpurchased=0;
}
void setminpurchased(int m)
{
minpurchased=m;
}
int getminpurchased()
{
return(minpurchased);
}
void print()
{
cout<<"Mins purchased are"<<getminpurchased();
}
};

class policeofficer
{
string name;
string badgenumber;
parkingticket *ticket;
public:
policeofficer()
{
name=" ";
badgenumber=" ";
ticket = NULL;
}
void setname(string n)
{
name=n;
}
void setbadgenumber(string b)
{
badgenumber=b;
}
string getname()
{
return(name);
}
string getbadgenumber()
{
return(badgenumber);
}
parkingticket* patrol(parkingticket pc1, parkingmeter pc2)
{
if ( pc1.getminutesparked() < pc2.getminpurchased() ||
pc1.getminutesparked()==pc2.getminpurchased() )
{
return NULL;
}
else
{
parkingticket t1[10];
t1[m].getticket(pc1.getminutesparked() - pc2.getminpurchased());
t1[m].showfine();
ticket=&t1[m];
return(ticket);
}
}
void print()
{
cout<<"Name: "<<name<<endl;
cout<<"Badge Number: "<<badgenumber<<endl;
}
};
void intro()
{
cout<<"******************************************* *************************************"<<endl;
cout<<" THIS IS PARKING TICKET SIMULATOR "<<endl;
cout<<"******************************************* *************************************"<<endl;
}
int main()
{
system("cls");
int c;
int i, j=0;

intro();
cout<<"Parking rate is Rs. 75 for 60 minutes"<<endl;
cout<<"In case number of minutes car has been parked exceeds the
number of minutes purchased, then a fine is applied."<<endl;
cout<<"Fine rates :"<<endl;
cout<<"Rs 125 for first hour or part of an hour that the car is
illegally parked"<<endl;
cout<<"And Rs 100 for every additional hour or part of an hour that
the car is illegally parked."<<endl;
cout<<"--------------------------------------------------"<<endl;
//Police Officer object created
policeofficer officer;

string pname, pbadge;
cout<<"OFFICER'S INFORMATION"<<endl;
cout<<"Name: ";
cin>>pname;
cout<<"Badge Number: ";
cin>>pbadge;
officer.setname(pname);
officer.setbadgenumber(pbadge);
cout<<"--------------------------------------------------"<<endl;

//Parking Ticket object created
parkingticket tck[10];

//Parking meter object created
parkingmeter meter[10];
do
{
parkingticket* ticket = NULL;

system("cls");
while(m<=j)
{
intro();
cout<<"Plese fill in the follwing details:"<<endl;
cout<<"--------------------------------------------------"<<endl;
cout<<"VEHICLE'S INFORMATION"<<endl;

string mk;
cout<<"Make :";
cin>>mk;
tck[m].setmake(mk);

int mod;
cout<<"Model :";
cin>>mod;
tck[m].setmodel(mod);

string lic;
cout<<"License number :";
cin>>lic;
tck[m].setlicnumber(lic);

string col;
cout<<"Color :";
cin>>col;
tck[m].setcolor(col);

int parmin;
cout<<"Minutes in parking :";
cin>>parmin;
tck[m].setminutesparked(parmin);
int purmin;
cout<<"Minutes purchased :";
cin>>purmin;
meter[m].setminpurchased(purmin);

system("cls");

intro();

//The officer patrols
ticket = officer.patrol(tck[m], meter[m]);
if(ticket==NULL)
{
// Display the ticket information.
tck[m].print();
cout<<"No crimes committed"<<endl;
}
else
{
cout<<endl<<"Non-permitted vehicle ::"<<endl;
// Display the ticket information.
tck[m].print();

ticket = NULL;
}
k[m] = tck[m].getminutesparked() - meter[m].getminpurchased();
m++;
} //while loop ends
cout<<"========================================="< <endl;
cout<<"Enter your choice -"<<endl;
cout<<"0 - Patrol another car"<<endl<<"1 - View cars
patrolled"<<endl<<"2 - Exit"<<endl;
cin>>c;
if(c==0)
{
j++;
}
else if(c==1)
{
break;
}
else
{
exit(0);
}
}while (c == 0);

system("cls");
intro();
for(i=0;i<m;i++)
{
tck[i].print();

if (k[i]>0)
{
tck[i].showfine();
}
else
{
cout<<"Vehicle permitted for "<<-k[i]<<" minutes"<<endl;
}
cout<<"__________________________________"<<endl;
}
cout<<"Officer responsible for issuing these tickets and
patroling ::"<<endl;
officer.print();
return 0;
}

[dertopper]

On 10 Jul., 08:31, [e-mail] wrote:

I have tried a lot but unable to correct just 1 logical error in the
following code. This error is in the for loop, when fine and minutes
are printed as the garbage values. Please check it out:

First of all, you should only post _minimal_ examples that expose the
behaviour you want to show. Your code is over 400 lines!!! Do you
really expect any of us to read through all this?

When I run your app in the debugger, I see that policeofficer::patrol
returns an invalid pointer:

>parkingticket* patrol(parkingticket pc1, parkingmeter pc2)
{

[snip]

parkingticket t1[10];
t1[m].getticket(pc1.getminutesparked() - pc2.getminpurchased());
t1[m].showfine();
ticket=&t1[m];
return(ticket);
}
}

At this point you take the address of the local variable tl. This will
cause trouble because when this method exists, the tl will go out of
scope so that the underlying memory can be used for storing other
values.

The solution for this case would be to allocate the memory on the heap
(note that the variable tl is located on the stack) using operator
new:
parkingticket* policeofficer::patrol(parkingticket pc1, parkingmeter
pc2)
{
if (no parking violation)
return NULL;
else
{
parkingticket* RetVal = new parkingticket ();
// Fill it.
return RetVal;
}
}

In this case the caller of the method will have to delete the parking
tickets or there will be a memory leak. Since this is a mistake that
can be made quite easily, you should consider to return a
std::auto_ptr<parkingticketinstead of a (plain) pointer.

Regards,
Stuart

[Jerry Coffin]

In article <fd1feef3-a6ac-439f-ad6b-ebb9f5627c9b@
79g2000hsk.googlegroups.com>, an********@gmail.com says...

I have tried a lot but unable to correct just 1 logical error in the
following code. This error is in the for loop, when fine and minutes
are printed as the garbage values. Please check it out:

Just glancing through the code, I think you have more than one error in
logic.

#include<iostream>
#include<string>

using namespace std;

int m=0, min, k[10];

class parkedcar
{
string make;
int model;
string color;
string licnumber;
int minutesparked;
public:
parkedcar() //default constructor//
{
make=" ";
model=0;
licnumber=" ";
color=" ";
minutesparked=0;
}

void setmake(string mk)
{
make=mk;
}

void setmodel(int ml)
{
model=ml;
}

void setcolor(string c)
{
color=c;
}

void setlicnumber(string l)
{
licnumber=l;
}

void setminutesparked(int mnp)
{
minutesparked=mnp;
}

string getmake()
{
return (make);
}

int getmodel()
{
return (model);
}

string getcolor()
{
return (color);
}

string getlicnumber()
{
return (licnumber);
}

int getminutesparked()
{
return (minutesparked);
}

void print();
};

void parkedcar::print() //outside class definition//
{
cout<<"Car Make :"<<getmake()<<endl;
cout<<"Car Model :"<<getmodel()<<endl;
cout<<"Car License Number :"<<getlicnumber()<<endl;
cout<<"Car Color :"<<getcolor()<<endl;
}
class parkingticket: public parkedcar

This, for example, asserts that a parkingticket can be substituted for a
parkedcar under any possible circumstance. That sounds pretty far-
fetched at least to me.

[ ... ]

cout<<"========================================="< <endl;
cout<<"Enter your choice -"<<endl;
cout<<"0 - Patrol another car"<<endl<<"1 - View cars
patrolled"<<endl<<"2 - Exit"<<endl;
cin>>c;
if(c==0)
{
j++;
}
else if(c==1)
{
break;
}
else
{
exit(0);
}
}while (c == 0);

The logic of this for loop does not seem to match the description
(instructions) given. In C++, you should also generally forget that the
'exit' function exists -- there is virtually never a good reason to use
it.

Right now, I think your code is badly polluted with imitation
encapsulation -- most of your classes are really structs in disguise,
with the 'set' and 'get' functions accomplishing nothing useful at all.

Until/unless you do more than just pass values through the get/set
member functions, you might as well just make the data public and be
done with it. If I were doing this, I'd think hard about overloading
operator>and operator<< for some of the types involved so you can
encapsulate those parts a bit.

Just for example, I'd have the officer class something vaguely like
this:

struct officer {
std::string name_;
std::string number_;

officer(std::string const &name, std::string const &number) :
name_(name), number_(number)
{}

friend std::ostream &
operator<<(std::ostream &os, officer const &o) {
return os << "Name: " << o.name_ << "\n"
<< "Badge Number: " << o.number_ << "\n";
}
};

and possibly one more member something like:

static officer *get_data(std::istream &is, std::ostream &os) {
std::string name, number;

os << "\nPlease Enter Officer's data:\n";
os << "Name: ";
std::getline(is, name);
os << "Badge Number: ";
std::getline(is, number);
return new officer(name, number);
}

As it stands right now, the rest of your code knows about the internals
of an officer object -- that an officer has a name and a badge number,
and even that both of those are strings. What you want is for the rest
of the code to treat an officer as nothing more or less than an officer.
The officer object itself knows what's needed for an officer, how to
display data about itself, and so on.

The ticket and car data should be roughly similar -- the outside code
should know nothing about the ticket except relatively high-level
operations like 'write a ticket to a stream' (which should automatically
include write the data for the car and the ticketing officer).

--
Later,
Jerry.

The universe is a figment of its own imagination.

[anchitgood]

On Jul 10, 9:05*pm, Jerry Coffin <[e-mail]:

In article <fd1feef3-a6ac-439f-ad6b-ebb9f5627c9b@
79g2000hsk.googlegroups.com>, [e-mail] says...

I have tried a lot but unable to correct just 1 logical error in the
following code. This error is in the for loop, when fine and minutes
are printed as the garbage values. Please check it out:

Just glancing through the code, I think you have more than one error in
logic.

#include<iostream>
#include<string>

using namespace std;

int m=0, min, k[10];

class parkedcar
{
*string make;
*int model;
*string color;
*string licnumber;
*int minutesparked;
*public:
*parkedcar() *//default constructor//
*{
* make=" ";
* model=0;
* licnumber=" ";
* color=" ";
* minutesparked=0;
*}

*void setmake(string mk)
*{
* make=mk;
*}

*void setmodel(int ml)
*{
* model=ml;
*}

*void setcolor(string c)
*{
* color=c;
*}

*void setlicnumber(string l)
*{
* licnumber=l;
*}

*void setminutesparked(int mnp)
* {
* minutesparked=mnp;
*}

*string getmake()
*{
*return (make);
*}

*int getmodel()
*{
* return (model);
*}

*string getcolor()
*{
* return (color);
*}

*string getlicnumber()
*{
* return (licnumber);
*}

*int getminutesparked()
* {
* *return (minutesparked);
* }

*void print();
};

*void parkedcar::print() * //outside class definition//
*{
*cout<<"Car Make * * * * * *:"<<getmake()<<endl;
*cout<<"Car Model * * * * * :"<<getmodel()<<endl;
*cout<<"Car License Number *:"<<getlicnumber()<<endl;
*cout<<"Car Color * * * * * :"<<getcolor()<<endl;
* }

class parkingticket: public parkedcar

This, for example, asserts that a parkingticket can be substituted for a
parkedcar under any possible circumstance. That sounds pretty far-
fetched at least to me.

[ ... ]

*cout<<"=========================================" <<endl;
*cout<<"Enter your choice -"<<endl;
*cout<<"0 - Patrol another car"<<endl<<"1 - View cars
patrolled"<<endl<<"2 - Exit"<<endl;
*cin>>c;
* if(c==0)
* *{
* * *j++;
* *}
* else if(c==1)
* *{
* * *break;
* *}
* else
* {
* exit(0);
* }
*}while (c == 0);

The logic of this for loop does not seem to match the description
(instructions) given. In C++, you should also generally forget that the
'exit' function exists -- there is virtually never a good reason to use
it.

Right now, I think your code is badly polluted with imitation
encapsulation -- most of your classes are really structs in disguise,
with the 'set' and 'get' functions accomplishing nothing useful at all.

Until/unless you do more than just pass values through the get/set
member functions, you might as well just make the data public and be
done with it. If I were doing this, I'd think hard about overloading
operator>and operator<< for some of the types involved so you can
encapsulate those parts a bit.

Just for example, I'd have the officer class something vaguely like
this:

struct officer {
* * * * std::string name_;
* * * * std::string number_;

* * * * officer(std::string const &name, std::string const &number) :
* * * * * * * * name_(name), number_(number)
* * * * {}

* * * * friend std::ostream &
* * * * operator<<(std::ostream &os, officer const &o) {
* * * * * * * * return os << "Name: " << o.name_ << "\n"
* * * * * * * * * * * * << "Badge Number: " << o.number_ << "\n";
* * * * }

};

and possibly one more member something like:

* * * * static officer *get_data(std::istream &is, std::ostream &os) {
* * * * * * * * std::string name, number;

* * * * * * * * os << "\nPlease Enter Officer's data:\n";
* * * * * * * * os << "Name: ";
* * * * * * * * std::getline(is, name);
* * * * * * * * os << "Badge Number: ";
* * * * * * * * std::getline(is, number);
* * * * * * * * return new officer(name, number);
* * * * }

As it stands right now, the rest of your code knows about the internals
of an officer object -- that an officer has a name and a badge number,
and even that both of those are strings. What you want is for the rest
of the code to treat an officer as nothing more or less than an officer.
The officer object itself knows what's needed for an officer, how to
display data about itself, and so on.

The ticket and car data should be roughly similar -- the outside code
should know nothing about the ticket except relatively high-level
operations like 'write a ticket to a stream' (which should automatically
include write the data for the car and the ticketing officer).

--
* * Later,
* * Jerry.

The universe is a figment of its own imagination.- Hide quoted text -

- Show quoted text -- Hide quoted text -

- Show quoted text -

I will definitely change it and make it look better and more simpler.
Thanks for your advice. I welcome more suggestions like this.
Cheers..!!