[hulla]

This code calculates the cube-root of an entered number.
Is there anything I can improve on, or any critiques regarding my code or simply a note that may help me in the future?
I know I used a goto but in the scenario it was efficient.
Here is my code . . .

#include <iostream>
#include <conio.h>
#include <windows.h>
#include <cstdlib>
#include <iomanip>
#include <ctime>
using namespace std;
long double dUserInput = 0, dTop = 0, dBottom = 0, dDivider = 0;
int pause()
{
    cout << "Paused.\n\nYou pressed a key.\nWould you like to exit this program?" << endl;
    while(true)
    {
        cin.sync();
        cout << "(\"YES\"/\"NO\")" << endl;
        string sExitOrNot;
        cin >> sExitOrNot;
        // If entry is "yes", exit after returning 0
        if(sExitOrNot == "Yes" ||
           sExitOrNot == "yes" ||
           sExitOrNot == "YES" ||
           sExitOrNot == "yES") exit(0);
        // If entry is "no", end execution of this function
        else if(sExitOrNot == "No" ||
                sExitOrNot == "no" ||
                sExitOrNot == "nO" ||
                sExitOrNot == "NO")
                {
                    system("CLS");
                    return 0;
                }
        // Unrecognised response
        else cout << "\"" << sExitOrNot << "\" is unrecognised as a response." << endl;
    }
}

void finale(int dUserInput,int dAnswer)
{
    cout << "The cube-root of " << dUserInput << " is " << dAnswer << endl;
    system("PAUSE");
    // Return to main where it (main) will return 0
    return;
}

int main()
{
start:
    clock();
    cout << "Enter a real positive whole number of which\nyou would like to find the cube-root of." << endl;
    cin >> dUserInput;
    if(dUserInput*dUserInput*dUserInput == dUserInput)
    {
        finale(dUserInput, dUserInput);
        return 0;
    }
    // Value of top
    dTop = dUserInput;
    // Value of mid
    dDivider = dUserInput/2;
    // Value of bottom
    dBottom = 0;
    cout.precision(16);
    system("CLS");
    do
    {
        cout << "Calculating . . .\n\nPress any key to pause.\n\nComparing " << dDivider << " cubed with " << dUserInput << endl;
        system("CLS");
        if(_kbhit()) pause();
        // If the loop has been running for a sufficient time, break
        if(clock() >= 100000 || dTop-dBottom <= 0.000000000000001) break;
        // If the cube-root was found . . .
        if(dDivider*dDivider*dDivider == dUserInput)
        {
            finale(dUserInput, dDivider);
            return 0;
        }
        // If the midpoint of top and bottom cubed is too high . . .
        if(dDivider*dDivider*dDivider > dUserInput){
            dTop = dDivider;
            dDivider = (dBottom+dDivider)/2;       }

        // If the midpoint of top and bottom cubed is too low . . .
        if(dDivider*dDivider*dDivider < dUserInput){
            dBottom = dDivider;
            dDivider = (dTop+dDivider)/2;
                                                 }
    }
    while(true);
    long double plusOff = dUserInput*1.08, minusOff = dUserInput * 0.92;
    // If the cube-root is too far off to be realistic (can happen with clock), goto start
    if(!(dDivider*dDivider*dDivider >= minusOff && dDivider*dDivider*dDivider <= plusOff))
    {
        cout << "There was an error.\n\nPress enter to try again." << endl;
        cin.sync();
        cin.get();
        goto start;
    }
    cout << "The program could not calculate the exact cube-root of " << dUserInput;
    cout << "\n\nEstimate: " << dDivider << "\n\nPress enter to end this program . . ." << endl;
    cin.sync();
    cin.get();
    return 0;
}

[janotte]

You have been told better ways to do the following several times.
Look back through the threads you have started and see how to do this better.

        // If entry is "yes", exit after returning 0
        if(sExitOrNot == "Yes" ||
           sExitOrNot == "yes" ||
           sExitOrNot == "YES" ||
           sExitOrNot == "yES") exit(0);

