C++ Same output while using switch inside while loop

Question

I have tried to make the following test program work for 2 days now,however its not working. It is based on several header files which work completely fine, because I checked them via another testprogram. It has header files called Area,Circle,Ring,Rectangle and Square. Also I defined function randcolor and randsize; I checked everyhting over and over, however it is producing the same ouptut aftera second try in the while loop:

int main()
{
  srand(time(NULL));
  Area *list[20];
  int m;
  Area *c;
  int j = 0;

  while (j < 20) {

    m = rand() % 4;
    cout << m << endl;

    switch (m) {
      case 0: {
        Circle a(randcolor(), randsize());
        c = &a;
        break; 
      }
      case 1: {
        Ring r(randcolor(), randsize(), randsize());
        c = &r;
        break;
      }
      case 2: {
        Rectangle re(randcolor(), randsize(), randsize());
        c = &re;
        break;
      }
      case 3: {
        Square sq(randcolor(), randsize());
        c = &sq;
        break;
      }
    }

    list[j] = c;
    j++;  
  }
  return 0;
}

Please help me Expected output should be like: 2 Area constructor is called.. 1 Area constructor is called 0 Area constructor is called

So it should be like: 20 times randomnumber between 0 and 3 "Area constructor is called..."

But it is giving the same number after the second try... in while loop

Do you understand what block-scope is, and what effect it has on the underlying objects you're addressing? `case 0` for example. as soon as the break hits, scope is left and `a` is destroyed, leaving `c` as an indeterminate pointer. Adding that to your array is icing on that UB cake. — WhozCraig, Apr 08 '14 at 00:37
@LightnessRacesinOrbit its not quite an *answer*. It is the problem, to be sure. An *possible answer* would involve using `std::vector< std::unique_ptr >` =P (at least if I were doing this). — WhozCraig, Apr 08 '14 at 00:42
How could I fix it, if I remove "break" then the cases after the chosen case will also run, is there a way of doing this so that it works? — L.G, Apr 08 '14 at 00:43
You're declaring `Circle` and other children of `Area` inside of the case statements.. as soon as the case statements exit, `Circle` is destroyed and as a result `c` points to a random memory location.. That's what WhozCraig said/means.. I think. — Brandon, Apr 08 '14 at 00:46
Does a similar thing happen when I use else if statements?, I tried it, and it is happening,so it means at the end of the if stament objects get destroyed? — L.G, Apr 08 '14 at 00:49
Yes it is the same. Anything within a local scope `on the stack` gets destroyed as soon as it leaves that scope. IE: `if (...) {object();}` Object will be destroyed as soon as it hits the second brace.. `if(...) Object();` Object is destroyed right after the `if statement` is ran. See here: http://stackoverflow.com/questions/10080935/when-is-an-object-out-of-scope — Brandon, Apr 08 '14 at 00:52

WhozCraig · Answer 1 · 2014-04-08T14:55:46.053

Not knowing how much depth you want out of this, the problem is you're pushing invalid addresses of temporary objects into your list. as each case is entered, the resulting object is created, addressed, then destroyed. the address will likely be reused on the next loop, but is invalid as soon as the scope for the last object is left.

Try this:

int main()
{
    srand ( time(NULL) );

    Area *list[20];

    int j=0;
    while(j < sizeof(list)/sizeof(*list)))
    {
        switch ( rand() % 4 )
        {
            case 0:
            {
                list[j++] = new Circle(randcolor(),randsize());
                break;
            }

            case 1:
            {
                list[j++] = new Ring(randcolor(),randsize(),randsize());
                break;
            }

            case 2:
            {
                list[j++] = new Rectangle(randcolor(),randsize(),randsize());
                break;
            }

            case 3:
            {
                list[j++] = new Square(randcolor(),randsize());
                break;
            }
        }
    }

    // TODO: Use your Area array list


    // cleanup
    for (int i=0; i<j; ++i)
        delete list[i];

    return 0;
}

I suggest spending some time learning about dynamic allocation. Once you do that, spend even more time learning about the standard library containers and smart pointers that can alleviate you of much of this headache.

Alternative (sooner or later you'll want to learn this stuff)

#include <iostream>
#include <vector>
#include <memory>
#include <random>

// include your Area and other declarations here

int main()
{
    std::random_device rd;
    std::mt19937 rng(rd());
    std::uniform_int_distribution<> dist(0,3);


    std::vector< std::unique_ptr<Area> > list;

    for (int i=0; i<20; ++i)
    {
        switch ( dist(rng) )
        {
            case 0:
                list.emplace_back(new Circle(randcolor(),randsize()));
                break;

            case 1:
                list.emplace_back(new Ring(randcolor(),randsize(),randsize()));
                break;

            case 2:
                list.emplace_back(ew Rectangle(randcolor(),randsize(),randsize()));
                break;

            case 3:
                list.emplace_back(new Square(randcolor(),randsize()));
                break;
        }
    }

    // TODO: Use your Area array list
    for (auto& ptr : list)
    {
        ptr->Method();
    }

    return 0;
}

Thanks a lot WhozCraig, however now the program is constructing a vector with same objects, all objects are the same that means one of the switch statement is being called 20 times.. — L.G, Apr 08 '14 at 01:13
Sry. was mowing the lawn. I would find that difficult to imagine unless there is a significant problem with the randXXXXX functions. Drop some std::cerr << "Here!" in the construction switch cases and make sure they're being uniformly distributed. Obiviously without more source code (like the source for all the geometrics) there is only so much I can do. I'm imagining if you're using the latter code the `srand()` should probably be put back at the top of main, since your `randXXXXX()` function likely require its seeding. — WhozCraig, Apr 08 '14 at 02:03

score 0 · Accepted Answer · answered Apr 08 '14 at 00:46

One issue with your code is that you seem to construct temporary objects in the switch cases, and once the scope is over, your c variable will not point to a valid object anymore due to the automated destruction at the end of the case scopes.

The fix would be to either move out the constructions before the switch statement, but still within the while loop, or just use pointers (maybe even smart if you wish).

I will give you an example for one switch case which could be followed for all the rest:

 case 0:
    {
        list[j++] = new Circle(randcolor(), randsize());
        break;
    }

C++ Same output while using switch inside while loop

2 Answers2