C++

Forum

 
Code Clean up help!

thtGamerGuy (3)
I was wondering if someone was willing to clean up my code, I want to take in as much criticism or advice as possible to advance me.
Thanks! 

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
 
#include <iostream>
#include <string>
using namespace std;

int main()
{
    string mostRecentFunction;
    bool keepGoing = true;
    string continuing;
    float answer;
    float x;
    float y;
    while(keepGoing == true)
    {
    cout << "Welcome to the calculator! your most recent function was: "<< mostRecentFunction << "\n\n";
    cout << "Would you like to add or subtract?\n\n";
    cin >> mostRecentFunction;


    if((mostRecentFunction == "Subtract")||(mostRecentFunction == "subtract"))
    {
        cout<<"\n\nInsert the number you want to have subtracted\n\n";
        cin >> x;

        cout << "\n\nInsert the amount you want subtracted\n\n";
        cin >> y;

        answer = x - y;
        cout <<"\n\n" << x << " - " << y << " is " << answer<<endl;
    }

    else if((mostRecentFunction == "Add")||(mostRecentFunction == "add"))
    {
        cout << "\nInsert the first of the two numbers you want to add\n"<<endl;
        cin>>x;
        cout<<"\nInsert the second of the two numbers you want to add\n"<<endl;
        cin>>y;

        answer = x+y;
        cout <<"\n\n" << x << " + " << y << " is " << answer<<endl;
    }

    else
    {
        cout<<"\nSorry, this is not a viable option! :-("<<endl;
    }
   cout<< "would you like to continue?"<<endl;
   cin >> continuing;
   if(continuing == "yes"||continuing== "Yes"){
        keepGoing = true;
   }
   else
   {
       keepGoing = false;
   }
   }
    return 0;
}
MiiNiPaa (8886)
Just a quick one:
while(keepGoing == true) is redundand, while(keepGoing) is enough.
Same with lines 49-55: keepGoing = (continuing == "yes"||continuing== "Yes");

Also it is a good idea to convert all letters to lowercase to avoid multiple conditions and allow user to enter string how he likes: all uppercase, mixed case, etc...
eraggo (181)
Code looked good, for beginner,

I have no knowledge about your level of skills on C++ so i did (ehm..) some "small" changes in your code to illustrate some good (as far as i know) things.

I Have too much free time on hand

Yes I did broke it a part and made some functions (which may or not to be familiar to you) but them are essential in future if you try to advance next level(s). I also tried to comment original code places needed.

Don't get shocked! Everything will be like a nature soon if you keep up coding :)

And yes; code is longer (in row count), but LOOK AT MAIN function
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
 
#include <iostream>
#include <cstring>
using namespace std;

// Infoscreen at beginning of program and after each iteration
void showInfo(string mostRecentFunction)
{
	// It is good habit to make a check something is not empty...
	// Aka more functionality :P
	if (mostRecentFunction != "")
	{
		cout << "Your most recent function was: "<< mostRecentFunction << "\n\n";
		cout << "Would you like to add or subtract?\n\n";
	}
	else
	{
		cout << "Welcome to the calculator!\n\n";
		cout << "Would you like to add or susbract?\n\n";
	}
}

// Substracting procedure
void substract()
{
	float x=0.0;
	float y=0.0;
	float answer=0.0;
	cout<<"\n\nInsert the number you want to have subtracted\n\n";
	cin >> x;
	cout << "\n\nInsert the amount you want subtracted\n\n";
	cin >> y;
	answer = x - y;
	cout <<"\n\n" << x << " - " << y << " is " << answer<<endl;
}

// Adding procedure
void add()
{
	float x=0.0;
	float y=0.0;
	float answer=0.0;
	cout << "\nInsert the first of the two numbers you want to add\n"<<endl;
	cin>>x;
	cout<<"\nInsert the second of the two numbers you want to add\n"<<endl;
	cin>>y;
	answer = x+y;
	cout <<"\n\n" << x << " + " << y << " is " << answer<<endl;
}

// Is user having any intetion to continue?
bool wantToContinue()
{
	string c="";
	cout<< "would you like to continue?"<<endl;
	cin >> c;
	if(c == "yes"||c== "Yes")
		return true;
	else
		return false;
}

