16
\$\begingroup\$

I want to adopt good coding habits now. the below code was written under tight constraints. We don't do assignment post mortems in class to discuss "real world" best practices -- we just get letter grades with little or no feedback. What are some examples of good questions I can ask myself to determine when code that works for an assignment would be re-written in real world applications?

(I'm also very interested in memory management, which we haven't covered in class. Any memory management tips re: the below code would be super-appreciated!)

Note: this is a finished assignment; I'm not looking for help to complete it, just help to develop good habits as early as possible.

Assignment:

  • simulate 5,500 craps games and print out the results.
  • other than main(), use only one function -- dice_roll(). All calculations are inline.
  • the assignment covers, enums, output precision and rand() and srand() functions.
  • use enum to define WIN, LOSE, and CONTINUE to give the results for each roll.
  • use const data sizes: int SIZE and int ROLLS.

Please feel free to provide any comments/questions as you see fit.

#include <iostream>
#include <iomanip>
#include <cstdlib>
#include <ctime>

// this is what will hold our data during the games
struct nodeType
{
  int num_rolls;
  int win;
  int loss;
  nodeType* link;
};

// these pointers will be used to traverse and perform operations
// on the nodes as we figure out what to do with them
nodeType *first, *last, *current, *trailCurrent, *newNode, *temp;

int roll_dice();


int main()
{
  enum gameOutcome { CONTINUE, WIN, LOSE };
  gameOutcome game_result;
  const int MAX_GAMES(5500);
  int sum, point, roll;
  first = NULL; // start of our list
  last = NULL; // end of our list

  srand(time(0)); // give rand() new seed

  for (int i = 1; i <= MAX_GAMES; ++i) // this for loop simulates all games
  {
    sum = roll_dice();

    newNode = new nodeType; // create new node for this game
    newNode->link = NULL; // make sure it doesn't point to anything

    switch (sum) // first roll test
    {
      case 7:
        game_result = WIN;
        roll = 1;
      case 11:
        game_result = WIN;
        roll = 1;
        break;
      case 2:
        game_result = LOSE;
        roll = 1;
      case 3:
        game_result = LOSE;
        roll = 1;
      case 12:
        game_result = LOSE;
        roll = 1;
        break;
      default:
        game_result = CONTINUE;
        point = sum;
        roll = 1;
        break;
    } // end of switch

    while (game_result == CONTINUE) // if no win/lose after 1st roll
    {
      sum = roll_dice();
      ++roll;

      if (sum == point)
        game_result = WIN;
      else if (sum == 7)
        game_result = LOSE;
    }

    // these assignments prepare our node fields to accept the
    // game outcome data
    newNode->num_rolls = roll;
    newNode->win = 0;
    newNode->loss = 0;

    if(game_result == WIN)
      newNode->win = 1; // adds one win for unique # of rolls

    else
      newNode->loss = 1; // adds one loss for unique # of rolls

    if(first == NULL) // if empty list creates list on first roll
    {
      first = newNode;
      last = newNode;
    }

    else
    {
      current = first;  // starts search at beginning of list
      int found(0);     // use for list elem search

      while (current != NULL && found < 1) // search to insert ascending order
      {
        if (current->num_rolls == roll) // has a game w/ this number of rolls
        {                               // been played?
          if (game_result == WIN)
            current->win += 1;          // if so add one win to that "row"
          else
            current->loss += 1;         // if so add one loss to that "row"
          found = 1;
        }

        else if (current->num_rolls > roll) // if game's #rolls is < than some
          found = 2;                        // #rolls in the list

        else
        {
          trailCurrent = current;
          current = current->link; // advances the search one node
        }
      } // end of while

      if (found == 1)
        delete[] newNode; // if #rolls for complete game already exists,
                          // delete this node. this is like "deduping" a
                          // database to first normal form

      else if (current == first) // inserts node at beginning of list
      {
        newNode->link = first;
        first = newNode;
      }
      else
      {
        trailCurrent->link = newNode; // inserts node in middle of list
        newNode->link = current;

        if (current == NULL) // if it's the biggest #rolls so far
          last = newNode;    // insert node at end of list
      } // end of last else
    } // end of 1st else
  } // end of main for loop

  int sum_wins(0), sum_loss(0);
  current = first; // set to first node in list before iterating over it
  while (current != NULL) // print results and sum wins/losses
  {
    std::cout << std::setw(4) << current->win << " games won and "
              << std::setw(3) << current->loss << " games lost on roll "
               << current->num_rolls << std::endl;
    sum_wins += current->win; // summing wins for use below
    sum_loss += current->loss; // summing losses for use below
    current = current->link;
  }

  // calculate the odds based on game results
  std::cout << std::setiosflags(std::ios::fixed | std::ios::showpoint)
            << "\nodds of winning are " << sum_wins << " / "
            << sum_wins + sum_loss << " = " << std::setprecision(2)
            << 100.0 * sum_wins / (sum_wins + sum_loss) << "%." << std::endl;

  // calculate avg num rolls per game
  double avg_length(0);
  current = first;
  while (current != NULL)
  {
    avg_length += (current->win + current->loss) * current->num_rolls;
    current = current->link;
  }
  std::cout << "avg num rolls/game is " << std::setprecision(2)
            << avg_length / 5500.00 << " rolls." << std::endl;

  while (first != NULL) // destroy list
  {
    temp = first;
    first = first->link;
    delete[] temp;
  }

  last = NULL;

  std::cout << "press RET to exit";
  std::cin.get();

  return 0;
} // end of int main()


