How can I improve my code?

[krow]

Hello,

The following program compiles and it does just what I need. What I'd like to know is if there's anything (I mean "anything") in the code that could be improve in terms of good programming practice. The program is quite simple but I would just like to make sure this is the way to do it.

For example, I'm not sure about the correct place where I should declare my variables. I read somewhere I should declare them in the header file preceded by the keyword 'extern' but when I do so I get the error "storage class specified for...". Even though it works I don't know if there's anything wrong in the code.

Any suggestions, tips and/or recommendations will be much appreciated.

#include<iostream>
#include "Circle.h";

using namespace std;

int main() {
	Circle circle;
}

#include<iostream>
#include "Circle.h";
using namespace std;

Circle::Circle() {
	double radius, diameter, circumference;

	setRadius(3.3);
	radius = getRadius();

	setDiameter(radius * 2);
	diameter = getDiameter();

	setCircumference(diameter);
	circumference = getCircumference();

	cout << "The radius is  " << radius << endl;
	cout << "The diameter is " << diameter << endl;
	cout << "The circumference is " << circumference << endl;
}

void Circle::setRadius(double radius) {
	this->radius = radius;
}
double Circle::getRadius() {
	return radius;
}
void Circle::setDiameter(double diameter) {
	this->diameter = diameter;
}
double Circle::getDiameter() {
	return diameter;
}
void Circle::setCircumference(double circumference) {
	this->circumference = circumference;
}
double Circle::getCircumference() {
	return circumference;
}

#ifndef CIRCLE_H_
#define CIRCLE_H_

class Circle {
	#define PI 3.14;
	double radius, diameter, circumference;
public:
	Circle();
	void setRadius(double radius);
	double getRadius();
	void setDiameter(double diameter);
	double getDiameter();
	void setCircumference(double circumference);
	double getCircumference();
};

#endif /* CIRCLE_H_ */ 

[krow]

I actually noticed I forgot to multiply diameter by PI to get the circumference. I set PI as a constant in the header file, how can I access it in Circle.cpp ?

[Raman009]

Declare it outside the class , I don't know whether this is a good or bad programming practice , but it should work.

[Zephilinox]

using namespace std;

this pollutes the global namespace, and can allow function names, class names, or variables names to collide even if they are not used, it isn't a good practice

The class doesn't calculate anything after it is created, which means you would have to edit the classes constructor, which is pointless and every single Circle would have the same stats

void Circle::setRadius(double radius) {
	this->radius = radius;
        //calc the other variables, do this for every set
       this->diameter = radius * 2
       this->circumference = this->diameter * 3.14159265
}

note that in your constructor you don't calculate the circumference, you never use PI, which by the way shouldn't be a MACRO, use a const variable and set it to 3.14159265, or just 3.14159265 directly.

[AbstractionAnon]

Line 6: What's the point of having diameter and circumference as members of the class? Why not simply calculate them when needed?
Line 8: Why are you setting the radius to 3.3 in the default constructor?
Line 9: What is the point of this line? It sets the member radius to the rsult of the function getRadius, which is the value of the member radius. i.e. You're setting member to itself.
Line 12: Same comment as line 9.
Line 15: Same comment as line 9.
Line 23: When changing the radius, the circumference and diameter need to be recalculated.
Line 28: When changing the diameter, you don't recalculate the radius and circumference.
Line 34: When changing the circumference, you don't recalculate the radius and diameter.

Header line 5: Extraneous semicolon after define.. Not detected since it's not used.

You probably should also have a a constructor that takes an explicit radius.

  circle (double r)
  {  radius = r; }

[mik2718]

The data in the circle class should be just ONE of radius, diameter or circumference (you decide)

the other properties would then be calculated when required.

[krow]

Guys, thank you for your comments, they're all useful. I was sure some of things and lines of code I wrote could be written in a better way, that was the main purpose of my thread. It's just hard to learn a programming language without getting any kind of supervision or suggestions. All your comments are valid, thanks.