This ugly code and a waste of your time to write all this when you have been shown better ways. Think about "toupper()" for a hint.

Worse it is faulty code.
What happens if the user enters "yEs"?

And why the use of "exit()"?
Again you have been shown better ways.

Time to go back and review what you have already been shown and incorporate it into your coding knowledge and practice.

[AKMafia001]

Why the use of global variables?

long double dUserInput = 0, dTop = 0, dBottom = 0, dDivider = 0;

They are risky and dangerous.

Well! You can either ask a user to input simple 'y' or 'Y' or the best way is,

Quote

janotte: Think about "toupper()" for a hint.

At this point I have a question... Does it compiles and give the accurate result?
Wanna move forward?

if(dUserInput*dUserInput*dUserInput == dUserInput)
    {
        finale(dUserInput, dUserInput);
        return 0;
    }

Multiplying a number three times and comparing to the original number again.... Will they match?
I guess you are doing this, For example, the user enters 2.

if( 2 * 2 * 2 == 2 ) // that's what i see if im not wrong.
{
      finale( 2, 2 );
      return 0; // Are you returning to main() here? If yes, then why?
}

The function finale() is called just to print,

cout << "The cube-root of " << dUserInput << " is " << dAnswer << endl;
[b]Prints:[/b] The cube-root of 2 is 2

You can perform the whole calculation in the function which would be considered Good Programming Practice.

Well! I will stop at this point. Odds are i might pointed out things wrong... If so i appologies.

Hope i Helped!

[hulla]

I did the if(dUserInput*dUserInput*dUserInput == dUserInput) in case the user entered one but now that I bring it up, I realize how lame I was. I could've just done this: if(dUserInput == 1) . . .

I'm currently improving my project now . . .

[baavgai]

No. Not good at all, I'm afraid, conio.h alone is a bad sign. Globals, damn near everything in main. Gotos?!? Honestly, do you have to ask?

The function pause, in addition to having that wretched compare logic ( again ), isn't. It doesn't pause...

The globals combined with gotos make the flow near impossible to follow. There are functions that return values for no reason. There are function calls (e.g. clock(); ) for no reason.

hulla, on 05 September 2011 - 03:12 AM, said:

I know I used a goto but in the scenario it was efficient.

No. This is never true. It might have seemed easier, which is more laziness than efficiency.

[AKMafia001]

Agree with baavgai! That's why i stopped in the middle because the whole code is confusing and have flaws.

[hulla]

The clock() is used to make sure that the program does not calculate for over 10 seconds. I forgot to remove it when I improved my code.

Now that clock() is gone, I can lose the goto because the goto was there to make sure that clock() did not malfunction.

Here is my new code . . .

#include <iostream>
#include <windows.h>
#include <cstdlib>
#include <iomanip>
using namespace std;

void finale(int dUserInput, int dAnswer) // Prints the answer and waits for user to press enter
{
    cout << "The cube-root of " << dUserInput << " is " << dAnswer << "\nPress enter to exit . . ." << endl;
    // Wait for the user to press enter
    cin.sync();
    cin.get();
    // Return to main where it (main) will return 0 (ending the program)
    return;
}

