Get the width and height of a shape as user input

Question

I made this program to understand inheritance better.

Now that I'm at this basic level it's not really a problem but I can see this coming in the future and so I'm asking what's the best method to ask the user to insert the width and height of a rectangle (in this case).

The code I'm providing includes the simple way, by simply asking the user and inserting the number in a variable, then passing the variable to the setter inside the object (rect1).

I think when I will add up more shapes this can become a little confusing, is there a better way I can ask the user to input the number?

Since I'm asked to provide more context: I'm generally asking what's the best or a better method to ask input from the user that is going inside an object.

This is my version of it, that I think is good in this simple level but it will get messy with more shapes or generally more classes, I use 2 variable to ask for 2 dimensions, I will need 1 variable for every dimension and when I will create an object if I pass the wrong variable the program will work anyway, giving the wrong result.

Another way that I thought is insert an "askSomething" method in each class so that I don't need variables and every time a new object is created the io stream will be asked automatically. The "AskSomething" method is identical to a setter except it has cout and cin already inside

I don't really came up with other ideas.

The code I'm providing is a simple program that by asking height and width of a Rectangle calculate its Area, but of course by the class name Rectangle, the member names height and width and the method name getArea was pretty obvious.

main.cpp:

int main() {
    Rectangle rect1;
    int width, height;

    std::cout << "Insert the width of the rectangle: ";
    std::cin >> width;

    std::cout << "Insert the height of the rectangle: ";
    std::cin >>height;

    rect1.setHeight(height);
    rect1.setWidth(width);

    std::cout << "The area of the rectangle is: " << rect1.getArea() << std::endl;


    std::cout << "Drawing: " << std::endl;
    std::cout << " ";
    for (int i = 0; i < width+1; ++i) {
        std::cout << "-";
    }
    std::cout << std::endl;
    for (int j = 0; j < (height+1)/2; ++j) { // divided by 2 to have a better proportion
        std::cout << "|";
        for (int i = 0; i < width+1; ++i) {
            std::cout << " ";
        }
        std::cout << "|";
        std::cout << std::endl;
    }
    std::cout << " ";
    for (int i = 0; i < width+1; ++i) {
        std::cout << "-";
    }

    return 0;
}

Shape.h:

class Shape {
public:
    int getWidth() const {
        return width;
    }

    void setWidth(int width) {
        Shape::width = width;
    }

    int getHeight() const {
        return height;
    }

    void setHeight(int height) {
        Shape::height = height;
    }

protected:
    int width, height;
};

Rectangle.h:

class Rectangle: public Shape {
public:
    int getArea(){
        return width*height;
    }
};

Thanks to everyone! :)

Answer 1

Here are a number of things that may help you improve your code.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people. Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code. This affects not only code reviews, but also maintenance of the code in the future, by you or by others. In this case, for example, there are no #include directives anywhere.

Use the appropriate #includes

In order to compile and link, the main code requires the following two lines:

#include "Rectangle.h"
#incldue <iostream>

It was not difficult to infer, but see the point above.

Don't write getters and setters for every class

C++ isn't Java and writing getter and setter functions for every C++ class is not good style. Instead, move setter functionality into constructors and think very carefully about whether a getter is needed at all. In this code, getters for Shape are ever used, which emphasizes why they probably shouldn't be written in the first place. See C.131 for more details.

Rethink the class design

A class is useful because it allows us to encapsulate data and functions and assure that any necessary constrains (called invariants in computer science) are always enforced. For example, we might specify a shape for which the area must always be at least 7. However this shape is not that, so there's not much point in making the width and height as protected or private here. It makes even less sense in this particular case because the setters and getters mentioned above mean that any other piece of code can arbitrarily modify those values anyway.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized. See Sl.io.50 for more details.

Think carefully about signed vs. unsigned integers

What would it mean if a rectangle had a negative number for its width or height? If you don't have a good answer for that question, it might be better to have those data members be unsigned instead of int.

Separate input, output and calculation

To answer your main question, to the degree practical it's usually good practice to separate input, output and calculation for programs like this. By putting them in separate functions, it isolates the particular I/O for your platform (which is likely to be unique to that platform or operating system) from the logic of the objects (which does not depend on the underlying OS). So having both the input and display of the rectangle in main rather than incorporated into the Rectangle object is probably generally the better choice. However, see the next point.

Decompose your program into functions

All of the logic here is in main except for the relatively trivial calculation of area. I'd suggest instead writing functions so that your main could look like this:

int main() {
    int width = get_prompted_int("Insert the width of the rectangle: ");
    int height = get_prompted_int("Insert the height of the rectangle: ");
    Rectangle rect1(width, height);
    std::cout << "The area of the rectangle is: " 
              << rect1.getArea() 
              << "\nDrawing:\n";
    draw(std::cout, rect1);
}

Answer 2

I think you have chosen really bad way to model the inheritance relation between a rectangle and a shape.

I believe that every shape has an area. Unlike your Shape class, which does not allow this, only the Rectangle class does.

On other hand, I believe that not all shapes have width and height. If anything, they have a smallest outer rectangle which has a width and height. Unlike, your Shape class which always has width and height and allows to set it to any arbitrary value.

You should turn it inside out.

You can still have the shape be able to give you the smallest outer rectangle (with its width and height), but implementing that method for anything except the Rectangle (where it would just return copy of itself) would be a bit more complex and providing such code for you would go far beyond a code review :) But that way you can provide the width/height pair in a read only manner, making sure that it cannot be modified for shapes where it would lead to corruption (like setting width!=height for a circle).

I would also rather use double for the numeric values as you would soon find out that areas of shapes are not always integers even for shapes which themselves are defined with integers (although this is not the case for rectangles).

class Rectangle;

class Shape
{
public:
  virtual ~Shape() {}
  virtual double getArea() const = 0;
  virtual Rectangle getSmallestOuterRectangle() const = 0;
};

class Rectangle : public Shape
{
public:
  double width, height;

  Rectangle(double width, double height) : width(width), height(height) {}
  virtual ~Rectangle() {}
  virtual double getArea() const {return width * height;}
  virtual Rectangle getSmallestOuterRectangle() const {return *this;}
};

Further obviously, you cannot create all shapes from just height and width. If I wanted to create a circle, two variables are too much. If I wanted to create an arbitrary triangle, two variables are not enough.

In any way, having the Shape/Rectangle/Circle/Triangle classes know anything about streams is a mistake. You can always read the values into variables and pass them through constructor or update the respective property (if you made them public, which is ok here I believe, no setters/getters are needed).