the c++ destructor is not working properly

Question

The problem in this code is that the second object cb2 is not created nor dispalyed. There's an infinite loop after displaying the first object cb1, so I think the problem is with the destrutor at the end of the code. But, I can't figure it out.

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

#ifndef SETCOLORFUN_H_INCLUDED
#define SETCOLORFUN_H_INCLUDED
#endif

using namespace std;

class ColoredBox {

private:
  int *length;
  int *width;
  char character;
  int color;
  int maxarea;

public:
  void setColor(int ForgC) {
    WORD wColor;

    HANDLE hStdOut = GetStdHandle(STD_OUTPUT_HANDLE);
    CONSOLE_SCREEN_BUFFER_INFO csbi;

    // We use csbi for the wAttributes word.
    if (GetConsoleScreenBufferInfo(hStdOut, &csbi)) {
      // Mask out all but the background attribute, and add in the forgournd
      // color
      wColor = (csbi.wAttributes & 0xF0) + (ForgC & 0x0F);
      SetConsoleTextAttribute(hStdOut, wColor);
    }
    return;
  }

  ColoredBox(int boxlen, int boxwid, char charac = '#', int colo = 15) {
    *length = boxlen;
    *width = boxwid;
    character = charac;
    color = colo;
    maxarea = 0;
  }

  int getArea() { return (*length) * (*width); }

  int getMaxArea() {
    if (maxarea < ((*length) * (*width))) {
      maxarea = ((*length) * (*width));
    }
    return maxarea;
  }

  void display() {
    setColor(color);
    for (int i = 0; i < *length; i++) {
      for (int j = 0; j < *width; j++) {
        cout << character;
      }
      cout << endl;
    }
    setColor(15);
  }

  void displayTransposed() {
    setColor(color);
    for (int i = 0; i < *width; i++) {
      for (int j = 0; j < *length; j++) {
        cout << character;
      }
      cout << endl;
    }
    setColor(15);
  }

  void displayWider() {
    setColor(color);
    for (int i = 0; i < *length; i++) {
      for (int j = 0; j < *width; j++) {
        cout << character << " ";
      }
      cout << endl;
    }
    setColor(15);
  }

  void displayChess(int color2) {

    for (int i = 0; i < *length; i++) {
      for (int j = 0; j < *width; j++) {
        if (j % 2 == 0 && i % 2 != 0) {
          setColor(color2);
        }
        if (i % 2 == 0 && j % 2 != 0) {
          setColor(color2);
        }
        if (j % 2 == 0 && i % 2 == 0) {
          setColor(color);
        }
        if (j % 2 != 0 && i % 2 != 0) {
          setColor(color);
        }
        cout << character;
      }
      cout << endl;
    }
    setColor(15);
  }

  ~ColoredBox() {
    delete[] length;
    delete[] width;
  }
};

int main() {
  int len, wid, col, col2, max;
  char boxChar;
  cout << "Enter length and width of the box separated by a space: ";
  cin >> len >> wid;
  ColoredBox *cb1;
  cb1 = new ColoredBox(len, wid);
  cb1->display();
  cout << "Set the box to another color: ";
  cin >> col;
  cout << "Displaying Transposed: " << endl;
  cb1->setColor(col);
  cb1->displayTransposed();
  cout << "Displaying Wider: " << endl;
  cb1->displayWider();
  cout << "Displaying Chess, enter the other color: " << endl;
  cin >> col2;
  cb1->displayChess(col2);
  cout << endl << "Area=" << cb1->getArea();
  cout << endl << "Max Area=" << cb1->getMaxArea();
  delete cb1;
  cout << "Enter length,width,color,character of the box separated by spaces: ";
  cin >> len >> wid >> col >> boxChar;
  ColoredBox *cb2;
  cb2 = new ColoredBox(len, wid, col, boxChar);
  cb2->display();
  cout << "Displaying Transposed: " << endl;
  cb2->displayTransposed();
  cout << "Displaying Wider: " << endl;
  cb2->displayWider();
  cout << "Displaying Chess, enter the other color: " << endl;
  cin >> col2;
  cb2->displayChess(col2);
  cout << "Displaying original agaigetArean:" << endl;
  cb2->display();
  cout << endl << "Area=" << cb2->getArea();
  cout << endl << "Max Area=" << cb2->getMaxArea();
  delete cb2;
  return 0;
}

You can run this code the first object cb1 is created and is displayed properly in the three display functions, also i deleted cb1 and tried using cb2 only and it worked so i see that the problem is the destructor

