11
\$\begingroup\$

First off I would like to state that this is a homework assignment. However, I am not looking for you to complete anything for me. I code I am posting is the completed homework assignment. However, since this is my first time using C++ I was curious if there might have been a better method to accomplish the same task.

/*
 * PROJECT: Week 1 - Homework (Circle Calculations)
 * VERSION: 0.0.0 as of 201210104
 *
 * DESC:
 * In this homework assignment you will create a program that
 * will calculate the area and circumference of a circle.
 *
*/

#include<iostream>
using std::cout;
using std::cin;
using std::endl;

#include<cmath>;


bool main(){

    const double PI = 3.14;
    double radius;
    double circumference;
    double area;

    // display backing
    cout << "In order to determine a circle's circumference and area the";
    cout << "\nradius is required." << endl;

    // display question
    cout << "\nWhat is the radius?" << endl;
    cin >> radius;      // except input

    // do the math
    circumference = PI * 2 * radius;    // circumference of a circle
    area = PI * pow(radius,2.0);        // area of a circle

    // display answer
    cout << "\nA circle with a radius of " << radius << endl;
    cout << "has a circumference of: " <<  circumference << endl;
    cout << "has an area of: " << area << "\n" << endl;

    system("pause");

    return true;
}

Here is the outcome of all changes I have made to the code above. I was able to solve how to handle an input other than of the the required data type.

/*
    * PROJECT:  Week 1 - Homework (Circle Calculations)
    * VERSION:  1.0.0 as of 201210106
    *
    * DESC:
    * In this homework assignment you will create a program that
    * will calculate the area and circumference of a circle.
    *
*/

#include <cmath>
#include <iostream>
#include <string>
#include <sstream>
using namespace std;

bool GetInt(float & n)
{
    string str;
    getline(cin,str);

    stringstream buffer(str);
    buffer >> n;

    if (!buffer)
    {
        cout << "non numerical data!" << endl;
        return false;
    }

    if (!buffer.eof())
    {
        cout << "buffer not consumed!" << endl;
        return false;
    }

    return true;
}

int main(){

    const float PI = acos(-1);
    float radius;
    float circumference;
    float area;

    // display backing
    cout << "In order to determine a circle's circumference and area the";
    cout << "\nradius is required." << endl;

    // display question
    cout << "\nWhat is the radius? (type float)" << endl;

    while (true){
        cout << "enter a floating point number (0 to quit): ";

        if (!GetInt(radius)){
            cout << "\nyou did not enter a floating point number..." << endl;
        }
        else{

            if (radius==0)
            {
                cout << "ok, bye!" << endl;
                break;
            }

            // do the math
            circumference = PI * 2 * radius;    // circumference of a circle
            area = PI * pow(radius,2.0);        // area of a circle

            // display answer
            cout << "\nA circle with a radius of " << radius << endl;
            cout << "has a circumference of: " <<  circumference << endl;
            cout << "has an area of: " << area << endl << endl;
        }
    }

    /*
        using while loop to catch unexpected data types being
        submitted, issue and error message, and ask for another
        value to be submitted

    while(!(cin >> radius)){// loop if not cin returns false
        // display the error message
        cout << "\nThat is not a floating point number." << endl;
        cout << "Please enter a value that is a floating point number." << endl;

        // display question again
        cout << "\nWhat is the radius? (type float)" << endl;

        // cin.clear() - reset the error control state
        cin.clear();
        /*
            cin.ignore() - clear out last input sequence.
            numeric_limits<streamsize>::max() - returns the max length of the
                cin streamsize.


        cin.ignore(numeric_limits<streamsize>::max(), '\n');
    }
    */

    return 0;
}
\$\endgroup\$
7
\$\begingroup\$

Extremely good for a first time at C++! A few things though:

  • #include<iostream> should be #include <iostream>
  • Same with #include<cmath>
  • Also, no need for the ; after the cmath include
  • main does not have a return type of bool. See this or this.
  • I would put the using declarations after all includes. It should be safe, but no need to pull things into the global namespace until the last minute.
  • cin >> radius; doesn't check for failure or success (see note below)
  • In your final output, you used all endl and then have a random \n
    • \n and endl are not functionality-wise equivalent. In this situation though, that difference is not going to matter. I suggest you either stick with all endl or have all \n and then a final endl (I would probably use all endl just for the consistency). endl is essentially equivalent to writing a newline and flushing
  • I'm not a fan of system("pause");
    • It's system dependent. (The 'pause' part, not the system part.)
    • And, in my opinion, your program has no reason to remain running once it's done. Leave it up to the user whether or not to leave the prompt open.
    • Additionally, though it seems like system("pause"); is a quick little solution, it may be better to just block on reading any character if you're determined to leave the application running. system seems a bit overkill.

