Critique 2hr noob code

Hello. I've only been learning C++ (my first language) for 2 hours now. So far, this is what I can/can't do.

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
  #include <iostream>
using namespace std;

double y;
double x;
int z;

int performmath()
{
	cout << " Which mathematical operations would you like me to perform?" << endl;
	cout << " Press 1 for addition." << endl;
	cout << " Press 2 for subtraction" << endl;
	cout << " Press 3 for multiplication" << endl;
	cout << " Press 4 for division" << endl;
	cin >> z; 
	cout << endl << "You chose option " << z << ".";
	return z;
}

int main()
{
	cout << "Hello Scott.  Could you please give me a number? " << endl;
	cin >> y;
	cout << "  Scott, you entered " << y << ". I would like to try some simple math.  Could you give me one more number please?" << endl;
	cin >> x;
	cout << "  Scott, you entered " << x << ". Now what would you like me to do with these two numbers?" << endl;
	performmath();

	if (z == 1)
	{
		cout << "The sum of " << y << " and " << x << " is " << y + x << "." << endl;
	}

	if (z == 2)
	{
		cout << " " << y << " minus " <<  x << " is " << y - x << "." << endl;
	}

	if (z == 3)
	{
		cout << " " << y << " multiplied by " << x << " is " << y * x << "." << endl;
	}

	if (z == 4)
	{
		cout << " " << y << " divided by " << x << " is " << y/x << "." << endl;
	}
	return 0;
}


I'm posting this because I would like some feedback. Please take into consideration that I'm as new as someone can be at programming. I woke up a few hours ago and said, "I wan't to learn to program".

Here's what I do know:

This has bugs. When prompted for options 1-4 for a mathematical operation, I'm not limited to just 1-4. I'm aware of the issue, but in my 2 hours of learning, I haven't yet found a way to plug that hole. Again, I'm fresh, and just haven't made it to a tutorial that covers this yet.

What I don't know:

I don't know if I'm being efficient. I don't know if this is written properly. Is there a better way to write this? Better yet, is this where I should be at? I'm sure many of you can relate to wanting to be some "master programmer" on day one. I want to too, but being a realist, I know I have to crawl first. I just want to make sure I'm crawling efficiently and not picking up any bad habits.

This is VERY good code for someone who just started. Even better, you're asking the right questions.

Learn about local variables. These are variables that are declared inside a block (somewhere inside a { and } pair). All of your variables could be local.

As for efficiency, your if statements could be replaced with a switch statement in this case:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
switch (z) {
case 1:
	cout << "The sum of " << y << " and " << x << " is " << y + x << "." << endl;
	break;
case 2:
	cout << " " << y << " minus " <<  x << " is " << y - x << "." << endl;
	break;
case 3:
	cout << " " << y << " multiplied by " << x << " is " << y * x << "." << endl;
	break;
case 4:
	cout << " " << y << " divided by " << x << " is " << y/x << "." << endl;
	break;
}

The basic format of a switch statement is:
1
2
3
4
5
6
7
8
9
switch(expression) {
case value1:
	statements;
case value2:
	statements;
...
default:
	statements;
}

It evaluates the expression once. Then if checks the cases and jumps to the statements corresponding to the value of the expression. If it doesn't match any case value, then it executes the statements for the "default" case. Some things to note:
- the expression must evaluate to an integral type (an integer, character, etc)
- the values must be constant expressions, which means they must evaluate to a constant at compile time. Put another way, you should be able reduce it to a constant just by reading the code.
- The default case is optional. If it's missing then none of the code executes.
- MOST IMPORTANT OF ALL: execution of the statements will "fall through" from one case to the next unless you do something to prevent it. The easiest thing to do is add a break statement which will cause execution to jump to the first statement after the whole switch structure.


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
#include <iostream>
// using namespace std;

// double y;
// double x;
// int choice;

int get_choice() // performmath
{
    std::cout << "\n Which mathematical operations would you like me to perform?" << '\n'
              << " Press 1 for addition." << '\n'
              << " Press 2 for subtraction" << '\n'
              << " Press 3 for multiplication" << '\n'
              << " Press 4 for division" << "\n\n";

    int choice ; // use semantically rich variable names
    std::cin >> choice ;

    if( choice > 0 && choice < 5 )
    {
        std::cout << '\n' << "You chose option " << choice << ".\n";
        return choice;
    }

    else
    {
        std::cout << "invalid input " << choice << ". valid choices are 1, 2, 3 or 4\n" ;
        return get_choice() ; // get a new input ( by calling get_choice() again)
    }
}

int main()
{
    std::cout << "Hello Scott.  Could you please give me a number? " << '\n';
    double y ;
    std::cin >> y;

    std::cout << "Scott, you entered " << y << ".\nI would like to try some simple math.\n"
                 "Could you give me one more number please?" << '\n';
    double x ;
    std::cin >> x;

    std::cout << "Scott, you entered " << x << ".\nNow what would you like me to do with these two numbers?" << '\n';

    const int choice = get_choice();

    if (choice == 1)
    {
        std::cout << "The sum of " << y << " and " << x << " is " << y + x << '.' << '\n';
    }

    if (choice == 2)
    {
        std::cout << " " << y << " minus " <<  x << " is " << y - x << '.' << '\n';
    }

    if (choice == 3)
    {
        std::cout << " " << y << " multiplied by " << x << " is " << y * x << '.' << '\n';
    }

    if (choice == 4)
    {
        if( x == 0 ) std::cout << "you can't divide by zero.\n" ;
        else std::cout << " " << y << " divided by " << x << " is " << y/x << '.' << '\n';
    }

    // return 0; // there is an implicit return 0 ; at the end of main
}
Last edited on
Thanks for the response. Using a switch statement does look cleaner and more efficient. I'm sure I'll be reading (or watching) example of these today. And thank you for the basic format - it is jotted down in my notebook.

I appreciate the tips.
Thanks JL.

I didn't quite understand how to make local variables. Your example helped a lot. Also, thanks for showing how to restrict the inputs for choice. I see you ended your statements with \n vs endl. I don't know the difference (if there is one) yet, but I'll get there. Again, thanks for your $.02.
'\n' - escape sequence for the new line character.
The new-line character \n has special meaning when used in text mode I/O: it is converted to the OS-specific newline byte or byte sequence.
http://en.cppreference.com/w/cpp/language/escape


std::endl Inserts a newline character into the output sequence os and flushes it.
http://en.cppreference.com/w/cpp/io/manip/endl
For cin/cout (and equivalent) interaction, there is no reason to flush; that's done automatically.
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-sl50-avoid-endl
You guys are awesome.
Topic archived. No new replies allowed.