int roll_dice()
{
  return (rand() % 6) + (rand() % 6) + 2;
}
\$\endgroup\$
6
  • 6
    \$\begingroup\$ well i know the program constraints limited you to only having 1 method. Real world says that is a bad habbit to have. Even worse is they want you to use the main method... Ugh my skin is crawling. Having one large method always screams refactor. There is nothing wrong with having many methods that only have 3-4 lines in them. It makes it easier to find bugs, and easier to read. \$\endgroup\$ – Robert Snyder May 21 '13 at 1:42
  • 1
    \$\begingroup\$ second thing i see is inconsistent white spaces. What I mean by that is you want to be consistant with lines in between blocks of statements. Normally when you have to do that you would pull out that line block and make a method for it. It tells me that you naturally want to break it up into somethign that is easier to read. That is good. It's also general practice to not have a whitespace between your if and else statement. \$\endgroup\$ – Robert Snyder May 21 '13 at 1:44
  • 3
    \$\begingroup\$ To answer your question about What questions you should ask your self. The one I use the most is, Will I remember what this method does next month? Next is Can I make this easier to read? another Is my method doing more than 1 thing? \$\endgroup\$ – Robert Snyder May 21 '13 at 1:58
  • \$\begingroup\$ @RobertSnyder, thx for the feedback. those seem like some great questions to ask. also, what do you think about automated checkers, like this -- google-styleguide.googlecode.com/svn/trunk/cpplint \$\endgroup\$ – d3vin May 22 '13 at 23:06
  • \$\begingroup\$ @d3vin why would you want to purposefully make your code as broken as Google's? \$\endgroup\$ – Cubbi Dec 25 '13 at 3:40
22
\$\begingroup\$

There are a number of things you want to ask yourself. The first is, "in the real world, do these requirements make sense?". In this case, the answer is an emphatic "No".

other than main(), use only one function -- dice_roll(). all calculations are inline.

This is a terrible requirement, and whoever wrote this assignment needs to be whacked with the common sense bat. It is teaching really bad practice right from the get go. It is never desirable to have a giant main function.

The second question to ask is, "Am I using C++ or am I using C with std::cout instead of printf?". Likely this is not your fault - any teacher who decides to make a student bundle all their code into the main function is doing a dubious job at best.

Let's tackle both of these. If you want to learn, I'm going to say throw away some of these requirements. The real requirement of the program is:

simulate 5,500 craps games and print out the results.

Here, the requirement itself leads to a relatively easy breakup of the program structure. We'll keep it simple, and say we want 4 functions in total:

  • A function to simulate dice rolls - roll_dice() (which already exists)
  • A function to simulate the games and store the results in some kind of data structure - let's call it run_simulation. This will take as a parameter the number of games to play.
  • A function to calculate and print statistics based on games. This will take the data structure from the previous function.
  • main, which will co-ordinate the above functions.

One of the nice things about C++ is the rich set of data structures available in the standard library. These generally perform memory management for you. It isn't listed in the requirements, but from your code, you want something that will store the number of wins/losses for a given number of rolls in sorted order.

Again, I'm not sure how much experience you have with data structures, however, the standard library has a good fit for this requirement: a std::map. A map stores key/value pairs in an efficient way, so that key lookup is fast.

Ok, so we'll start as before: defining our enum and something to store our game data - call it (unimaginatively) game_data:

#include <iostream>
#include <iomanip>
#include <cstdlib>
#include <ctime>
#include <map>    // <--- Additional include so we can use `std::map`

enum game_outcome { CONTINUE, WIN, LOSE };

struct game_data
{
    int win;
    int loss;

    game_data()
      : win(0),
        loss(0)
    { } 