int main()
{
	string mostRecentFunction=""; // Just for good practise
	bool keepGoing = true;
	string continuing="";

	while(keepGoing) //Just for MiiNiPaa's comment
	{
		showInfo(mostRecentFunction); // Helps for reading code. Look below for more
		cin >> mostRecentFunction;

		// Had no idea how long C++ your experience is so far, so i did left
		// switch-case out of it based on code lenght
		// Notice difference on main and how easy it is to read?
		// remove comments if ou want to see difference
		if((mostRecentFunction == "Subtract")||(mostRecentFunction == "subtract"))
			substract();
		else if((mostRecentFunction == "Add")||(mostRecentFunction == "add"))
			add();
		else
			cout<<"\nSorry, this is not a viable option! :-("<<endl;

		//does user wants to continue?
		// remember to add "not viable option" to function i created
		keepGoing = wantToContinue();
	}

    return 0;
}


Have fun!
Last edited on
andywestken (4087)
1. std::strings default constructor inits the contained string to "" so you should not use e.g. string continuing="";

string continuing; shows you understand how classes behave!

2. string parameters should be passed to functions by const ref.

3. main should be pretty much the first function defined. It's the start of the app and makes sense for it to be defined first so you don't have to scroll down to find out how the program begins (exceptions being miniscule inline functions.)

4. double is the preferred floating point type unless you have good reason to do otherwise.

5. converting input strings to lowercase as suggested earlier in thread simplifies things.

Also corrected the spelling of subtract

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
 
#include <iostream>
#include <string>
#include <cctype>
using namespace std;

void showInfo(const string& functionName);
bool wantToContinue();

void add();
void subtract();

inline void makelower(string& str)
{
    const int len = str.length();
    for(int i = 0; i < len; ++i)
        str[i] = static_cast<char>(tolower(str[i]));
}

int main()
{
    string functionName; // strings init themselves!
    bool keepGoing = true;

    while(keepGoing)
    {
        showInfo(functionName); // Helps for reading code. Look below for more
        cin >> functionName;
        makelower(functionName);

        if(functionName == "add")
            add();
        else if(functionName == "subtract")
            subtract();
        else
            cout << "\nSorry, \"" << functionName << "\" is not a viable option! :-(\n";

        keepGoing = wantToContinue();
    }

    return 0;
}

// Infoscreen at beginning of program and after each iteration
void showInfo(const string& functionName)
{
    if (!functionName.empty())
        cout << "Your most recent function was: "<< functionName << "\n\n";
    else
        cout << "Welcome to the calculator!\n\n";
    cout << "Would you like to add or subtract?\n\n";
}

// Does the user have any intention of continuing?
bool wantToContinue()
{
    while(true)
    {
        string response;
        cout << "would you like to continue? ";
        cin >> response;
        makelower(response);
        if((response == "yes") || (response == "no"))
            return (response == "yes");
        cout << "\"" << response << "\" is an invalid response - please enter \"yes\" or \"no\".\n";
    }
}

// Adding procedure
void add()
{
    double x = 0.0;
    double y = 0.0;
    cout << "\nEnter the first of the two numbers you want to add\n\n";
    cin >> x;
    cout << "\nEnter the second of the two numbers you want to add\n\n";
    cin >> y;
    double answer = x + y;
    cout << "\n\n" << x << " + " << y << " is " << answer << "\n";
}


// Substracting procedure
void subtract()
{
    double x = 0.0;
    double y = 0.0;
    cout << "\n\nEnter the number you want to be subtracted from\n\n";
    cin >> x;
    cout << "\n\nEnter the amount you want to be subtracted\n\n";
    cin >> y;
    double answer = x - y;
    cout << "\n\n" << x << " - " << y << " is " << answer << "\n";
}


AND

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
 
// 1. declare variable in as tight a scope as possible
//    (here the x, y, and answer used to add does not need
//     to be the same as those used to subtract)
// 2. double is the preferred floating point type
// 3. endl is overkill for console output (is same as << '\n' << flush)
// 4. with makelower as suggested
//
// There are prob too many "\n"s now?

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

inline void makelower(string& str)
{
    const int len = str.length(); // so don't call it multiple times in loop
    for(int i = 0; i < len; ++i) // prefer pre-increment
        str[i] = static_cast<char>(tolower(str[i]));
}

