i want to improve my program -- what questions could/should i ask myself during the re-write?

[cpp beginner]

tl;dr - 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?

(also -- i'm very interested in memory management, which we haven't covered in class. any memory management tips 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();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.

ok, so with that being said, below is my code. please feel free to provide any comments/questions as you see fit.

thx in advance!

#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;
}

[andywestken]

I assume from the style that you haven't started on classes, etc., as the code is more or less C with C++ i/o. So I won't include things like it would be better if you used classes, C++ standard containers, etc.

On initial impressions, some of the good points are:

1. The code is well laid out and is easy enough to follow.

2. You're not using using std; so you're not poluting the global namespace

3. You're using enums and consts rather than literals, at least some of the time. (Though maybe you could have used the Craps names for the dice throws? 2 = snake_eyes, 7 = natural_or_seven_out ... But I'm not sure about this?)

4. You're defining variables at first point of use.

5. You're using const

6. You're using ++i rather than ++i (Edit corrected first i++ to ++i)

When it comes to improvements

1. You could do with a comment at the head of the file explaining what the program is (rather than explaining things in normal text above)

2. main() is way too long. If a function gets bigger than a screen full (in height), then it's a bad sign. You really need to split the code up into a number of functions (this is actually my main point!)

Factoring out the list management would be a particularly good thing. Same for any other utility code (i.e. code not directly part of the game flow).

3. Your using globals for no reason (on line 7). All the work is beging done by main, so the variables could be declared within its scope. Even if you factor your code into function, there should still be no need for globals.

4. You could compact the switch statement like this

    // roll = 1; is in all cases, inc the default, so should
    // be moved out of switch
    switch (sum) // first roll test
    {
      case 7:
      case 11:
        game_result = WIN;
        break;
      case 2:
      case 3:
      case 12:
        game_result = LOSE;
        break;
      default:
        game_result = CONTINUE;
        point = sum;
        break;
    } // end of switch 

Well, those are the key points. Obviously there are techniques you'll be learning in the near future which help you structure more clearly.

As I've already said, I think the main thing to focus on is using function to structure things (even) more clearly.

Andy

PS Was the break for case 7, starting at line 42, deliberately omitted? (It's a benign bug as the following case does eactly the same thing). And the use of delete[] on looks a bit suspcious; is this a pointer to an array? The only new I can see is allocating a single list node element.

[cpp beginner]

@andy, this is great, thx for the feedback.

a couple things real quick:

6. You using i++ rather than i++

- could you explain what you mean?

1. You could do with a comment at the head of the file explaining what the program is (rather than explaining things in normal text above)

- you're right. i actually used header text w/ this file but took it out to make the post shorter.

2. main() is way too long.

- yep, makes sense. others have suggested i write two programs in cases like this -- 1 for the assignment requirements, and 1 that attempts best practices (i.e. not putting everything in main()).

3. Your using globals for no reason (on line 7). All the work is beging done by main, so the variables could be declared within its scope. Even if you factor your code into function, there should still be no need for globals.

- could you give me an example of what you mean (pseudo code is fine)?

PS Was the break for case 7, starting at line 42, deliberately omitted? (It's a benign bug as the following case does eactly the same thing).

- yeah, i thought it didn't make a difference one way or the other. could you explain what you mean by "it's a benign bug"?

And the use of delete[] on looks a bit suspcious; is this a pointer to an array? The only new I can see is allocating a single list node element.

- could you help me understand what you mean? i'm not well-versed in memory mgmt. i thought it was necessary to destroy a linked list after creation to free memory?

thx!

[Mats]

6. You using i++ rather than i++

I think the last part should be ++i.

- yeah, i thought it didn't make a difference one way or the other. could you explain what you mean by "it's a benign bug"?

The poster meant that in this case it does not cause a bug, but in other programs, it might do. i.e. It is bad practice.

[cpp beginner]

thx mats.

I think the last part should be ++i.

or the other way around?

The poster meant that in this case it does not cause a bug, but in other programs, it might do. i.e. It is bad practice.

yup, that makes sense. i also posted this code to code review (stackexchange) and someone suggested using an array lookup because it, "makes the code a bit more concise and arguably less error prone (it's easy to forget break at the end of switch statements)."

http://codereview.stackexchange.com/questions/26395/i-want-to-improve-my-program-what-questions-could-should-i-ask-myself-during

[giblit]

++i increments first ( prefix ) i++ increments after( post ).
try this

1 2 3 4

int number = 5 , i = 0; i = number++; std::cout << i << std::endl; std::cout << number << std::endl;

5 6

THen this

1 2 3 4

int number = 5 , i = 0; i = ++number; std::cout << i << std::endl; std::cout << number << std::endl;

6 6

As you can see in the first case it sets i equal to number then increments by one( post ) and for the second case it increments number by 1 then sets i equal to the new number.

[andywestken]

I think the last part should be ++i.

or the other way around?

Oops - I've corrected my post.

Yes, I was saying that it was good you are using ++i rather than i++

In a lot of circumstances, either will result in the same machine code. But if the operator++ is for a class, rather than a native data type, the latter can result in a temporary object to be constructed and deleted.

Andy

PS I have seen someone (a question on this site...) do this kind of thing in an iterative function:

int iter_func(int i)
{
   // do stuff and handle termination condition

  return iter_funct(i++);;
}

and then wonder why it blew up the stack.

(iter_funct(++i); would work here, but I'd code it as iter_func(i + 1); to avoid the chance of the mistake.)

Andy