int main() // Main calculations go on here
{
    long double dUserInput = 0, dTop = 0, dBottom = 0, dDivider = 0;
    cout << "Enter a real positive whole number of which\nyou would like to find the cube-root of." << endl;
    cin >> dUserInput;
    if(dUserInput == 1)
    {
        finale(1, 1);
        return 0;
    }
    // Value of top
    dTop = dUserInput;
    // Value of mid
    dDivider = dUserInput/2; // Same as "dDivider = dTop/2;"
    // Value of bottom
    dBottom = 0;
    cout.precision(16);
    system("CLS");
    do
    {
        cout << "Calculating . . .\n\nPress any key to pause.\n\nComparing " << dDivider << " cubed with " << dUserInput << endl;
        system("CLS");
        // If the loop has been running for a sufficient time or the program has calculated enough, break
        if(dTop-dBottom <= 0.000000000000001) break;
        // If the cube-root was found . . .
        if(dDivider*dDivider*dDivider == dUserInput)
        {
            finale(dUserInput, dDivider);
            return 0;
        }
        // If the midpoint of top and bottom cubed is too high . . .
        if(dDivider*dDivider*dDivider > dUserInput){
            dTop = dDivider;
            dDivider = (dBottom+dDivider)/2;       }

        // If the midpoint of top and bottom cubed is too low . . .
        if(dDivider*dDivider*dDivider < dUserInput){
            dBottom = dDivider;
            dDivider = (dTop+dDivider)/2;
                                                 }
    }
    while(true);
    // If the cube-root is too far off to be realistic, goto start
    cout << "The program could not calculate the exact cube-root of " << dUserInput;
    cout << "\n\nEstimate: " << dDivider << "\n\nPress enter to end this program . . ." << endl;
    cin.sync();
    cin.get();
    return 0;
}

This post has been edited by hulla: 05 September 2011 - 03:55 AM

[janotte]

Okay. Another step along the way.

One thing to aim for is to have a single return from any function (there are rare exceptions but as a rule single returns usually reflect a sound program design).

You should definitely only have a single return from main().

Try and restructure your program to achieve a single return from main().

[baavgai]

It's an improvement. Thank you for your refactoring!

This is how I might organize it:

#include <iostream>
#include <cstdlib>
#include <iomanip>

using namespace std;

// you do this a few times, yes?
void waitForEnter() {
	cin.sync();
	cin.get();
}

// mostly because I don't have a CLS and don't want one
void clearScreen() {
	// system("CLS");
}

void finale(int dUserInput, int dAnswer) { // Prints the answer and waits for user to press enter
	cout << "The cube-root of " << dUserInput << " is " << dAnswer << "\nPress enter to exit . . ." << endl;
	// waitForEnter();
	// this is pointless, you're already returning
	// return;
}

void process(long double dUserInput) {
	if(dUserInput == 1) {
		finale(1, 1);
		return;
	}

	long double dTop = dUserInput;
	long double dDivider = dUserInput/2;
	long double dBottom = 0;
	cout.precision(16);
	clearScreen();
	while(true) {
		// just gonna change this a little, because it's busy
		// cout << "Calculating . . .\n\nPress any key to pause.\n\nComparing " << dDivider << " cubed with " << dUserInput << endl;
		cout << "Comparing " << dDivider << " cubed with " << dUserInput << endl;
		// clearScreen()// not sure why this is hread, kind of kills the above
		if(dTop-dBottom <= 0.000000000000001) {
			cout << "The program could not calculate the exact cube-root of " << dUserInput;
			cout << "\n\nEstimate: " << dDivider << "\n\nPress enter to end this program . . ." << endl;
			break;
		}
		
		if(dDivider*dDivider*dDivider == dUserInput) {
			finale(dUserInput, dDivider);
			break;
		}
		if(dDivider*dDivider*dDivider > dUserInput) {
			dTop = dDivider;
			dDivider = (dBottom+dDivider)/2;
		}

		if(dDivider*dDivider*dDivider < dUserInput) {
			dBottom = dDivider;
			dDivider = (dTop+dDivider)/2;
		}
	}
	
	// waitForEnter();
}

long double getUserInput() {
	long double dUserInput;
	cout << "Enter a real positive whole number of which\nyou would like to find the cube-root of." << endl;
	cin >> dUserInput;
	return dUserInput;
}

int main() {
	// now, if you like, you can throw some test cases at it without the keyboard
	//process(getUserInput());
	process(777*777*777);
	// process(64000);
	waitForEnter();
	
	return 0;
}

[hulla]

Oh yeah I do do line 8 a few times. Thanks

Damn that code is genius. Thank you so much Baavgai.