Craps game rules and code: version 2

Question

I've taken all the advice from here and have created a Craps class and gameplay. I just wanted to ask for more advice on how to optimize my code.

#include <iostream>
#include <cmath> // atan(), fabs()
#include "Craps.h"
using namespace std;
int main(int argc, char* argv[])
{
 int choice;
 Craps game;
 do {
 cin>> choice;
 if(!cin)
 {
 cin.clear();
 cin.ignore(200,'\n');
 cout<<"\n please inter int number \n\n";
 }
 switch (choice) {
 case 1:
 game.Play();
 break;
 case 2:
 break;
 default:
 break;
 }
 } while (choice != 2);
 return 0;
}
#include <iostream>
//#include <catdlib>
#include <ctime>
#include <string>
using namespace std;
class Craps
{
public:
 void DisplayInstruction();
 void Play();
 int DiceRoll();
private:
 int random;
};
void Craps::DisplayInstruction(){
 cout<<"DisplayInstruction"<<endl;
}
int Craps::DiceRoll()
{
 int die1 = (rand() + time(0)) % 6+1 ;
 int die2 = (rand() + time(0)) % 6+1 ;
 int sum = die1 + die2;
 return sum;
}
void Craps::Play()
{
 bool won = false;
 bool lost = false;
 string anyKey;
 int myPoint = 0;
 int sumeOfDice = DiceRoll();
 switch (sumeOfDice)
 {
 case 7:
 won= true;
 break;
 case 11:
 won= true;
 break;
 case 2:
 lost = true;
 break;
 case 3:
 lost = true;
 break;
 case 12:
 lost = true;
 break;
 default:
 myPoint = sumeOfDice;
 cout<< myPoint <<endl;
 break;
 }
 while (won == false && lost ==false) {
 sumeOfDice = DiceRoll();
 if(sumeOfDice == myPoint)
 {
 won = true;
 }
 else
 if(sumeOfDice == 7)
 {
 lost = true;
 }
 }
 if (won == true)
 {
 cout<<"player win"<<endl;
 }
 else
 {
 cout<<"player lose "<<endl;
 }
}
#endif /* defined(__Craps__Craps__) */

Answer 1

• Try to refrain from using using namespace std, especially in a program like this. If you were to put it in the header, you could introduce bugs due to issues such as name-clashing.

• Your comments next to <cmath> suggests that you're using it, but I don't see these functions used anywhere. No need to include this library.

• #include headers before libraries to prevent unwanted dependency:

  #include "Craps.h" // header won't be forced to use <iostream>
  #include <iostream>
  

• Naming convention states that functions should start lowercase:

  thisIsAFunction();
  this_is_also_a_function();
  

• DisplayInstruction() is a needless function as it only displays one line of text. Functions shouldn't be created just for that. In general, functions that don't do anything with the data members are best as free functions (non-member functions).

• The dice functionality should have its own class, allowing the Craps class to maintain object(s) of it as needed. This will also ensure separation of concerns. You may refer to my implementation to get an idea for this.

• You didn't seed std::rand() with std::srand(). This will cause rand() to produce the same random numbers each time. Put this at the top of main() only:

  // this requires <cstdlib> and <ctime>
  // use nullptr instead if using C++11
  std::srand(static_cast<unsigned int>(std::time(NULL)));
  

  However, if you're using C++11, you should no longer use rand. I'll quote one of my answers:

  You should refrain from using rand altogether in C++11 as it is considered harmful. Instead, utilize C++11's <random> library.

  You also no longer need <ctime> and should use C++11's <chrono>. From this library, you can obtain a seed for a PRNG with this:

  auto chrono_seed = std::chrono::system_clock::now().time_since_epoch().count();
  

• The switch in Play() could be its own function, specifically of type bool. As it isn't working with a data member, it can be a free function. If you go this route, then each case will need a return true or return false. The default will also need to handle errors resulting from an invalid case value, such as by throwing an exception.

• You never use anyKey anywhere, so remove it.

• Tested conditions could be shortened like this:

  if (condition) // if (condition == true)
  if (!condition) // if (condition == false)
  

• Tested streams can be shortened like this:

  if (!(cin >> choice)) // cin >> choice;
   // if (!cin)