    game_data(int w, int l)
      : win(w),
        loss(l)
    { }
};

Above, we've defined some constructors for our game_data type. Now, we'll define our map type:

typedef std::map<int, game_data> result_list;

This says the type result_list is a map from integers to keys. Here, the int key will be the number of rolls. A nice thing about map is that it stores things in sorted order - so our sorting comes for free.

int roll_dice()
{
    return (rand() % 6) + (rand() % 6) + 2;
}

result_list run_simulation(int num_games)
{
    result_list results;
    static game_outcome outcome[] = {CONTINUE, CONTINUE, LOSE, LOSE, CONTINUE, CONTINUE, 
                                     CONTINUE, WIN, CONTINUE, CONTINUE, CONTINUE, WIN,
                                     LOSE};

    for(int i = 0; i < num_games; ++i) {

        int roll = 1;
        int sum = roll_dice();
        game_outcome out = outcome[sum];

        while (out == CONTINUE) {
            int point = sum;
            sum = roll_dice();
            ++roll;
            if(sum == point) {
                out = WIN;
            }
            else if(sum == 7) {
                out = LOSE;
            }
        }
        game_data& d = results[roll];
        (out == WIN) ? ++d.win : ++d.loss;
    }    

    return results;
}

Firstly, we've replaced the switch statement with an array lookup, which makes the code a bit more concise and arguably less error prone (it's easy to forget break at the end of switch statements). The while loop is mostly unchanged, other than some reordering. What has mostly changed is the replacement of a lot of code with the two lines:

game_data& d = results[roll];
(out == WIN) ? ++d.win : ++d.loss;

Lookup in a map can be done using [] syntax (much like arrays). If the key doesn't exist in this case, it is inserted. Thus, we lookup the data by rolls. For example, if rolls was 3, this would find the game_data class that sits in the map with the previous results for 3. Then, we simply increment the correct win or loss based on the outcome.

Now, we need to write a function that will take what is in the map and print out some information from what is in it: let's call it result_statistics.

void result_statistics(const result_list& results, int num_games)
{
    int wins = 0;
    int losses = 0;
    double avg_length = 0;

    for(auto const & p : results) {
        std::cout << std::setw(4) << p.second.win << " games won and " 
                  << std::setw(3) << p.second.loss << " games lost on roll "
                  << p.first << "\n";
        wins += p.second.win;
        losses += p.second.loss;
        avg_length += (p.second.win + p.second.loss) * p.first;
    }
    int total_games = wins + losses;
    std::cout << std::setiosflags(std::ios::fixed | std::ios::showpoint) 
              << "\nodds of winning are " << wins << " / "
              << total_games << " = " << std::setprecision(2)
              << 100.0 * wins / total_games << "%." << "\n";
    std::cout << "avg num rolls/game is " << std::setprecision(2)
              << avg_length / num_games << " rolls." << "\n";

}

The print code is mostly the same.

Finally, our new main function:

int main()
{
    const int kGames = 5500;
    srand(time(nullptr));
    result_list r = run_simulation(kGames);
    result_statistics(r, kGames);
}

Note how we can now follow (at least at a high level) the flow of the code. We define the total number of games, seed our random generator, run the simulations, and calculate the statistics from those runs. Breaking things up in this way makes things much easier to reason about and follow in the code.

Even if some (or most) of this doesn't make sense to you yet, I think the main things to take away from this are that if you get told to shove everything into a main function, do it for your assignment if that is what is required. Afterwards, go back and split it up into smaller functions, each one having a specific, well defined, singular purpose. Compare the code from the two. Which one is easier to understand? Investigate the standard library, teach yourself as much as you can about it (basic containers are probably the most important thing for now). Finally, look at the assignment specs. Do they all make sense? Do they lead to more or less readable code? What would you do differently if you had free-reign?

\$\endgroup\$
3
  • \$\begingroup\$ this is fantastic. thx for the feedback. much of what you included is new to me -- i.e. initializer lists in structures, maps, recursion -- and i'd like the opportunity to review and perhaps post some questions as comments. is that cool w/ you? \$\endgroup\$ – d3vin May 22 '13 at 23:01
  • \$\begingroup\$ also, i'm wondering if in your enum it would make sense to do this -- static game_outcome outcome[] = {LOSE = 2, LOSE, CONTINUE, ... } -- or if that just adds unnecessary complexity. \$\endgroup\$ – d3vin May 22 '13 at 23:04
  • 1
    \$\begingroup\$ @d3vin Sure, ask away. Unfortunately, you can't do the above with arrays like you can with an enum - it won't compile. \$\endgroup\$ – Yuushi May 23 '13 at 1:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.