`*length=boxlen;` - what do you think `length` points to in your constructor? You haven't initialized it to anything — UnholySheep, Nov 12 '21 at 13:18
Change `int * length ;` to `int length ;` and change `int * width ;` to `int width ;` - and then remove all the de-referencing of the above 2 variables, — Richard Critten, Nov 12 '21 at 13:19
The problem is not only the destructor, but also your mysterious decision to use pointers when there is no point and then not allocating anything for them to point to. It appears to work sometimes because undefined behaviour may sometimes appear to work. — molbdnilo, Nov 12 '21 at 13:20
`delete []length ;` was length supposed to be a dynamic array? Same question for width? Like others said, you don't want these to be pointers or a dynamic array. — drescherjm, Nov 12 '21 at 13:20
Run your program w/valgrind . If you don't know what's valgrind, look it up, it is going to save you a lot of time debugging your C++ code. — Sergey Kalinichenko, Nov 12 '21 at 13:21
Yes i removed the pointers and the deatructor and it worked, thank you guys. But, the problem is that it's a school assignment so there should be a desctructor that frees dynamic memory. But i did not know how to do it so i choose the lenght and width to be the variables to free. I will appreciate if someone tell me how to do that destructor. — Mohamed Tarek, Nov 12 '21 at 13:34
There is no dynamic memory in the `ColoredBox` class and there is also not a good reason to have dynamic allocation for that either. Maybe you were supposed to create some container class of ColoredBox and use that instead of having cb1, cb2 ... in your main — drescherjm, Nov 12 '21 at 13:48
Your header guard is not correct either. Should instead be like this example: [https://en.wikipedia.org/wiki/Include_guard#Example_2](https://en.wikipedia.org/wiki/Include_guard#Example_2) — drescherjm, Nov 12 '21 at 14:01

score 2 · Answer 1 · answered Nov 12 '21 at 14:49

As it seems you are new to C++ and you don't have the basic understanding of pointers

From MSDN

A pointer is a variable that stores the memory address of an object

The reason to use pointers is when you want to have something store at the heap, so you can access it when it goes out of scope. Check this stack overflow answer for more information

In this particular example it seems there is no need to use pointers. So change your pointer variables to integers. Remove the dereferences and the deletes[] from the destructor. Of course your code has some other problems, but here is the code without using pointers.

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

#ifndef SETCOLORFUN_H_INCLUDED
#define SETCOLORFUN_H_INCLUDED
#endif

using namespace std;

class ColoredBox {

private:
    int length;
    int width;
    char character;
    int color;
    int maxarea;

public:
    void setColor(int ForgC) {
        WORD wColor;

        HANDLE hStdOut = GetStdHandle(STD_OUTPUT_HANDLE);
        CONSOLE_SCREEN_BUFFER_INFO csbi;

        // We use csbi for the wAttributes word.
        if (GetConsoleScreenBufferInfo(hStdOut, &csbi)) {
            // Mask out all but the background attribute, and add in the forgournd
            // color
            wColor = (csbi.wAttributes & 0xF0) + (ForgC & 0x0F);
            SetConsoleTextAttribute(hStdOut, wColor);
        }
        return;
    }

    ColoredBox(int boxlen, int boxwid, char charac = '#', int colo = 15) {
        length = boxlen;
        width = boxwid;
        character = charac;
        color = colo;
        maxarea = 0;
    }

    int getArea() { return (length) * (width); }

    int getMaxArea() {
        if (maxarea < ((length) * (width))) {
            maxarea = ((length) * (width));
        }
        return maxarea;
    }

    void display() {
        setColor(color);
        for (int i = 0; i < length; i++) {
            for (int j = 0; j < width; j++) {
                cout << character;
            }
            cout << endl;
        }
        setColor(15);
    }

    void displayTransposed() {
        setColor(color);
        for (int i = 0; i < width; i++) {
            for (int j = 0; j < length; j++) {
                cout << character;
            }
            cout << endl;
        }
        setColor(15);
    }

    void displayWider() {
        setColor(color);
        for (int i = 0; i < length; i++) {
            for (int j = 0; j < width; j++) {
                cout << character << " ";
            }
            cout << endl;
        }
        setColor(15);
    }

    void displayChess(int color2) {

        for (int i = 0; i < length; i++) {
            for (int j = 0; j < width; j++) {
                if (j % 2 == 0 && i % 2 != 0) {
                    setColor(color2);
                }
                if (i % 2 == 0 && j % 2 != 0) {
                    setColor(color2);
                }
                if (j % 2 == 0 && i % 2 == 0) {
                    setColor(color);
                }
                if (j % 2 != 0 && i % 2 != 0) {
                    setColor(color);
                }
                cout << character;
            }
            cout << endl;
        }
        setColor(15);
    }

    ~ColoredBox() {}
};

int main() {
    int len, wid, col, col2, max;
    char boxChar;
    cout << "Enter length and width of the box separated by a space: ";
    cin >> len >> wid;
    ColoredBox* cb1;
    cb1 = new ColoredBox(len, wid);
    cb1->display();
    cout << "Set the box to another color: ";
    cin >> col;
    cout << "Displaying Transposed: " << endl;
    cb1->setColor(col);
    cb1->displayTransposed();
    cout << "Displaying Wider: " << endl;
    cb1->displayWider();
    cout << "Displaying Chess, enter the other color: " << endl;
    cin >> col2;
    cb1->displayChess(col2);
    cout << endl << "Area=" << cb1->getArea();
    cout << endl << "Max Area=" << cb1->getMaxArea();
    delete cb1;
    cout << "Enter length,width,color,character of the box separated by spaces: ";
    cin >> len >> wid >> col >> boxChar;
    ColoredBox* cb2;
    cb2 = new ColoredBox(len, wid, col, boxChar);
    cb2->display();
    cout << "Displaying Transposed: " << endl;
    cb2->displayTransposed();
    cout << "Displaying Wider: " << endl;
    cb2->displayWider();
    cout << "Displaying Chess, enter the other color: " << endl;
    cin >> col2;
    cb2->displayChess(col2);
    cout << "Displaying original agaigetArean:" << endl;
    cb2->display();
    cout << endl << "Area=" << cb2->getArea();
    cout << endl << "Max Area=" << cb2->getMaxArea();
    delete cb2;
    return 0;
}

I suggest you do some research and C++ pointers and memory management.

Good luck!

Marek R · Answer 2 · 2021-11-12T13:33:22.250

This is undefined behavior:

  ColoredBox(int boxlen, int boxwid, char charac = '#', int colo = 15) {
    *length = boxlen;
    *width = boxwid;

length and width are uninitialized values (one UB) so those contains some garbage values and then those pointers are dereferenced (second UB) so you write values boxlen boxwid to some random location of memory.

At the end in a destructor you are try to free memory which is not allocated, since those pointers are pointing to some random place.

Why you use pointers there at all? Why you try to use heap (I do not see reason to do it)?

the c++ destructor is not working properly

2 Answers2