2
\$\begingroup\$

So I have this code:

#include <iostream>
#include <string>
#include <list>

using std::cin;
using std::cout;
using std::string;
using std::endl;
using std::list;
using std::getline;

class Saw {
private:
    string name;
    int power, speed;
public:
    Saw(string &name, int &power, int &speed) :
        name(name), power(power), speed(speed) {};
    Saw(void) :
        name(""), power(0), speed(0) {};
    virtual ~Saw(void) {};
    string getName(void) const;
    int getPower(void) const;
    int getSpeed(void) const;
    virtual void displayInfo(void) const;
};

class SawNonElectric : public Saw {
private:
    string bandType;
    bool mitre;
public:
    SawNonElectric(string &name, int &power, int &speed, string &bandType, bool mitre) :
        Saw(name, power, speed), bandType(bandType), mitre(mitre) {};
    ~SawNonElectric(void) {};
    string getBandSaw(void) const;
    bool isMiterSaw(void) const;
    virtual void displayInfo(void) const;
};

class SawElectric : public Saw {
private:
    bool variatorspeed;
    int weight;
public:
    SawElectric(string &name, int &power, int &speed, bool variatorspeed, int &weight) :
        Saw(name, power, speed), variatorspeed(variatorspeed), weight(weight) {};
    ~SawElectric(void) {};
    bool isVariator(void) const;
    int getWeigth(void) const;
    virtual void displayInfo(void) const;
};

string Saw::getName(void) const {
    return (string)name;
}

int Saw::getPower(void) const {
    return (int)power;
}

int Saw::getSpeed(void) const {
    return (int)speed;
}

void Saw::displayInfo(void) const {
    cout << endl << "Name: " << name;
    cout << endl << "Power: " << power;
    cout << endl << "Speed: " << speed;
}

string SawNonElectric::getBandSaw(void) const {
    return (string)bandType;
}

bool SawNonElectric::isMiterSaw(void) const {
    return (bool)mitre;
}

void SawNonElectric::displayInfo(void) const {
    Saw::displayInfo();
    cout << endl << "Band type: " << bandType;
    cout << endl << "Is a miter saw?: " << (mitre ? "Yes." : "No.") << endl;
}

bool SawElectric::isVariator(void) const {
    return (bool)variatorspeed;
}

int SawElectric::getWeigth(void) const {
    return (int)weight;
}

void SawElectric::displayInfo(void) const {
    Saw::displayInfo();
    cout << endl << "It has a variator speed: " << (variatorspeed ? "Yes." : "No.");
    cout << endl << "Weight: " << weight << endl;
}

Saw *readSaw(int type) {
    string name, bandType;
    int power, speed, weight;
    bool mitre, variatorspeed;

    cout << endl << endl << "Name: ", cin >> name;
    cout << "Power: ", cin >> power;
    cout << "Speed: ", cin >> speed;

    if (type == 1) { // If we read a non-electric saw.
        cout << "Band type: ", cin >> bandType;
        cout << "Is mitre? (1-yes/0-no): ", cin >> mitre;
        return (Saw*)(new SawNonElectric(name, power, speed, bandType, mitre));
    }
    else if (type == 2) { // Electric saw.
        cout << "It has a speed variator? (1-yes/0-no): ", cin >> variatorspeed;
        cout << "Weight: ", cin >> weight;
        return (Saw*)(new SawElectric(name, power, speed, variatorspeed, weight));
    }
    return NULL;
}

