Memory leak in C++ program

Question

Thanks guys, for every const char * i went ahead and replaced it with string. THanks again!

Could someone help please? :/ (This is not a homework question)

#include <iostream>
#include <string>
#include <vld.h>

using namespace std;

class Time
{
private:

    string dayTime;

    int hour,
        minute;

public:

    Time(int hour, int minute, string dayTime); // constructor
    Time(); // default constructor

    void setTime();

    // using encapsulation
    const int getHour()  const  { return hour;   };
    const int getMinte() const  { return minute; };
    string getDayTime() const  { return dayTime;  };

    const int incrementHours();
    int incrementMinutes();

    friend ostream& operator<<(ostream& out, const Time tObj); // to write the objects attributes
};

class Date
{

private:
    string  month;
    int     day,
            year;

public:

    Date(string month, int day, int year);
    Date(const Date& d);// copy constructor

    Date(); // default constructor
    virtual ~Date();

    void setDate();

    string getMonth()  const { return month; };
    const int getDay()  const { return day;  };
    const int getYear() const { return year; };

    friend ostream& operator<<(ostream& out, const Date dObj); // to write the objects attributes
};

class Event
{
private:

    string eventName,
           userEvent;

    struct node
    {
        node();
        node  * nextByName;
        string eventName;
    };

    node * headByName;

public:
    Event(string eventName, const Date &myDate);
    Event();

    virtual ~Event();

    void insert(string eventName, const Date &myDate, const Time &myTime);
    void setEvent();

    string getEvent() const { return userEvent; };
    void displayByName(ostream& out) const;

};

/***************************************************/

Time::Time(int hour, int minute,
           string dayTime) : minute(minute),
                             hour(hour),
                             dayTime(dayTime)

{
    this->hour = hour;
    this->minute = minute;
    strcpy_s((char*)dayTime.c_str(), dayTime.length()+1, dayTime.c_str());
}

Time::Time() : hour(0),
               minute(0),
               dayTime(NULL)
{
}

void Time::setTime()
{
    cout << "Enter the hour: ";
    cin  >> hour;
    cout << "Enter the minute: ";
    cin  >> minute;
    cout << "is it P.M. or A.M.? ";
    cin  >> dayTime;

    this->incrementMinutes();
}

int Time::incrementMinutes()
{
    if ( minute <= 0 )
    {
        minute %= 59;
    }
    else if ( minute >= 59 )
    {
        hour++; // we have a new hour
        minute %= 59; // get the rest of the minutes
        // it is unlikely that the user will enter a 4-5 digit amount.
    }
    this->incrementHours();
    return minute;
}

const int Time::incrementHours()
{
    if ( hour > 12 )
    {
        hour %= 12;
        this->incrementHours();
    }
    else if ( hour == 0 )
    {
        hour = 12;
    }
    return hour;
}


ostream& operator<<(ostream& out, const Time tObj)
{
    if ( tObj.getMinte() < 10 )
    {
            return out << endl << tObj.getHour() << ":"
                       << "0" << tObj.getMinte() << " "
                       << tObj.getDayTime()      << "\n"
               ;
    }
    else
    {
        return out << endl << tObj.getHour() << ":"
                       << tObj.getMinte()    << " "
                       << tObj.getDayTime()  << "\n"
               ;
    }
}

/*************************************/


Date::Date(string month, int day, int year) : month(month),
                                                    day(day),
                                                    year(year)
{
    strcpy_s((char*)month.c_str(), month.length()+1, month.c_str());
    this->day = day;
    this->year = year;
}

Date::Date() : month(0),
               day(0),
               year(0)
{

}

Date::~Date()
{
}

Date::Date(const Date &d) : month(d.month),
                            day(d.day),
                            year(d.year)
{

}

void Date::setDate()
{
    cout << "enter the month: ";
    cin  >> month;
    cout << "enter the numeric day: ";
    cin  >> day;
    cout << "enter the numeric year: ";
    cin  >> year;
    cout << endl;
}

ostream& operator<<(ostream& out, Date dObj)
{
    return out << endl << dObj.getMonth() << " "
                       << dObj.getDay()   << ", "
                       << dObj.getYear()  << "\n"
                ;
}

/*****************************************/

