Random number game

Question

I have created a small 'game' which basically asks the user for the row number and column number and then picks a random number and if that number is above 11, out of 15, the user wins. When the user wins that position in the array is replaced with a X or O to indicate a win or loss.

The program works fine. As it is my first real program, I know that there is room for improvement.

I'm looking for constructive feedback on program efficiency, possible performance improvements and just better ways to code a similar program (at a basic level).

#include <stdafx.h>
#include <iostream>
#include <string>
#include <stdlib.h>
#include <ctime>
using namespace std;
void fill(char Array[10][10]);
 int xRan;
 int choicei = 12;
 int choicej = 12;
int main()
{
 char Array[10][10];
 srand(time(0));
 xRan = rand() % 15 + 1;
 while (choicei > 10 || choicej > 10)
 {
 cout << "Enter The Row Number Less Than 10!" << endl;
 cin >> choicei;
 cout << endl;
 cout << "Enter The Column Number Less Than 10!" << endl;
 cin >> choicej;
 cout << endl;
 }
 fill(Array);
 for (int i = 0; i < 10; i++)
 {
 for (int j = 0; j < 10; j++)
 {
 cout << Array[i][j] << " ";
 }
 cout << endl;
 } 
 if (xRan > 11)
 {
 cout << "\nCongratulations! You Won!\n" << endl;
 }
 else
 {
 cout << "\nBetter Luck Next Time!\n" << endl;
 }
}
void fill(char Array[10][10])
{
 for (int i = 0; i < 10; i++)
 {
 for (int j = 0; j < 10; j++)
 {
 Array[i][j] = '*';
 }
 }
 if (xRan > 11)
 {
 for (int i = 0; i < 1; i++)
 {
 for (int j = 0; j < 10; j++)
 {
 Array[choicei - 1][choicej - 1] = 'X';
 }
 }
 }
 else
 {
 for (int i = 0; i < 1; i++)
 {
 for (int j = 0; j < 10; j++)
 {
 Array[choicei - 1][choicej - 1] = 'O';
 } 
 }
 }
}

Answer 1

• Try not to use using namespace std.

• <stdlib.h> is a C library. Use <cstdlib> for C++.

• These shouldn't be global:

  int xRan;
  int choicei = 12;
  int choicej = 12;
  

  As you're using xRan in another function, initialize it in main() only:

  int xRan = rand() % 15 + 1;
  

  and pass it to that function as an argument.

  I also don't see why choicei and choicej are initialized to 12. If there's a reason for that, there should be a comment specifying that. Since it looks needless, just declare them in main() closely in scope with the input:

  cout << "Enter The Row Number Less Than 10!" << endl;
  int choicei; // declare here
  cin >> choicei;
  

  cout << "Enter The Column Number Less Than 10!" << endl;
  int choicej; // declare here
  cin >> choicej;
  

  On another note, choicei and choicej are not good names. What are i and j? One would assume they have to do with simple loop counters, but this doesn't seem to be the case here. Consider something like rowNumberChoice and colNumberChoice.