int main(int argc, char **argv) {

    list<Saw*> saws; // Linked list of 'Saw' objects (actually pointers to 'Saw' objects).
    int opt{ 0 };
    string aux;
    int aux2{ 0 };

    while (1) {
        cout << endl << endl << "1) Add an non-electric saw.";
        cout << endl << "2) Add an electric saw.";
        cout << endl << "3) Print all saws.";
        cout << endl << "4) Print all saws by a specific power.";
        cout << endl << "5) Delete an electric saw (by weight).";
        cout << endl << "6) Exit.";

        cout << endl << endl << "Your option: ", cin >> opt;

        switch (opt) {
        case 1:
            saws.push_back(readSaw(1)); // Add a circular non-electric Saw.
            // Sort the list.
            saws.sort([](Saw * lhs, Saw * rhs) {return (lhs->getName() < rhs->getName()); });
            break;
        case 2:
            saws.push_back(readSaw(2));
            saws.sort([](Saw * lhs, Saw * rhs) {return (lhs->getName() < rhs->getName()); });
            break;
        case 3:
            for (auto i : saws) {
                cout << "\n\n----------------------------|";
                i->displayInfo();
                cout << "----------------------------|";
            }
            break;
        case 4:
            cout << "\nPower: ", cin >> aux2;
            for (auto i : saws) {
                if (i->getPower() == aux2) {
                    i->displayInfo();
                }
            }
            break;
        case 5:
            cout << "\nWeight: ", cin >> aux2;
            for (auto i : saws) {
                SawElectric &fe = static_cast<SawElectric&>(*i);
                if (fe.getWeigth() == aux2) {
                    saws.remove(i);
                    break;
                }
                else {
                    ++i;
                }
            }
            break;
        case 6:
            for (auto i : saws) {
                delete i;
            }
            saws.clear();
            return 0;
        default:
            continue;
        }
    }

    return 0;
}

Basically I need to read two types of saws (electric and non-electric). I created a linked list (using std::list), of saw objects to store the two types to saws. (I do this by casting from derived class, to the parent class, nothing fancy).

But there appears a situation when I need to know what kind of saw I have in list (SawElectric or SawNonElectric). For solving this, I downcast all objects to a one of a inherit type (from Saw to SawElectric in my specific problem), and I do this using static_cast.

One question that bothers me is: What will happen when an object that was NonElectricSaw will be statically casted from Saw type to ElectricSaw (because I downcast all elements from list)?

Is there a way to solve this problem more correct/elegant?

Overall, what improvements can I make to this design?

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Is there a way to solve this problem more correct/elegant?

Yes. Use virtual functions.

Overall, what improvements can I make to this design?

Use virtual functions.

But there appears a situation when I need to know what kind of saw I have in list (SawElectric or SawNonElectric). For solving this, I downcast all objects to a one of a inherit type (from Saw to SawElectric in my specific problem), and I do this using static_cast.

No don't do that its dangerous.
Looking at your code you avoid the pitfall of slicing (because you use references). But all to often a downcast results in the object being sliced and you now no longer have any-type of Saw.

