General
In many places, your formatting and alignment are inconsistent. You also have typos in your comments and even a couple of typos in the #endif portion of the header include guards. All of this needs to be cleaned up because it looks sloppy and makes the code harder to read.
Adopt a consistent style—whatever it may be—and be disciplined about using it. Use a consistent number of blank lines between functions (unless you are separating them for some reason, and then use a consistent multiple of your normal number of blank lines), be consistent in whether or not you put spaces around operators, be consistent in your indentation, etc.
I also recommend that you outdent the accessibility specifiers (e.g., public:, private:, etc.) in a class definition, and that you begin your comments with a leading space for readability. I would further recommend that you always use braces around if/else blocks, even if you only have one statement there at the moment. When you come along later and add code, it is too easy to forget to add the braces and introduce a bug.
Otherwise, you have a nice solid start here. You appear to have a lot of the basic idioms down, including include guards, virtual functions, const-qualified member functions, and so on. However, you are not using member initialization lists, and I believe that you should. It has no effect on efficiency in this case, since all of your members are POD types, but it is nevertheless a good habit to get into.
Aside from these general comments, I have some further suggestions for improvements that I will present for each class implementation to hopefully make them more manageable. Even if I do not call something out again the next time that it appears, please try to carry knowledge forward!
Point class
As mentioned above, this class definition needs to be better formatted. This first time, I'll give an example of what I think good formatting would look like:
#ifndef Point_h
#define Point_h
class Point
{
public:
Point();
Point(int x, int y);
int getX() const;
int getY() const;
void setX(int x);
void setY(int y);
void print() const;
private:
int x, y;
};
#endif
Notice how I've standardized on the number of blank lines, outdented my accessibility specifiers, broke member functions up into logical groups, and so forth. This is significantly more readable.
The code file needs the same treatment applied. Also, I recommend placing documentation comments above the function definition, rather than out to the side. This makes things look much neater, especially where you have functions with multiple parameters (or parameters with long names). But consider whether you really need comments for these functions at all! It should be obvious by looking at the name that Point::Point is a constructor for class Point, so you can omit that. And "function update X" barely even makes sense. If you're going to write documentation comments, make them meaningful and professional looking. If they just restate things that are obvious from the name of the function, leave them off!
If you're going to use your current error-handling strategy, you should report them via std::cerr, rather than std::cout. But I don't think this is the best approach. In C++, you will generally want to throw an exception when you are passed an invalid parameter that you do not know how to deal with. The caller can then catch this exception and handle it however they want, including displaying an error message. This better preserves a division of responsibility: what if I want to use your Point class in a GUI or remote application where the output from std::cerr is not visible? If you used an exception-based error-handling strategy, I could catch the exception and present the error to the user however I wanted.
void Point::setX(int x)
{
if (x < 0)
{
throw std::out_of_range("Invalid value for x");
}
else
{
this->x = x;
}
}
If you didn't want to introduce the complexity of exceptions and feel that you can assign reasonable "default"/"fallback" values, then you could simply assert upon receiving an invalid parameter. This would allow the caller to fix their bug in a debugging build, without introducing any error-reporting overhead in release builds. This is more similar to what you have now:
void Point::setX(int x)
{
if (x < 0)
{
assert(!"Invalid value for x; setting to default.");
this->x = 0;
}
else
{
this->x = x;
}
}
In keeping with the "separation of concerns" argument presented above, I would not have a print method as part of my Point class. A Point object should not know how to "print" itself. That's far beyond its level of abstraction. If you wanted to facilitate printing a Point object, you could simply define a toString method:
// Obtain a formatted string that contains the x and y coordinates of this point.
std::string Point::toString() const
{
std::stringstream ss;
ss << "x: " << this->x
<< " "
<< "y: " << this->y;
return ss.str();
}
Of course, since this method doesn't actually require access to any of the private members of Point, it would enhance encapsulation to make it a free function. By doing so, you could even make it work analogously to the standard library's to_string family of functions:
std::string to_string(const Point& pt)
{
std::stringstream ss;
ss << "x: " << this->x
<< " "
<< "y: " << this->y;
return ss.str();
}
I hesitate to mention this because it's somewhat advanced and probably not something you should think about when writing code, but I recently refactored usage of my own Point class to always pass Point objects by value, instead of by const-reference. The reason is that it is a very simple class with only two member variables. Most compilers will generate more efficient code if such types are passed by value, rather than by const-reference (which is usually implemented behind-the-scenes as by-pointer). This doesn't affect correctness, of course, and if you have no idea what I'm talking about, you can just ignore it. When in doubt, especially as a beginner, it is reasonable to always pass class types by const-reference, unless you need a local copy.
Shape class
This class looks pretty good, aside from the formatting inconsistencies. I would personally put the destructor's declaration above the other member functions, since I like to have my member functions appear in the semi-standard order of: constructors, destructors, accessors, mutators, other member functions, overloaded operators, member variables.
Circle class
- The constructor's
l1 parameter is very poorly named. I have no idea what l1 means. I assume it is the center point of the circle, but the parameter certainly doesn't make that clear. Rename it to something like center or ptCenter.
- Your member variables are also very poorly named. It isn't 1970—there is no reason for single-character variable names. I personally like to append
m_ prefixes to my member variables, but I know not everyone likes that. Whether you use a prefix or not, definitely rename them to radius and center.
Constants should never be defined using macros in C++! Instead, you should declare a static const object instead, or a const object in an unnamed namespace.
Also note that the POSIX-compliant standard libraries provide an M_PI constant in the <cmath> header. You should check and see if this is available in your implementation, and if so, just use it. If it isn't, or you want to make sure that you are maximally compatible, you will need to define your own constant. But I recommend choosing a different name in order to avoid potential conflicts.
Polygon class
Don't leave stale, commented-out function definitions in the header file. This is completely fine while you're writing and debugging the code, but remove it before you check in the final results. It's just sloppy and confusing.
And if you are going to leave them, and have a very good documentation-related reason for doing so, you need to improve the associated comment beyond "no need". I hope they're not needed, as they're commented out! The more important question to be answering is why they are not needed.
There is no reason to include the Shape.h header in your Polygon.cpp file. You've already included it in the header, and the implementation does not depend on Shape per se. It requires only Polygon and Point (and, of course, cmath).
In C++, the standard library functions are always in the std namespace. The c-prefixed headers guarantee this, and the language standard says that they do not even have to provide these functions in the global namespace. Therefore, instead of sqrt, you should write std::sqrt.
It is a good habit to get into to declare all variables that you possibly can const. This rarely helps the optimizer, but it can help to prevent hard-to-find bugs. I'm of the opinion that const should have been the default, and we should have needed to use a mutable keyword to declare a non-const variable.
Also, the way you repeat yourself inside of the getDist function is confusing and hard to maintain. Introduce a separate variable to hold intermediate results. There is absolutely no cost at run-time for doing so; the optimizer eats stuff like this for lunch.
Finally, the getDist function shouldn't even be a member function of the Polygon class! Logically, the distance between two points has absolutely nothing to do with a polygon, and technically, the implementation of that function does not depend on any members of a Polygon class. Make it a free function instead, probably prototyped in the Point class's header, since it is logically related only to the Point type.
double getDist(const Point &p1, const Point &p2)
{
const double distX = (p1.getX() - p2.getX();
const double distXSquared = (distX * distX);
const double distY = (p1.getY() - p2.getY());
const double distYSquared = (distY * distY);
const double distXY = std::sqrt(distXSquared + distYSquared);
return distXY;
}
Rectangle class
Again, the names you've chosen for your parameters here are lousy. I guess l and r are supposed to stand for "left" and "right", but that is anything but obvious if I'm looking only at the header and can't see the implementation. Furthermore, there's no penalty for long descriptive parameter names, so use them! Rename these topLeft and bottomRight. (Parameters can have the same name as member variables inside of the constructor, and if the clash bothered you anyway, you could use my strategy of prefixing member variables with something like m_.)
Also, the constructor should be using a member initialization list, as discussed above. Here, it actually does affect efficiency; your current code initializes the member variables twice. Note, though, that you can't (easily) initialize the length and width member variables in an initialization list, so you'll have to do it in the function body. There is no cost to this, though, because they are plain-old-ints.
Finally, inside the body of the function, you should be calling the constructor for the temporary Point object as you initialize it, rather than making a dummy copy and resetting both of its members.
Taking all of this into account, I would rewrite the constructor to look like this:
Rectangle::Rectangle(const Point &topLeft, const Point &bottomRight)
: topLeft(topLeft)
, bottomRight(bottomRight)
{
const Point tempPoint(bottomRight.getX(), topLeft.getY());
length = getDist(topLeft, tempPoint); // remember, getDist is now a free function in Point.h!
width = getDist(tempPoint, bottomRight);
}
Personally, I like to use the virtual keyword in all derived classes, too. The compiler doesn't care about this. If a function is marked virtual in a parent class, it is always and perpetually virtual in all derived classes. But I'm forgetful and can't always see the parent class's definition, so I like to include it as a reminder. In C++11, you can even use the override specifier to prevent errors!
Triangle class
Again, I would annotate both getArea and getPerim with virtual and override.
This constructor's parameter names are a little bit better, but mostly by accident. How about naming them vertex1, vertex2, and vertex3? You already know they are "points" based on the type name—the more interesting thing to communicate is what kind of points? Well, they are the three vertices of the triangle!
Even worse than poorly-chosen parameter names are inconsistent parameter names! The names used in the Triangle.cpp file are different from the names used in the Triangle.h file! Very confusing and bad practice.
Here, too, the constructor should use an initialization list.
Standardize on a language for comments! In general, since code is written in English, I think comments should be in English, too. But if you're going to write comments in your native language, at least be consistent and do so for all comments!
Only declare variables at the point of initialization. There are no limitations on when a variable declaration can be introduced within a block, so there's no reason to declare them all at the top of the function like you're writing in old-school K&R C. This also allows you to make heavy use of const, like I mentioned earlier.
Here's the way I would have written the code:
Triangle::Triangle(const Point &vertex1, const Point &vertex2, const Point &vertex3)
: vertex1(vertex1)
, vertex2(vertex2)
, vertex3(vertex3)
, edgeA(getDist(vertex1, vertex2)) // comment goes here, if you need one
, edgeB(getDist(vertex2, vertex3))
, edgeC(getDist(vertex1, vertex3))
{ }
/* virtual */ double Triangle::getArea() const override
{
const double temp = sqrt(( edgeA + edgeB + edgeC) *
( edgeA + edgeB - edgeC) *
( edgeA - edgeB + edgeC) *
(-edgeA + edgeB + edgeC));
return (temp * 0.25);
}
Usage/Main function
Arrange your #includes in logical order, in increasing order of dependencies, much like I've done in my review here. For example:
#include <Point.h>
#include <Shape.h> // depends on Point
#include <Circle.h> // depends on Shape
#include <Polygon.h> // depends on Shape
#include <Rectangle.h> // depends on Polygon
#include <Triangle.h> // depends on Polygon
#include <iostream>
Don't use raw pointers! This is C++, so use a smart pointer class, such as std::unique_ptr. This will automatically call delete and save you a lot of headaches.
std::unique_ptr<Shape> pRectangle(new Rectangle(a, b));
std::unique_ptr<Shape> pTriangle(new Triangle(o, a, b));
std::unique_ptr<Shape> pCircle(new Circle(o, 1));
or, if you wanted an array:
std::unique_ptr<Shape> shapes[] = { make_unique<Rectangle>(a, b),
make_unique<Triangle>(o, a, b),
make_unique<Circle>(o, 1),
};
better yet:
std::array<std::unique_ptr<Shape>, 3> shapes =
{
std::unique_ptr<Shape> { new Rectangle(a, b) },
std::unique_ptr<Shape> { new Triangle(o, a, b) },
std::unique_ptr<Shape> { new Circle(o, 1) },
};
