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
– WhozCraig, Commented Apr 8, 2014 at 0:37
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: stackoverflow.com/questions/10080935/… — Brandon
– Brandon, Commented Apr 8, 2014 at 0:52

WhozCraig · Accepted Answer · 2014-04-08 14:55:46Z

6

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;
}

edited Apr 8, 2014 at 14:55

answered Apr 8, 2014 at 0:48

WhozCraig

66.5k11 gold badges82 silver badges148 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

L.G Over a year ago

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..

WhozCraig Over a year ago

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.

László Papp · Accepted Answer · 2014-04-08 00:46:32Z

0

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;
    }

answered Apr 8, 2014 at 0:46

László Papp

53.5k40 gold badges118 silver badges142 bronze badges

Collectives™ on Stack Overflow

C++ Same output while using switch inside while loop

2 Answers 2

2 Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related