Your problems are caused because you expose the internal implementation of the object via getter(s) (don't do this it breaks encapsulation).

class Saw {

    // Ask yourself why do I need to access these values.
    // Is it because you are doing some external calculation.
    // Then why not get the Saw() itself to the caluculation and give you the result.

    string getName(void) const;
    int getPower(void) const;
    int getSpeed(void) const;
    virtual void displayInfo(void) const;
};

Lets look at one of these:

                saws.sort([](Saw * lhs, Saw * rhs) {return (lhs->getName() < rhs->getName()); });

You are exposing the name of the Saw just so you can sort. I think it would be easier to define the comparison operator rather than expose the internal works of your Saw.

    class Saw
    {
        public:
            friend bool operator<(Saw const& lhs, Saw const& rhs) {return lhs.name < rhs.name;}
    };

Now your code becomes:

      // No need to expose the internal representation of your object
      saws.sort([](Saw * lhs, Saw * rhs) {return *lhs < *rhs });

General Code Review

You better not be doing this in a header file:

using std::cin;
using std::cout;
using std::string;
using std::endl;
using std::list;
using std::getline;

You are polluting the namespace of anybody that uses that header file. That will get you banned from most projects. Do this in as small a scope as possible. Or better yet don't do it all (learn to type std:: at the front of stuff).

It would be nice if you could specify the stream you want to display the objects on:

virtual void displayInfo(void) const;
// I would have done
virtual void displayInfo(std::ostream& str = std::cout) const;

Since the standard way to display something is with operator<< it would be nice if you define (and use it).

std::ostream& operator<<(std::ostream& str, Saw const& data)
{
    data.displayInfo(str);
    return str;
}

If you are not going to do anything in a method that the compiler defines then best not to do anything.

 ~SawNonElectric(void) {};  // Don;t need this.
                            // You need the one in Saw (as it needs to be declared virtual).

Never use C style casts:

string Saw::getName(void) const {
    return (string)name;  // Why the cast.
}

The C style cast is to tell the compiler to shut up about errors. If you want the compiler to shut up about errors you better know what you are doing (do don't do it).

If you must cast use a C++ style cast. And then you should be very judicious about their usage (never do it). If you are doing it then you are probably doing something else wrong.

Owner ship!!!!!

 Saw *readSaw(int type) 

Your returning a pointer. Who owns that pointer? Use smart pointers to define ownership.

\$\endgroup\$
2
  • \$\begingroup\$ So using "getter(s)" is generally a bad idea? Wouldn't then better make the variable public? \$\endgroup\$ Dec 8, 2014 at 22:58
  • \$\begingroup\$ @MariusMG: No. Getters is one step better than public variables. But it is still not a good design. You should make the variables private. You should not be exposing the attributes of an object via getters/setters. But you should provide methods that perform actions on your object. So your methods should look like verbs that do stuff yo your object. class Saw { public: void print(); void cut(); void applyToZombie();} \$\endgroup\$ Dec 8, 2014 at 23:31
1
\$\begingroup\$

You definitely should not downcast to the wrong saw type as this will lead to undefined behaviour.

First of all, do you need to downcast? Can you move the getWeight() method to the base class? Then you don't need to cast at all and you can validly call the getWeight() method.

Presumably all saws have a weight.

I assume you are constrained so that you can't use dynamic_cast since I think would be the best solution if you really need to know what the specific derived type is at runtime. If you can use dynamic_cast then you could have code like this:

SawElectric* fe = dynamic_cast<SawElectric*>(i);
if( fe != nullptr && fe->getWeight() == aux2 )
{
   //remove it ...

Failing that you could roll your own rtti and add an enum and getType() method to the base class which you could use to know when its safe to downcast. A simple solution here could be just to use an enum and getType() method in the base class, for example:

enum SawType
{
    eSawUnknown,
    eSawElectric, 
    eSawNonElectric

};

class Saw
{
  SawType sawType;
public:
  SawType getSawType() const
  {
     return sawType;
  }

  //Constructors
  Saw()
    : sawType(eSawUnknown)
  {
  }

  Saw(SawType type, string &name, int &power, int &speed)
     : sawType(type), ...

}

class SawElectric
{
...
SawElectric(string &name, int &power, int &speed, bool variatorspeed, int &weight) :
    Saw(eSawElectric, name, power, speed), variatorspeed(variatorspeed), weight(weight) {};
...
}

class SawNonElectric
{
...
   SawNonElectric(string &name, int &power, int &speed, string &bandType, bool mitre) :
    Saw(eSawNonElectric,name, power, speed), bandType(bandType), mitre(mitre) {};
...
}

This has the drawback that you need to know your hierarchy up front and is not very maintainable as it can get messy quite quickly if constantly adding new classes to hierarchy but for a small hierarchy it can be okay. Usage would look like

for (auto saw : saws) 
{
   switch( saw->getSawType() )
   { 
     case eSawElectric:
     {
       SawElectric &fe = static_cast<SawElectric&>(*saw);
            if (fe.getWeigth() == aux2) 
            {
              remove it
            }            
      }
      break;

      default:
         //do nothing
      break;
 }

If roll your own RTTI is the route you want to take then I would suggest looking for better solutions than this and making sure you really need to.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.