An issue with Switch (Scope?)

Hello,

I have a function similar to this for character construction (types omitted).

The switch statement does not work. I though it is an issue of score, but setting curly brackets inside each case does not help. The actual switch statement is quite long, because there are many admissible combinations of weapons (and more than two weapons are allowed).

Can you please help with an advice? Thanks!

Code:

---

CowboyPtr ConstructCowboy(std::string name, const int loadoutcode)
{
        CowboyPtr cowboy = CowboyPtr(new Cowboy(name));
        switch (loadoutcode) {
                default:
                WeaponPtr leftweapon = WeaponPtr(new Colt());
                WeaponPtr rightweapon = WeaponPtr(new Whip());
                break;
                case 1:
                WeaponPtr leftweapon = WeaponPtr(new Colt());
                WeaponPtr rightweapon = WeaponPtr(new Rifle());
                break;
        }
        cowboy->Loadout.insert(WeaponPair(leftweapon->Range, leftweapon));
        cowboy->Loadout.insert(WeaponPair(rightweapon->Range, rightweapon));
        return cowboy;
}

---

Each case creates two variables, leftweapon and rightweapon, and they both go out of scope outside the switch statement. Define them before the switch statement and only assign to them inside the switch statement.

Ah..get it! So I need:

Code:

---

WeaponPtr leftweapon = nullptr;
WeaponPtr rightweapon = nullptr;

---