int main()
{
    string mostRecentFunction;
    bool keepGoing = true;

    while(keepGoing)
    {
        cout << "Welcome to the calculator! your most recent function was:"
             << mostRecentFunction << "\n\n";
        cout << "Would you like to add or subtract?\n\n";
        cin >> mostRecentFunction;
        makelower(mostRecentFunction);

        if(mostRecentFunction == "subtract")
        {
            double x = 0.0;
            double y = 0.0;
            cout<<"\n\nEnter the number you want to have subtracted\n\n";
            cin >> x;
            cout << "\n\nEnter the amount you want subtracted\n\n";
            cin >> y;

            double answer = x - y;
            cout <<"\n\n" << x << " - " << y << " is " << answer << "\n";
        }
        else if(mostRecentFunction == "add")
        {
            double x = 0.0;
            double y = 0.0;
            cout << "\nEnter the first of the two numbers you want to add\n\n";
            cin >> x;
            cout << "\nEnter the second of the two numbers you want to add\n\n";
            cin >> y;

            double answer = x + y;
            cout << "\n\n" << x << " + " << y << " is " << answer << "\n";
        }
        else
        {
            cout << "\nSorry, this is not a viable option! :-(\n";
        }

        string continuing;
        cout << "would you like to continue?\n";
        cin >> continuing;
        makelower(continuing);
        keepGoing = (continuing == "yes");
    }
    // end while(keepGoing)

    return 0;
}
Last edited on
eraggo (181)
1. std::strings default constructor inits the contained string to "" so you should not use e.g. string continuing="";

string continuing; shows you understand how classes behave!

2. string parameters should be passed to functions by const ref.

3. main should be pretty much the first function defined. It's the start of the app and makes sense for it to be defined first so you don't have to scroll down to find out how the program begins (exceptions being miniscule inline functions.)

4. double is the preferred floating point type unless you have good reason to do otherwise.

5. converting input strings to lowercase as suggested earlier in thread simplifies things.

Also corrected the spelling of subtract


Seems i have to do more FTFM soon.
I had idea to introduce functions before declaring them but you get the point :)
megatron 0 (471)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
 
// Adding procedure
void add()
{
    double x = 0.0;
    double y = 0.0;
    cout << "\nEnter the first of the two numbers you want to add\n\n";
    cin >> x;
    cout << "\nEnter the second of the two numbers you want to add\n\n";
    cin >> y;
    double answer = x + y;
    cout << "\n\n" << x << " + " << y << " is " << answer << "\n";
}


// Substracting procedure
void subtract()
{
    double x = 0.0;
    double y = 0.0;
    cout << "\n\nEnter the number you want to be subtracted from\n\n";
    cin >> x;
    cout << "\n\nEnter the amount you want to be subtracted\n\n";
    cin >> y;
    double answer = x - y;
    cout << "\n\n" << x << " - " << y << " is " << answer << "\n";
}


Separating interface from implementation?

Add and Subtract should be very simple inline non void returning function?

EDIT:

Maybe a little out of scope here, but do you see how add and subtract work very similar? The only difference being the operation?
Last edited on
eraggo (181)
Dou you guys remember OP has very basic understanding here?
Last edited on
andywestken (4087)
Yes - which is why held off taking the step megatron 0 is alluding to, as it would be a bit more than appropriate here. Esp. as it would cause quite a bit of ripple in the code.

My second version (based on the orig code) was all I had planned; but I felt your version raised a few issues which needed to be clarified.

Andy
megatron 0 (471)
Dou you guys remember OP has very basic understanding here?


Non void, inline functions are basic knowledge?

The only difference is it returns a value and the inline basically for the OP's sake "replaces the function call with the answer"*.

* This doesn't actually happen. but as a beginner you wouldn't notice a difference.

EDIT:

By replace with the answer, I mean like an #include it will copy and paste the code within the function where you made the function call.
Last edited on
closed account (48T7M4Gy)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
 
#include <iostream>
#include <string>
using namespace std;

int main()
{
    string mostRecentFunction = "+";
    float answer = 0, x = 0, y = 0;
    
    while( mostRecentFunction != "q" )
    {
        cout << "Welcome to the calculator! Your most recent function was: "<< mostRecentFunction << endl;
        cout<<"Enter + or - to continue, q to quit.\n";
        cin >> mostRecentFunction;
    
    
        if(mostRecentFunction != "q")
        {
            cout<<"Insert the first number\n";
            cin >> x;

            cout << "Insert the second number\n";
            cin >> y;
        
            if(mostRecentFunction == "-" )
                answer = x - y;
            else
                answer = x + y;
    
        cout << x << mostRecentFunction << y << " is " << answer <<endl;
        }
    }
    
    cout << "Please call again :)";
    return 0;
}
Last edited on
Topic archived. No new replies allowed.