Help me out to correct logical error in this code

Discussion in 'C++' started by anchitgood@gmail.com, Jul 10, 2008.

  1. Guest

    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.print();

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


    return 0;
    }
    , Jul 10, 2008
    #1
    1. Advertising

  2. Guest

    On 10 Jul., 08:31, 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<parkingticket> instead of a (plain) pointer.

    Regards,
    Stuart
    , Jul 10, 2008
    #2
    1. Advertising

  3. Jerry Coffin Guest

    In article <fd1feef3-a6ac-439f-ad6b-ebb9f5627c9b@
    79g2000hsk.googlegroups.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::eek:stream &
    operator<<(std::eek:stream &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::eek:stream &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.
    Jerry Coffin, Jul 10, 2008
    #3
  4. Guest

    On Jul 10, 9:05 pm, Jerry Coffin <> wrote:
    > In article <fd1feef3-a6ac-439f-ad6b-ebb9f5627c9b@
    > 79g2000hsk.googlegroups.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::eek:stream &
    >         operator<<(std::eek:stream &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::eek:stream &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..!!
    , Jul 11, 2008
    #4
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. joon
    Replies:
    1
    Views:
    515
    Roedy Green
    Jul 8, 2003
  2. sachin bond
    Replies:
    0
    Views:
    268
    sachin bond
    Sep 11, 2003
  3. Karl Heinz Buchegger
    Replies:
    8
    Views:
    314
    Alexander Terekhov
    Sep 12, 2003
  4. DannyB
    Replies:
    13
    Views:
    649
    Paul McGuire
    Feb 22, 2006
  5. Matthew
    Replies:
    3
    Views:
    85
    Matthew
    Feb 10, 2004
Loading...

Share This Page