A few stylistic issues to address:
[list][*]You didn't provide the code that shows what are the types involved, but I'm going to guess that CowboyPtr and WeaponPtr are just typedefs for Cowboy* and Weapon* respectively. If so, get rid of these typedefs: they are useful when you are naming your own smart pointer type, but otherwise they just obscure the pointer type.[*]Avoid directly using the new operator where feasible. In practice, this means that if you can use a container or std::make_unique or std::make_shared, you should do so.[*]Use auto instead of repeating type names verbosely.[/list
Combined, you might end up with something like this:

Code:

---

std::unique_ptr<Cowboy> ConstructCowboy(const std::string& name, int loadoutcode)
{
    using std::make_unique;

    auto cowboy = make_unique<Cowboy>(name);
    std::unique_ptr<Weapon> leftweapon;
    std::unique_ptr<Weapon> rightweapon;
    switch (loadoutcode) {
    case 1:
        leftweapon = make_unique<Colt>();
        rightweapon = make_unique<Rifle>();
        break;
    default:
        leftweapon = make_unique<Colt>();
        rightweapon = make_unique<Whip>();
    }
    cowboy->Loadout.insert(WeaponPair(leftweapon->Range, leftweapon));
    cowboy->Loadout.insert(WeaponPair(rightweapon->Range, rightweapon));
    return cowboy;
}

---

It may not compile due to the Loadout related code (you may need to use std::move or release the unique_ptr), but then you have not shown the relevant code behind WeaponPair and Loadout so I leave those for you to fix.

Thank you very much! I will rethink the style (still very new to cpp, and it shows). Over the past few months, I have learnt so much from all of you in this forum and I greatly appreacite it.

I have a follow up question (sorry, still in the old style):

I need to instantiate a player by calling a function that returns a typed (shared) pointer, which can either be typed KnightPtr or CowboyPtr.

The pointer goes out of scope when leaving the if-statement. How can I preserve the pointer by defining it outside of the if-statment if I don't know which type will come out of the if-statement?

I guess I need some generic pointer to by cast inside the if-statement. How best to do this?

Code:

---

if (A_type == "knight") {
        KnightPtr A = ConstructKnight("\nKnight A", A_loadout);
} else {
        CowboyPtr A = ConstructCowboy("\nCowboy A", A_loadout);
}

PrintPlayer(A);

---

For example somethink like:

Code:

---

PlayerPtr A = nullptr;

if (A_type == "knight") {
        // cast A to KnightPtr
        A = ConstructKnight("\nKnight A", A_loadout);
} else {
        // cast A to CowboyPtr
        A = ConstructCowboy("\nCowboy A", A_loadout);
}

PrintPlayer(A);

---

More info: classes Knight and Cowboy inherid from Player, but are not polymorphic to it (class Player is not vitrual). The function PrintPlayer (despite its name) cannot be applied to Player but only to Knight or Cowboy, because Player does not hold all the data.

Quote:

---

Originally Posted by laserlight

A few stylistic issues to address:
[list][*]You didn't provide the code that shows what are the types involved, but I'm going to guess that CowboyPtr and WeaponPtr are just typedefs for Cowboy* and Weapon* respectively. If so, get rid of these typedefs: they are useful when you are naming your own smart pointer type, but otherwise they just obscure the pointer type.[*]Avoid directly using the new operator where feasible. In practice, this means that if you can use a container or std::make_unique or std::make_shared, you should do so.[*]Use auto instead of repeating type names verbosely.[/list
Combined, you might end up with something like this:

Code:

---

std::unique_ptr<Cowboy> ConstructCowboy(const std::string& name, int loadoutcode)
{
    using std::make_unique;

    auto cowboy = make_unique<Cowboy>(name);
    std::unique_ptr<Weapon> leftweapon;
    std::unique_ptr<Weapon> rightweapon;
    switch (loadoutcode) {
    case 1:
        leftweapon = make_unique<Colt>();
        rightweapon = make_unique<Rifle>();
        break;
    default:
        leftweapon = make_unique<Colt>();
        rightweapon = make_unique<Whip>();
    }
    cowboy->Loadout.insert(WeaponPair(leftweapon->Range, leftweapon));
    cowboy->Loadout.insert(WeaponPair(rightweapon->Range, rightweapon));
    return cowboy;
}

---

It may not compile due to the Loadout related code (you may need to use std::move or release the unique_ptr), but then you have not shown the relevant code behind WeaponPair and Loadout so I leave those for you to fix.

---

Another question that came to my mind is why does inserting e.g. weapon in loadout increase the counter of shared pointers (I am using shared)? My types are:

Code:

---

typedef std::shared_ptr<Weapon> WeaponPtr;
typedef std::pair<int, WeaponPtr> WeaponPair;

---

Quote:

---

Originally Posted by serge

More info: classes Knight and Cowboy inherid from Player, but are not polymorphic to it (class Player is not vitrual). The function PrintPlayer (despite its name) cannot be applied to Player but only to Knight or Cowboy, because Player does not hold all the data.

---

I think that this is the fundamental problem: you should make Player a polymorphic base class, defining a common interface that Knight and Cowboy can both implement, and which will be used by the PrintPlayer function.

If you really cannot do this, then it implies that Knight and Cowboy aren't really Player subclasses except for the sole purpose of inheriting implementation. Therefore, PrintPlayer no longer makes sense: you should have PrintKnight and PrintCowboy instead, with entirely separate code paths for creating one of them and printing it.

Quote:

---

Originally Posted by serge

Another question that came to my mind is why does inserting e.g. weapon in loadout increase the counter of shared pointers (I am using shared)?

---

You're copying the shared pointer, so it has to increase the reference count. After all, if the original pointer is destroyed, you still want the pointer in the pair to be around.

Quote:

---

Originally Posted by laserlight

I think that this is the fundamental problem: you should make Player a polymorphic base class, defining a common interface that Knight and Cowboy can both implement, and which will be used by the PrintPlayer function.

---

But still it should be possible to recast pointers of unrelated classes, I just can' wrap my head around how:

Something like this should doable:

Code:

---

auto A = nullptr;
if (true) {
  MyType B = foo(); // foo() returns a shared pointer
  // copy B to A with type conversion, A does not go out of scope
}

---

Quote:

---

Originally Posted by serge

But still it should be possible to recast pointers of unrelated classes, I just can' wrap my head around how:

---

Doing that reinterprets an object of one type as being of another type via pointers, and isn't what you want here. What you're probably thinking of is a pointer to void, but that is not type safe, and anyway that isn't going to work with a shared_ptr. (You could get a pointer that could be converted to void* from a shared_ptr, but once the shared_ptr goes out of scope, that pointer to void becomes invalid.)

Even if you switched from a shared_ptr so as to use a pointer to void, you have other problems: you could pass the pointer to void to PrintPlayer, but PrintPlayer would have no clue as to what is the underlying type, so you would need some way to specify it, and you'll be hardcoding PrintPlayer (and every other "generic" Player function) for each possible subclass of Player, whereas if you used polymorphism, you could add new Player subclasses without having to update PrintPlayer.

Why are you trying so hard to avoid better design?