Note about cin >> radius;

The typical approach with handling istreams is to use the extraction as a bool:

if (cin >> radius) {
    //A double was successfully read
} else {
    //A double was not successfully read
}

To be honest, I'm not actually sure what the idiomatic way to force a user to provide a valid input is. Hopefully one of the (much) more knowledge C++ regulars will comment on that. My guess though would be to use ignore() to clear out cin's buffer and try to grab a new input.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ system("pause"); is how they are teaching to do it at the moment. I was also trying to figure out how to check for a successful input, but I wasn't able to come up with one. Perhaps that will be addressed in the next couple classes. \$\endgroup\$ – Brook Julias Oct 4 '12 at 6:27
  • 2
    \$\begingroup\$ I would put the using declarations inside main (if I used them at all). Try and constrain using declarations to the smallest scope possible. \$\endgroup\$ – Martin York Oct 5 '12 at 20:12
  • \$\begingroup\$ @BrookJulias: Tell your teacher that he is an idiot for using system("pause");. There are better ways. Most IDE will stop on exit if you configure them correctly and running from the command line you don't need it. Otherwise you can do platform neutral pause by waiting for a line to be entered. stackoverflow.com/q/2529617/14065 \$\endgroup\$ – Martin York Oct 5 '12 at 20:21
  • \$\begingroup\$ I suggest adding a validation of the input that the radius must be a positive number in order to define a circle. \$\endgroup\$ – Gürkan Çetin Jun 28 '15 at 4:39
4
\$\begingroup\$

In addition to what Corbin said, except where differing:

  • I recommend just dropping the using-declarations altogether. As you use more of the standard library they will become a pain. Also, seeing as you cannot use them everywhere (sanely), you'll end up with a mix of qualified and unqualified things.
  • If you want to use std::system, you need to include cstdlib. However, that doesn't matter because you shouldn't use std::system.
  • I do not recommend using std::pow to square things. Writing out radius*radius is not going to make your code less clear (quite the opposite, imho).
  • I do not recommend writing your own pi constant. Use std::acos(-1) or somesuch.
  • Write a function to do input sanely with error-checking and all, then just write double radius = readDouble("Enter a number: ");. Or, better auto const radius = read<double>("Enter a number: ");. See here for an example implementation. Simplify as necessary.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ So would recommend using namespace std; instead of breaking it down? Also thank you for the link. \$\endgroup\$ – Brook Julias Oct 4 '12 at 14:46
  • \$\begingroup\$ @BrookJulias: No, just explicitly qualify the names. \$\endgroup\$ – Anton Golov Oct 4 '12 at 21:15
  • \$\begingroup\$ No using declarations? Are you sure? Seems a bit overkill to be naming every single identifier everywhere (I mean, I already do it in header files, but to extend that to cpp files too...) \$\endgroup\$ – luiscubal Oct 5 '12 at 12:59
  • \$\begingroup\$ @luiscubal: I find that most identifiers are used sufficiently rarely that qualifying them explicitly (especially in the case of the std namespace) is not a problem. If any particular one gets used very often I may add a using declaration for it, but I find the inconsistent use of name vs std::name in source vs header files more of a bother than typing those extra characters. (It also helps a lot to know where a name comes from in more complex programs.) \$\endgroup\$ – Anton Golov Oct 5 '12 at 13:39
  • 1
    \$\begingroup\$ PI: stackoverflow.com/q/1727881/14065 \$\endgroup\$ – Martin York Oct 5 '12 at 20:16
2
\$\begingroup\$

Analysis of new code:

  • Stop using using namespace std;, that's definitely worse than what you had before.
  • A function called GetInt that returns a float by reference is just silly.
  • Your while(true) loop will keep going even if std::cin has reached EOF.
  • Don't keep commented-out code there like that, especially not between /* ... */ comments due to their unnestability; use #if 0 ... #endif or just get rid of the code and rely on your version control system to find it back.

For your loop, I'd suggest...

while (std::cin) {
    if (!GetFloat(radius)) {
        std::cout << "Not a float!" << std::endl;
        continue;
    }
    // Logic
}
| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

You've done it well bro. Just a little comment:

+) please remove the semi-colon

#include<cmath>;

+) put all #include stuff at the start of program before importing anything from namespace.

+) int main() and return 0; instead of yours. It's standard after all.

| improve this answer | |
\$\endgroup\$
  • 3
    \$\begingroup\$ Don't need a return 0;. In main() (in C++ not C) no return means the compiler plants a return 0 automatically. Thus if there is no way for your program to fail (as in this case) the standard is not to put a return statement (an indication to the maintainer it will not fail). Only have a specific return if there is a possibility of failure. \$\endgroup\$ – Martin York Oct 5 '12 at 20:18

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.