Minimal fix
As others already pointed out one of the main problems is that you allocate an inbuilt array of 13 circles and you try to access position 13 which is right outside the memory you allocated.
From your code I understand that your aim was to read your file, which actually has 12 elements, so all following loops try to read one Circle too much and use one Circle too much. Probably it would be better to use a separate variable for your "reference" circle.
Moreover you are using a "while not eof()
" loop, which is typically(always) wrong, but in this case it makes no sense, since you already know and decided that the file will contain 12 elements. So the minimal fix for your main could be:
int main() {
int x, y, radius;
const int SIZE = 12;
Circle myCircleArry[SIZE + 1];
myCircleArry[SIZE] = Circle(5, 9, 3);
cout << endl;
myCircleArry[SIZE].printCircleInfo(); cout << " : ";
ifstream Lab6DataFileHandle;
Lab6DataFileHandle.open("Lab6Data.txt");
for (int i = 0; i < SIZE; i++) {
Lab6DataFileHandle >> x;
Lab6DataFileHandle >> y;
Lab6DataFileHandle >> radius;
myCircleArry[i] = Circle(x, y, radius);
}
for (int i = 0; i < SIZE; i++) {
if (myCircleArry[SIZE].doIBumpIntoAnotherCircle(myCircleArry[i])) {
myCircleArry[i].printCircleInfo();
if (myCircleArry[SIZE] == myCircleArry[i])
{
cout << "*";
}
cout << " ; ";
}
}
Lab6DataFileHandle.close();
myCircleArry[SIZE].printCircleInfo();
}
Notice that we know that SIZE
elements are in the file, we allocate SIZE+1
slots and use the one in position SIZE as the reference one. Loops go from 0 to SIZE (excluded).
Major restructuring
This should probably go to Code Review, but since we are here...
Old includes
These
#include <cstdlib>
#include <math.h>
could become
#include <cmath>
You don't need the old C standard library.
Avoid using namespace std;
Plenty of anwsers here explain why.
My take on your Point
This is more my opinion than actual problems. Your class
class Point {
protected:
int x, y;
double operator-(const Point &def) {
return sqrt(pow((x - def.x), 2.0) +
pow((y - def.y), 2.0));
}
};
uses protected attributes, which appears usless overdesign. Moreover it disables the ability of initializing with braces.
operator-
between points shouldn't give the distance. It's really counter intuitive with the math everybody studies at school.
Later you will compare circles centers, so an operator==
is useful.
Finally why not using double
coordinates?
Result:
struct Point {
double x, y;
friend double distance(const Point &a, const Point &b) {
return sqrt(pow(a.x - b.x, 2.0) + pow(a.y - b.y, 2.0));
}
bool operator==(const Point &rhs) const {
return x == rhs.x && y == rhs.y;
}
};
My take on your Circle
This is your Circle
:
class Circle : public Point {
Honestly, a circle is not a Point, so I don't think hineritance is the right choice.
private:
int radius;
This is really personal: I'd avoid protecting the radius
attribute, allowing users to access it.
public:
Circle() {
this->x = x;
this->y = y;
this->radius = radius;
}
This is plain wrong. It tells the compiler that the default initialization must take the class (uninitialized) attributes and use them to initialize the same things, because here this->x
is the same as x
(the same holds for y
and radius
). Drop this useless thing.
Circle(int x, int y, int radius) {
this->x = x;
this->y = y;
this->radius = radius;
}
This makes sense, but my personal choice is to follow Google Style and designate object attributes with an underscore after the name. Others prefer m_
in front of the name. This allows me to avoid the this->
mess and the accidental use of those members.
void printCircleInfo() {
cout << x << " " << y << " " << radius << " ";
}
This is fine, but I'd go for a more standard inserter/extractor couple.
bool operator==(const Circle &def) {
return (x == def.x) & (y == def.y) & (radius == def.radius);
}
Wrong use of &
. You definitely want &&
.
bool doIBumpIntoAnotherCircle(Circle anotherCircle) {
if (anotherCircle.radius + radius >= *this - anotherCircle)
return true;
return false;
}
};
Terrible name (again: personal opinion). Methods should talk about the object they are applied to.
anotherCircle
is copied for no reason.
Classic if (something) return true; else return false;
which is easily substituted by return something;
.
So I'd go with:
struct Circle {
Point center_;
double radius_;
friend std::ostream& operator<<(std::ostream& os, const Circle& c) {
return os << c.center_.x << " " << c.center_.y << " " << c.radius_;
}
friend std::istream& operator>>(std::istream& is, Circle& c) {
return is >> c.center_.x >> c.center_.y >> c.radius_;
}
bool operator==(const Circle &rhs) {
return center_ == rhs.center_ && radius_ == rhs.radius_;
}
bool touches(const Circle& other) {
return radius_ + other.radius_ >= distance(center_, other.center_);
}
};
Now the main
function
Let's make the program able to read an arbitrary number of circles and compare them to your reference. Stop using inbuilt arrays and jump to std::vector<>
.
int main()
{
Circle ref = { 5, 9, 3 };
std::cout << ref << " : ";
std::vector<Circle> circles;
std::ifstream is("Lab6Data.txt");
Circle cur;
while (is >> cur) {
circles.push_back(cur);
}
for (const auto& c : circles) {
if (ref.touches(c)) {
std::cout << c;
if (ref == c) {
std::cout << " *";
}
std::cout << " ; ";
}
}
}
Notice that we can initialize Circle with braces. You can also drop the =
and go for uniform initialization.
Opening a stream is just calling the constructor. Closing will be automatically done at object destruction.
The loop now uses the extractor to read a Circle
. Honestly this is compact, but the version that I think is the most didactic is this:
while (true) {
// Read
Circle cur;
is >> cur;
// Check
if (!is) {
break;
}
// Use
circles.push_back(cur);
}
Notice the pattern: infinite loop with Read/Check/Use. Check is where we can exit the loop. First you read, than you check if the reading operation was successful or failed, than you can use the data or exit based on that. Believe me: write your loops like that, than, if you really must, change them to other forms.
Ok, we read all circles in the file. Now for all circles (notice the use of a range-based for loop with a const auto reference
) if the reference touches the current circle we print it (with a *
if it is equal to the reference).
Hope this long runt has some use...
Full code for easier copy and paste
#include <fstream>
#include <cmath>
#include <iomanip>
#include <iostream>
#include <vector>
struct Point {
double x, y;
friend double distance(const Point &a, const Point &b) {
return sqrt(pow(a.x - b.x, 2.0) + pow(a.y - b.y, 2.0));
}
bool operator==(const Point &rhs) const {
return x == rhs.x && y == rhs.y;
}
};
struct Circle {
Point center_;
double radius_;
friend std::ostream& operator<<(std::ostream& os, const Circle& c) {
return os << c.center_.x << " " << c.center_.y << " " << c.radius_;
}
friend std::istream& operator>>(std::istream& is, Circle& c) {
return is >> c.center_.x >> c.center_.y >> c.radius_;
}
bool operator==(const Circle &rhs) const {
return center_ == rhs.center_ && radius_ == rhs.radius_;
}
bool touches(const Circle& other) const {
return radius_ + other.radius_ >= distance(center_, other.center_);
}
};
int main()
{
Circle ref = { 5, 9, 3 };
std::cout << ref << " : ";
std::vector<Circle> circles;
std::ifstream is("Lab6Data.txt");
Circle cur;
while (is >> cur) {
circles.push_back(cur);
}
for (const auto& c : circles) {
if (ref.touches(c)) {
std::cout << c;
if (ref == c) {
std::cout << " *";
}
std::cout << " ; ";
}
}
}