2
\$\begingroup\$

I have another home work from school and would like to see if I am on the right path to learning C++.

We have to do the calculation of the measures of a sphere. I did it two ways:

  1. Just calculating everything at the main
  2. Using a function

#include "stdafx.h"
#include "iostream" 
#include <cmath>



double *calcSphereMesures(double r);

double *calcSphereMesures(double r)
{
const double PI = 3.1416;
double d, area, circumference, volume;
double results[4];

d = 2*r;
circumference = PI * d;
area = (4 * PI * pow(r,2));
volume = (4 * PI * pow(r,3))/3;

results[0] = area;
results[1] = circumference;
results[2] = volume;
results[3] = d;
return results;
}

int main()
{
double r;
std::cout << "Please enter the radius of a circle: ";
std::cin >> r;
double *results = calcSphereMesures(r);

std::cout << "\n**************************************" << "\n"
<< "* Area and Circumference of A Sphere *" << "\n"
<< "**************************************" << "\n"
<< "\nRadius: " << r << "\n"
<< "Diameter: " << results[3] << "\n"
<< "Area: " << results[0] << "\n"
<< "Circumference: " << results[1] << "\n"
<< "Volume: " << results[2];
return 0;
}

I tried to check all the tips that were given to me the last time. Is that still bad code? What can I improve here and remember for future exercises?

\$\endgroup\$
  • 1
    \$\begingroup\$ Since you are using C++ you could have made a class for Sphere and make get and set methods. Get methods like getArea(), getVolume(), etc. In the Driver you will need to create an object named Sphere. Object oriented codes looks better, cleaner and simpler. \$\endgroup\$ – Ugnes Sep 12 '17 at 12:40
  • \$\begingroup\$ @Ugnes Please do not answer in comments. All suggestions for improving the code belong in answers. \$\endgroup\$ – 200_success Sep 12 '17 at 14:28
8
\$\begingroup\$

I see a number of things that may help you improve your code.

Prefer portable code

This code could be entirely portable if the #include "stdafx.h" were omitted. It's Windows specific and doesn't really help here, even on Windows.

Understand header include paths

There is a subtle difference between #include "iostream" and #include <iostream>. Although it's implementation defined, most compiler implementations is that the quotation mark form searches locally first (e.g. the current directory) and then searches system include directories if that fails. The angle bracket form usually searches in system include directories. See https://stackoverflow.com/questions/21593/what-is-the-difference-between-include-filename-and-include-filename for more.

Fix your formatting

The code is all jammed up on the left margin with no indenting. I don't know if that's how you wrote it or if it's a cut-and-paste error when posting the question, but it really makes the program hard to read. It makes no difference to the compiler, but you should strive for a good consistent style that includes visual clues for the human reader of your code.

Fix the bug

The calcSphereMesures routine is returning the address of a variable that's local to that routine. That's a bug. If you need to return multiple values, either pass in a pointer to an appropriate structure to be filled in by the routine or allocate one using new and returning that. I'd prefer the former, because it makes ownership of the structure a little cleaner (that is, the caller must both allocate and free the structure). Note that although I'm using the word structure, here, that isn't necessarily a struct -- it could just as well be an array or some other data structure.

Consider using an object

The area, circumference, etc., could all be attributes of a Sphere class. This would have the advantage that one could write sphere.diameter() instead of results[3] for example.

Use name spaces

When you #include <cmath>, it brings those functions into the std:: namespace and may also put the symbols into the global namespace. You're counting on the latter which is not guaranteed by the standard. You can fix that by using std::pow instead of just pow.

Use a better value of \$\pi\$

There's little point in asking for the precision of a double if you're going to approximate \$\pi\$ as 3.1416.

Understand the use of function declarations

A function declaration is required before the use of the function. Your separate declaration and implementation of the calcSphereMesures() function would make sense if the declaration were in its own header file, but that's not the case here. For that reason, I'd suggest eliminating the declaration -- keeping them synchronized is just another task that you can eliminate.

Check your spelling

