Questions about the logic and scope of a destructor that frees a linked list from memory

Question

I have a class called Stopwatch that I'm working on (so please ignore most of the incompleteness below). Its interface is supposed to be a good abstraction of what a stopwatch is like in real life. Right now I'm trying to write the destructor, which will free the memory for the linked list that represents all the laps.

class Stopwatch
{
    typedef enum {UNSTARTED, RUNNING, PAUSED, FINISHED} state;
    typedef struct
    {
        unsigned hours;
        unsigned minutes;
        unsigned seconds;
    } time;
    typedef struct
    {
       unsigned n; // lap number
       time t; // lap time
       lap* next_ptr;
    } lap;

    public: 
    Stopwatch();
    ~Stopwatch();
    void right_button(); // corresponds to start/stop/pause
    void left_button();  // corresponds to lap/reset

    private: 
    state cur_state;
    lap* first_ptr;

}

Stopwatch::Stopwatch()
{
    cur_state = UNSTARTED;
    first_ptr = NULL;
}

Stopwatch::~Stopwatch()
{
    // Destroy all laps
    for (lap* thisptr = first_ptr; thisptr != NULL;)
    {
        lap* tempptr = thisptr; 
        thisptr = thisptr.next_ptr;
        free (tempptr); 
    }
    cur_state = FINISHED;
}

I have not actually tried to compile anything yet, but I have a few questions before I proceed.

(1) Is my logic for freeing the linked list correct? The procedure I used is

(i) Set thisptr equal to the pointer to the first lap.

(ii) While thisptr isn't NULL, store a copy of thisptr, increment thisptr, then free the memory that the copy points to

which seems correct, but then again pointers are still tricky for me.

(2) Am I supposed to set a pointer equal to NULL after using free on it? In all the code samples I've seen so far, when the writer wanted to get rid of a variable they simply used free on it. But I was reading this guy's instructions http://www.cprogramming.com/tutorial/c/lesson6.html and he says to set it equal to NULL afterwards. I've always thought his tutorials were good.

(3) Do I need to use a namespace operator when I'm referring to lap* in my destructor? I.e., do I need to write Stopwatch::lap* instead of lap* ??? Did I even declare the lap structure in the correct place inside my class?

Answer 1

1) The logic looks good to me. I can't promise it will work without trying it, but the idea is right.

2) If a pointer is visible to other code, it's a good idea to set it to null after freeing or deleteing it so everyone will understand it's no longer pointing at anything valid. But if you're in a destructor where everything's about to go away anyway, it doesn't really matter that much. If the reset functionality in left_button actually frees the list, though, you'd want to set first_ptr to null at the end of that.

3) No, you don't need to explicitly scope to your own class when you're inside it.

Answer 2

Setting a pointer to NULL after releasing it is not necessary. It can be recommended so that if there is a use after a free() the code will (should...) crash immediately and it is easier to debug. However in C++ you should NOT need it, because you should RAII and never have raw pointer with ownership.

Note also that in C++ you use new and delete, not malloc and free.

You for loop is not very idiomatic, it looks more like a while loop so it is more readable this way:

lap* thisptr = first_ptr;
while(thisptr)
{
    lap* tempptr = thisptr; 
    thisptr = thisptr.next_ptr;
    free (tempptr); 
}

The logic seem OK. However if it is not some homework project, you should change a few things:

1) Use standard containers. If you can use vector, then use it. If your code does not fit vector, rethink it to use vector ;-) You don't need a destructor if you change you code this way:

class Stopwatch
{
    ...
    typedef struct
    {
       unsigned n; // lap number
       time t; // lap time
    } lap;

    ...

    private: 
    ...
    std::list<lap> laps; // Could you use vector?

}

Note, in C++ you usually declare a struct this way:

    struct lap
    {
       unsigned n; // lap number
       time t; // lap time
    };

And you can refer to it using lap inside the Stopwatch class.

2) C++11 provides date/time utility:

class Stopwatch
{
    ...
    typedef std::chrono::system_clock::time_point time;
    ...
}

3) It is good practice to use initializer list in you constructor:

Stopwatch::Stopwatch()
  : cur_state(UNSTARTED)
{
}

You don't even need a constructor for simple cases in C++11:

class Stopwatch
{
    ...
    private: 
    state cur_state = UNSTARTED;
    ...
}

Note also that changing the state to FINISHED in the destructor is pretty much useless, as the object is... destroyed.