Event::Event(string eventName, const Date &myDate) : eventName(eventName),
                                                     userEvent(userEvent),
                                                     headByName(NULL)

{
    strcpy_s((char*)eventName.c_str(), eventName.length()+1, eventName.c_str());
}

Event::Event() : eventName(NULL), userEvent(NULL), headByName(NULL)
{
}

Event::~Event()
{
    node * temp_node = NULL;
    node * current_node = headByName;

    while ( current_node )
    {
        temp_node = current_node->nextByName;
        delete current_node;
        current_node = temp_node;
    }
}

void Event::insert(string eventName, const Date &myDate, const Time &myTime)
// when we insert we dont care about the time, just the name and the date
{
    node * current_node = new node();

    if ( headByName == NULL )
    {
        headByName = current_node;
        headByName->eventName = eventName;
    }
    else
    {
        node * search_node = headByName;
        node * prev_node   = NULL;

        while ( search_node != NULL )
        {
            prev_node = search_node;
            search_node = search_node->nextByName;
        }
        if ( NULL == prev_node )
        {
            headByName = current_node;
        }
        else
        {
            prev_node->nextByName    = current_node;
        }
            current_node->nextByName = search_node;
            current_node->eventName  = eventName  ;
    }
}

void Event::displayByName(ostream& out) const
{
    cout << "Scheduled Events are: " << endl << endl;
    node  * current_node = headByName;

    while ( current_node )
    {
        strcpy_s((char*)eventName.c_str(), current_node->eventName.length()+1, current_node->eventName.c_str());
        out << eventName.c_str() << endl;
        current_node = current_node->nextByName;
    }
}

Event::node::node() : nextByName(NULL), eventName(eventName)
{
    strcpy_s((char*)eventName.c_str(), eventName.length()+1, eventName.c_str());
}

void Event::setEvent()
{
    cout << "\n\nEnter a new event! ";
    cin.getline((char*)userEvent.c_str(), 256);

}

/*****************************/

int main()
{
    Date  dObj("March", 21, 2010); // instaintiate our Date class object by allocating default date paramateres.
    Event eObj("First Day of Spring", dObj);
    Time  tObj(10,12,"PM");

    cout << "default Time is: " << tObj << endl;
    cout << "default Date is: " << dObj << endl;

    eObj.insert("First Day of Spring", dObj, tObj);
    eObj.insert("Valentines Day",   Date("February",14,2010), tObj);
    eObj.insert("New Years Day",    Date("Janurary",1, 2011), tObj);
    eObj.insert("St. Patricks Day", Date("March",17, 2010),   tObj);

    eObj.displayByName(cout);

    eObj.setEvent();
    const char * const theEvent = eObj.getEvent().c_str();
    dObj.setDate();

    eObj.insert((string)theEvent, dObj, tObj);
    tObj.setTime();

    cout << "Your event: " << theEvent << " is scheduled for: " << endl
         << dObj << "at" << tObj;

    eObj.displayByName(cout);

    cin.ignore(2);
    return 0;
}

All you've given us is the interface. Memory leaks don't happen in the interface, they happen in the implementation. :) — GManNickG
– GManNickG, Commented Mar 24, 2010 at 6:28

Potatoswatter · Accepted Answer · 2010-03-24 07:12:52Z