The word "measure" is consistently misspelled within the code. It would be nice to fix that.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I believe he returns local array as a pointer, which is UB, is it correct? That is why I close voted \$\endgroup\$ – Incomputable Sep 12 '17 at 21:37
  • \$\begingroup\$ @Incomputable: yes, it's undefined behavior, but depending on compiler and OS, the code might well appear to work. \$\endgroup\$ – Edward Sep 13 '17 at 1:08
  • \$\begingroup\$ I am using the VS and if I do not include the stdafx.h library, the code does not compile. \$\endgroup\$ – EAzevedo Sep 15 '17 at 12:58
  • \$\begingroup\$ You can turn off precompiled headers and VS will compile it just fine. \$\endgroup\$ – Edward Sep 15 '17 at 13:27
3
\$\begingroup\$

As mentioned by Edward, bug-less version would look somewhat like this:

void calcSphereMeasures(double[] results, double r)
{
    const double PI = double(3.1415926535897932384626433832795);
    double d, area, circumference, volume;

    d = 2*r;
    circumference = PI * d;
    area = (4 * PI * pow(r,2));
    volume = (4 * PI * pow(r,3))/3;

    results[0] = area;
    results[1] = circumference;
    results[2] = volume;
    results[3] = d;
}

int main()
{
    ...
    double results[4]; // memory remains in this scope
    calcSphereMeasures(results, r);
    std::cout << results[0];
    ...
}

Yet it is considered unsafe as you can pass, for example, 0 as first parameter, and that will result in attempt to write to inacessible memory.

This, on the other hand, is a more readable and safe version:

struct SphereMeasures
{
    double area;
    double circumference;
    double volume;
    double d;
};

SphereMeasures calcSphereMeasures(double r)
{
    const double PI = double(3.1415926535897932384626433832795);
    SphereMeasures result;

    result.d = 2*r;
    result.circumference = PI * result.d;
    result.area = (4 * PI * pow(r,2));
    result.volume = (4 * PI * pow(r,3))/3;

    return result;
}

int main()
{
    ...
    SphereMeasures result = calcSphereMeasures(r);
    std::cout << result.area;
    ...
}

It is miles far from clean popular solutions, but it is a start.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Could you fix the two typos in the struct and funcion names, please? Also, what's the point of the double in double(3.141...)? \$\endgroup\$ – Roland Illig Sep 13 '17 at 7:28
  • \$\begingroup\$ @RolandIllig , thanks, seems to be good now. No actual point in using double(val), just the preference. \$\endgroup\$ – A. Knorre Sep 13 '17 at 10:18
-2
\$\begingroup\$

I think you could provide hours of joy to your HW assignment checker if you implement something based on the 2D case (a disc), which has the following charming solution:

#define _ -F<00||--F-OO--;
int F=00,OO=00;main(){F_OO();printf("%1.3f\n",4.*-F/OO/OO);}F_OO()
{
            _-_-_-_
       _-_-_-_-_-_-_-_-_
    _-_-_-_-_-_-_-_-_-_-_-_
  _-_-_-_-_-_-_-_-_-_-_-_-_-_
 _-_-_-_-_-_-_-_-_-_-_-_-_-_-_
 _-_-_-_-_-_-_-_-_-_-_-_-_-_-_
_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_
_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_
_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_
_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_
 _-_-_-_-_-_-_-_-_-_-_-_-_-_-_
 _-_-_-_-_-_-_-_-_-_-_-_-_-_-_
  _-_-_-_-_-_-_-_-_-_-_-_-_-_
    _-_-_-_-_-_-_-_-_-_-_-_
        _-_-_-_-_-_-_-_
            _-_-_-_
}

Note that this scales very well and in fact gets even more accurate as you scale it!

For making this 3-D, consider measuring slices of the sphere at different latitudes - which gradually increase in size up to the equator. Another fun challenge :-)

Source: IOCCC 1988.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Interesting - but in how far does this answer constitute a code review of the question? \$\endgroup\$ – ChrisWue Sep 12 '17 at 23:06
  • \$\begingroup\$ @ChrisWue: It suggests an alternative approach to that taken by the author in his/her code. \$\endgroup\$ – einpoklum Sep 12 '17 at 23:08

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.