• I don't know how high your compiler warnings are turned up, but your std::srand() may produce warnings corresponding to "possible loss of data." This is usually remedied by casting the std::time() return to an unsigned int.

  If these warnings become present, call std::srand() like this:

  // if you're using C++11, use nullptr instead of NULL
  std::srand(static_cast<unsigned int>(std::time(NULL));
  

• In C++, prefer to use standard containers over C-style arrays. You especially shouldn't pass one to a function as it would decay into a pointer. It's best to avoid this in C++.

  I recommend using std::array, if you're using C++11. As 2D, it would be initialized like this:

  std::array<std::array<char>, 10>, 10> gameBoard;
  

  As @nvuono mentioned, you could have row and column constants so that minimal updating is needed. You could then have this:

  const unsigned int rows = 10;
  const unsigned int cols = 12;
  std::array<std::array<char>, rows>, cols> gameBoard;
  

  To pass this 2D array to functions:

  // you may use a typedef to "rename" this type for less typing
  // name should be capitalized as it is a type
  typedef std::array<std::array<char>, rows>, cols> Board;
  // pass as const-ref as the board shouldn't be modified
  void displayBoard(Board const& gameBoard) { }
  

• Danturr: I have made choicei/j equal 12 as the program runs a loop until the user enters a choice under 10. Thanks for the feedback.

Answer 2

Think about what happens when you want to change your dimensions from 10x10 to 12x14. How many locations in your code would require updating?

You can look into using constants for the dimensions and the .length property of your arrays to bound your for loops.

• Danturr: Yes that is a good point. I could let the user define the dimensions of the 'playing board'. Thanks for this.
• jliv902: The tag says C++. Raw arrays have no properties/members.

Answer 3

When reading and validating choicei and choicej, it would be better from the user's point of view to only re-supply the invalid entry, not both of them in case one of them is invalid. Code-wise, a do-while loop is more suitable since you need to run the loop at least once.

Something along this line would certainly be an improvement

do {
 read choicei
} while(choicei is invalid)
do {
 read choicej
} while(choicej is invalid)

I also think that you should account for the user entering <= 0, because indexing Array with a 0 (since you subtract 1 from the indices) or negative choicei/choicej will result in an out of bounds access.

• Loki Astari: Also check for bad input. If you user enters 'PLOP' the read will fail, the value of choice is not set correctly and your while loop infinitely.

Answer 4

In Fill you're wasting resources running more loops than necessary:

void fill(char Array[10][10])
{
 for (int i = 0; i < 10; i++)
 {
 for (int j = 0; j < 10; j++)
 {
 Array[i][j] = '*';
 }
 }
 if (xRan > 11)
 Array[choicei - 1][choicej - 1] = 'X';
 else
 Array[choicei - 1][choicej - 1] = 'O';
}

In this program it's probably not too big a deal, but I think it would make more sense to fill the array at the beginning and change the specific element when you get an answer. This way if you wanted to allow the user to play again, you can reset the array by changing the one element, instead of re doing the whole array.

Answer 5

1. Remove these the inclusion of <stdlib.h> and <string>.
   You never use them.

2. Use a consistent naming convention for variables.

   You have 3 different naming conventions being used:
   xRan
choicei
Array

   If you're working by yourself, pick one and stick to it. If you're working on a larger project, use the conventions already in use.

3. Avoid global variables. In general, polluting the global namespace is a bad practice.

4. Avoid magic numbers.
   I normally don't recommend using macros. But in this case, it wouldn't hurt. You could do something like this:

   #define MAX_ROWS 10 
   #define MAX_COLS 10
   #define MIN_WIN 11 // Random number must >= MIN_WIN 
   

5. Following @Erevis's advice, create a function to encapsulate the functionality of accepting user input.

   int GetValue (const int min, const int max, const char *szWhat = "row") ;
   // ...
   int GetValue (const int min, const int max, const char *szWhat)
   {
    int n = 0 ;
    do {
    std::cout << "Enter a " << szWhat << " number between " << min << " and " << max << "!" << std::endl ;
    std::cout << "> " ;
    std::cin >> n ;
    std::cout << std::endl ; 
    if (std::cin.good () == false) {
    std::cin.clear () ;
    std::cin.ignore (std::numeric_limits <std::streamsize>::max (), '\n') ;
    continue ;
    } 
    } while (n < min || n > max) ;
    return n ;
   }
   

6. Based on @Tinstaafl's answer, create a function for printing the outcome.

   void PrintOutcome (const int row, const int column, const bool has_won) ;
   // ...
   void PrintOutcome (const int row, const int column, const bool has_won)
   {
    char board [MAX_ROWS] [MAX_COLS] ;
    for (size_t i = 0; i < MAX_ROWS; ++i) {
    for (size_t j = 0; j < MAX_COLS; ++j) {
    board [i] [j] = '*' ;
    }
    }
    char c = (has_won == true) ? 'X' : 'O' ;
    board [row - 1] [column - 1] = c ;
    for (size_t i = 0; i < MAX_ROWS; ++i) {
    for (size_t j = 0; j < MAX_COLS; ++j) {
    std::cout << board [i] [j] << ' ' ;
    }
    std::cout << std::endl ;
    }
    std::cout << std::endl ;
    if (has_won == true) {
    std::cout << "Congratulations, you won!" << std::endl ;
    }
    else {
    std::cout << "Unfortunately, you lost!" << std::endl ;
    }
   }
   

7. Give the user a chance to continue playing the game.

   bool ContinuePlaying () ;
   /...
   bool ContinuePlaying ()
   {
    char c ;
    std::cout << "Do you wish to continue playing? <y/n>" << std::endl ;
    std::cout << "> " ;
    std::cin >> c ;
    return (c == 'y') ;
   }
   

8. Your main function can then be simplified into:

   int main (void)
   {
    srand (static_cast <unsigned> (time (NULL))) ;
    do {
    bool has_won = (rand () % 15 + 1) >= MIN_WIN ;
    int row = GetValue (1, MAX_ROWS) ;
    int column = GetValue (1, MAX_COLS, "column") ;
    PrintOutcome (row, column, has_won) ;
    } while (ContinuePlaying () == true) ;
    return 0 ;
   }
   

9. @Jamal mentioned not to pass an array an as argument to your function. I want to give you an example of why you should not do that. Suppose you have a function like this:

   void SomeFunc (int board [10] [10])
   {
    for (size_t i = 0; i < 10; ++i) {
    for (size_t j = 0; j < 10; ++j) {
    std::cout << board [i] [j] << " " ;
    }
    std::cout << std::endl ;
    }
   }
   

   From the compiler's point of view, this is the same as:

   void SomeFunc (int *board [10])
   

   So you could do something like this:

   int main (void)
   {
    int b1 [10] [10] ;
    int b2 [5] [10] ;
    // ... Initialize b1 and b2 to valid values.
    SomeFunc (b1) ;
    SomeFunc (b2) ; // error
    return 0 ;
   }
   

   This would compile, but cause undefined behavior and possibly crash your program (if you're lucky).

Comments
@LokiAstari

Unless I'm misunderstanding your fourth comment, here's a counter-example for it (compiled with VS2012):

void SomeFunc (int board [2] [3])
{
 auto s1 = sizeof (board [0]) / sizeof (board [0] [0]) ; // s1 = 3
}
int main (void)
{
 int b1 [2] [3] = 
 {
 {1, 2, 3},
 {4, 5, 6}
 } ;
 int b2 [3] [2] =
 {
 {1, 2},
 {3, 4},
 {5, 6}
 } ;
 SomeFunc (b1) ;
 SomeFunc (b2) ; // Does not compile
 return 0 ;
}

• Loki Astari: Rather than macros use const int. This way your constants are also typed.
• Loki Astari: In your read you should also check for bad input. Otherwise you will go into an infinite loop (you are assuming the user can enter bad values so you should really check for bad input).
• Loki Astari: From the compilers point of view its void SomeFunc (int** board) all arrays are collapsed to pointers across function boundaries. You should pass by reference to maintain validity of the array semantics. (ie sizeof() will not work as you expect based on function definition).
• Loki Astari: 3) Yes. But that's encapsulation for you. It forces you to be nice and tidy and keep functions that work together in the same pace.
• Loki Astari: 4) Now try: sizeof (board) / sizeof (board [0]). Do you expect all combinations to work or just the ones you happen to remember are exceptions to the rule. Using this technique will lead to you making a mistake. Pass by reference to avoid the possibility of it going wrong (or preferably pass a standard container by reference).

Answer 6

Also using the name 'Array' should be camelCase, but with small letter start, as it is a variable, and types should start with big letter CamelCase. So I'd suggest using 'array' instead. It will help you on bigger projects and other languages.