You're relying too much on manual memory management. It looks like you've added an object-oriented interface to a C program rather than using the features of C++. Try switching to storing strings in std::string (I see you're using one already), the linked list to std::list (EDIT: actually std::set would be better), and putting objects on the stack where possible. You shouldn't need new or any pointers in this program at all.

(My list suggestion below assumes that each event has its own separate list; sorry if that's wrong.)

    class Time
    {
    private:

        string dayTime
            ;

        int hour,
            minute
            ;

    public:

        Time(int hour, int minute, string const dayTime); // constructor
        Time(const Time& myTime); // copy constructor

        Time(); // default constructor

        virtual ~Time(); // destructor

        void setTime();

        // using encapsulation
        const int getHour()  const  { return hour;   };
        const int getMinte() const  { return minute; };
        string const getDayTime() const  { return dayTime;  };

        const int incrementHours();   
        int incrementMinutes();

        friend ostream& operator<<(ostream& out, const Time * tPtr); // to write the objects attributes
    };

    class Date
    {

    private:
        string  month;
        int     day,
                year;

    public:

        Date(const char * month, int day, int year);
        Date(const Date& d);// copy constructor

        Date(); // default constructor
        virtual ~Date();

        void setDate();

        string const getMonth()  const { return month; };
        const int getDay()  const { return day;  };
        const int getYear() const { return year; };

        friend ostream& operator<<(ostream& out, const Date * dPtr); // to write the objects attributes
    };

    class Event
    {
    private:

        string eventName
            ;
        string userEvent;

        list< string > event_names;

    public:
        Event(const char * eventName, const Date &myDate);
        Event();

        virtual ~Event();   

        void insert(const char * eventName, const Date &myDate, const Time &myTime);
        void setEvent();

        string const getEvent() const { return userEvent; };
        void displayByName(ostream& out) const;

    };

… in main …

        Date  dPtr("March", 21, 2010); // instaintiate our Date class object by allocating default date paramateres.
        Event ePtr("First Day of Spring", *dPtr);
        Time  tPtr(10,12,"PM");

Your code is quite verbose…

Well, thanks. I fixed it like that, but I have an error in my Time Copy Ctor and my time Dtor.
@lampshade: Right, I only edited the class definitions. You'll need to remove the string management code. It should suffice to delete the copy constructor and destructor completely. I think you maybe shouldn't need to define any destructors in this program.
Yeah, its all messed up.. I'll try tomorrow its late in my timezone. Thanks for all the help! =)

aJ. · Accepted Answer · 2010-03-24 06:41:08Z

1

You are not deleting eventName in Event destructor.

Event::~Event()
{
delete [] eventName;

Also,

 Time::~Time()
        {
            delete[] dayTime;
                   ^missing
        }

  Date::~Date()
        {
            delete[] month;
        }

edited Mar 24, 2010 at 6:41

answered Mar 24, 2010 at 6:21

aJ.

35.7k22 gold badges89 silver badges132 bronze badges

8 Comments

aJ. Over a year ago

There are other delete[] missing in ~Time and ~Date.... Fix all of them and check.

user40120 Over a year ago

I honestly don't think the [] operator paired with delete has a different effect than just delete.

Patrick Over a year ago

You should use delete[] when the memory was allocated with new[], and delete if it was allocated with new. The new[] operator will store the number of allocated elements in a hidden field (a piece of memory just before the allocated memory). This number is then used by delete[], so that delete[] knows for how many elements to call the destructor.

user40120 Over a year ago

Contracdictory to what I think, I went ahead and made those changes but with the same result still.

Potatoswatter Over a year ago

@Patrick, @lampshade: sometimes new[] does that. It always may so you should assume it does. You must have other leaks, as clearly you've fixed one, right?

|

Lara Bailey · Accepted Answer · 2010-03-24 08:35:05Z

This constructor (assuming that strcpy_s is something like the standard function strcpy) is illegal.

You've already initialized minute, hour and day in the initializer list so reassigning them in the constructor body seems unnecessary.

c_str() returns a read-only (possibly copy) of the controlled string so trying to write over it is potentially dangerous.

I couldn't see any obvious memory leaks but this may be the cause of some unexpected behaviour.

    Time::Time(int hour, int minute, 
               string dayTime) : minute(minute),
                                 hour(hour),
                                 dayTime(dayTime)

    {
        this->hour = hour;
        this->minute = minute;
        strcpy_s((char*)dayTime.c_str(), dayTime.length()+1, dayTime.c_str());
    }

quamrana · Accepted Answer · 2010-03-24 09:42:15Z

In your Time constructor you are initialising hour and minute twice (just inefficient), but you are initialising dayTime with a strcpy. You only need this:

Time::Time(int hour, int minute, string dayTime):
  minute(minute), hour(hour), dayTime(dayTime)
{}

Also for the default constructor you only need this:

Time::Time(): hour(0), minute(0)
{}

since dayTime is a string and will be default initialised for you.

You have similar problems in the Date constructors.

Collectives™ on Stack Overflow

Memory leak in C++ program

4 Answers 4

3 Comments

8 Comments

Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

3 Comments

8 Comments

Comments

Comments

Related