How can I improve this code?

Hello.

Please consider the following code:

#include <iostream>

using namespace std;

int main(int argc, char** argv) {
enum object {rock, paper, scissors};
object player1, player2;
string player1Input, player2Input;
cout <<"Enter input";
cin >> player1Input >> player2Input;

object getChoice(string input){
if (input == "rock") return object::rock;
else if (input == "paper") return object::paper;
else if (input == "scisor") return object::scissors;
}
object player1Choice = getChoice(player1Input);
object player2Choice = getChoice(player2Input);


switch(player1Choice){
case object::rock: cout << ""; break;
case object::paper: cout << ""; break;
case object::scissors: cout << ""; break;
}

switch(player2Choice){
case object::rock: cout << ""; break;
case object::paper: cout << ""; break;
case object::scissors: cout << ""; break;
}
}

When compiling I get the errors "Function 'getChoice' is used but not defined in this translation unit, and cannot be defined in any other translation unit because its type does not have linkage" and "Function definition is not allowed here" for line 12 (object getChoice(string input){) and "Use of undeclared identifier 'getChoice'" for lines 17 and 18 that is (object player1Choice = getChoice(player1Input);) and (object player2Choice = getChoice(player2Input);)

Please help me remove the errors.
"Function definition is not allowed here" for line 12

You cannot define a function inside the body of an another function. You have to do it outside. For example:
1
2
3
4
5
6
7
object getChoice(string input){
  // code
}

int main(int argc, char** argv) {
  // code
}


Your function getChoice returns a value. What value does it return, if input is "Spock"?
Since getChoice() must be outside of main() and it returns an object, the definition of object must be outside of main() too.

getChoice() is a bad name for this function. I'd call it stringToObject() because it converts a string to an object. You will probably find that you also need an objectToString() function.

Your switch statements are nearly identical. That code should be in a function.

This version of your code compiles.
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
#include <iostream>
#include <string>

using std::string;
using std::cin;
using std::cout;

enum object { rock, paper, scissors };

object
getChoice(string input)
{
    if (input == "rock")
	return object::rock;
    else if (input == "paper")
	return object::paper;
    else if (input == "scisor")
	return object::scissors;
}

int
main(int argc, char **argv)
{
    object player1, player2;
    string player1Input, player2Input;
    cout << "Enter input";
    cin >> player1Input >> player2Input;

    object player1Choice = getChoice(player1Input);
    object player2Choice = getChoice(player2Input);

    switch (player1Choice) {
    case object::rock:
	cout << "";
	break;
    case object::paper:
	cout << "";
	break;
    case object::scissors:
	cout << "";
	break;
    }

    switch (player2Choice) {
    case object::rock:
	cout << "";
	break;
    case object::paper:
	cout << "";
	break;
    case object::scissors:
	cout << "";
	break;
    }
}

Last edited on
@keskiverto and @dhayden Thank you very much for responding.

@keskiverto I now get the error Control may reach end of non-void function for return object::scissors;}

Could you explain what the error means and how I can remove it?
Last edited on
He already addressed that:
Your function getChoice returns a value. What value does it return, if input is "Spock"?
@dhayden You mean I need to add an else statement for any other input? I know it give an error to add:

else cout >>"invalid input";

How can I add the else statement for other unrelated inputs?
Last edited on
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
#include <iostream>
#include <string>
#include <cctype>

enum object { rock, paper, scissors };
const std::string object_names[] = { "rock", "paper", "scissors" } ;

std::string to_lower( const std::string& str )
{
    std::string lcase_str ;
    for( unsigned char c : str ) lcase_str += std::tolower(c) ;
    return lcase_str ;
}

// return a valid choice; try again on invalid input
object get_object() 
{
    std::cout << "choose object: one of [ " ;
    for( const std::string& name : object_names ) std::cout << name << ' ' ;
    std::cout << "]: " ;

    std::string choice ;
    std::cin >> choice ;
    choice = to_lower(choice) ; // convert to all lower case

    if( choice == object_names[0] ) return rock ;
    else if( choice == object_names[1] ) return paper ;
    else if( choice == object_names[2] ) return scissors ;

    // else
    std::cout << "invalid choice '" << choice << "' : try again\n" ;
    return get_object() ; // try again
}

int main()
{
    const object item = get_object() ;
    std::cout << "you chose the object '" << object_names[item] << "'\n" ;
}
@JLBorges Thanks a lot. I'll try to explain it to myself. But wasn't it possible to add an else branch to the object getChoice?
If you add e.g. invalid to your enum you can simply react to that:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
enum object {invalid, rock, paper, scissors};

object getChoice(string input){
if (input == "rock") return object::rock;
else if (input == "paper") return object::paper;
else if (input == "scisor") return object::scissors;

return object::invalid;
}

...

switch(player1Choice){
case object::rock: cout << ""; break;
case object::paper: cout << ""; break;
case object::scissors: cout << ""; break;
default: cout << "Invalid choice"; break;
}

...
@coder777 Thanks. I will try it to see the result.
Can someone explain line 19 especially the role of & name in the for statement in the code written above by JLBorges.
Last edited on
1
2
for ( const std::string& name : object_names )
  std::cout << name << ' ' ;

Uses range-syntax that was added for the for-loop in C++11.

It is (here) equivalent to:
1
2
3
4
for ( int w=0; w < 3; ++w ) {
  const std::string& name = object_names[w];
  std::cout << name << ' ' ;
}


The & in declaration of 'name' tells that 'name' is a reference rather than independent variable.
Reference is an alias -- additional name that can be used to refer to existing value.
@keskiverto Thanks. I understand it now.
Topic archived. No new replies allowed.