Practice Code Analysis

[MrCluckyToYou]

Hello, it's been a while since I've been on the forum, but I have been very busy, working and enrolled in school in pursuit of my AAS in programming and development. In what little free time I have left, I sometimes pursue practice exercises of my own and in so doing I stumbled upon this thread: http://www.cplusplus.com/forum/articles/12974/

I attempted the "dungeon crawl" exercise, and following a respected coder's advice I did not look up any ways to solve the problem (although I did look up syntax when I got stuck trying to do something I knew was possible).

I wanted to post my code here and just get some feedback and observations. I have not yet implemented the monsters, but I have begun to consider ways in which I could implement them.

My main questions are:
1. Are there any outright mistakes?
2. Would an object-oriented approach be overkill?
3. What do you think of my current program structure (organization, cohesion, coupling, clarity, etc.)?
4. Do you have any suggestions for improvement, especially any areas where there might be a more elegant solution (I am unfortunately not up to par in mathematics and tend to do things the hard way).

Thanks!

#include <cstdlib>
#include <cstring>
#include <ctime>
#include <iostream>

const int EMPTY = 0, WALL = 1, TRAP = 2, GOAL = 3, PLAYER = 4;
const int UP = 8, RIGHT = 6, DOWN = 2, LEFT = 4, QUIT = 5;
const int MAP_SIZE = 10, TRAP_COUNT = 10;

void populateMap(int [MAP_SIZE][MAP_SIZE]);
void printMap(int [MAP_SIZE][MAP_SIZE]);
void printState(int);
void printPrompt();
int getInput();
void movePlayer(int[2], int, int [MAP_SIZE][MAP_SIZE]);
int checkCollision(int[2], int, int [MAP_SIZE][MAP_SIZE]);

int main()
{
	srand((unsigned int)time(NULL));

	int map[MAP_SIZE][MAP_SIZE];
	populateMap(map);

	int player[2] = { 0,0 };

	int state = 0, nextMove = 0;

	bool quit = false;

	while (!quit)
	{
		if (state == QUIT || state == TRAP || state == GOAL)
		{
			quit = true;
		}
		std::system("CLS");
		printState(state);
		printMap(map);
		printPrompt();
		if (!quit)
		{
			nextMove = getInput();
			state = checkCollision(player, nextMove, map);
			if (state != WALL)
			{
				movePlayer(player, nextMove, map);
			}
		}
	}
}

void populateMap(int map[MAP_SIZE][MAP_SIZE])
{
	// Initialize empty map.
	for (int y = 0; y < MAP_SIZE; y++)
	{
		for (int x = 0; x < MAP_SIZE; x++)
		{
			map[y][x] = EMPTY;
		}
	}

	// Set player and goal in opposing corners.
	map[0][0] = PLAYER;
	map[MAP_SIZE - 1][MAP_SIZE - 1] = GOAL;

	// Distribute traps randomly throughout remaining cells.
	for (int traps = 0; traps < TRAP_COUNT; traps++)
	{
		int x = rand() % 10, y = rand() % 10;
		if (map[y][x] == EMPTY)
		{
			map[y][x] = TRAP;
		}
	}
}

void printMap(int map[MAP_SIZE][MAP_SIZE])
{
	// Print map to screen.
	std::cout << "+ - - - - - - - - - - +" << std::endl;
	for (int y = 0; y < MAP_SIZE; y++)
	{
		std::cout << "|";
		for (int x = 0; x < MAP_SIZE; x++)
		{
			switch (map[y][x])
			{
			case PLAYER: std::cout << " @";
				break;
			/* case TRAP: std::cout << " *";
				break; */
			case GOAL: std::cout << " X";
				break;
			default: std::cout << " .";
			}
		}
		std::cout << " |" << std::endl;
	}
	std::cout << "+ - - - - - - - - - - +" << std::endl;
}

void printState(int state)
{
	switch (state)
	{
		case EMPTY:
			std::cout << "Welcome to tiny dungeon. Try to reach the treasure!" << std::endl;
			break;
		case WALL:
			std::cout << "There's a wall there..." << std::endl;
			break;
		case TRAP:
			std::cout << "You hit a trap! You died..." << std::endl;
			break;
		case GOAL:
			std::cout << "You found the treasure! You win!" << std::endl;
			break;
		case QUIT:
			std::cout << "Thanks for playing. Goodbye..." << std::endl;
			break;
	}
}

void printPrompt()
{
	std::cout << "What is your next move (use numpad)?" << std::endl;
	std::cout << "8. North." << std::endl;
	std::cout << "2. South." << std::endl;
	std::cout << "4. West." << std::endl;
	std::cout << "6. East." << std::endl;
	std::cout << "5. Quit." << std::endl;
}


void movePlayer(int player[2], int move, int map[MAP_SIZE][MAP_SIZE])
{
	int x = 0, y = 1;
	map[player[y]][player[x]] = EMPTY;

	switch (move)
	{
		case UP: player[y] -= 1;
			break;
		case RIGHT: player[x] += 1;
			break;
		case DOWN: player[y] += 1;
			break;
		case LEFT: player[x] -= 1;
			break;
		default:
			break;
	}

	map[player[y]][player[x]] = PLAYER;
}

int getInput()
{
	int nextMove = 0;

	do
	{
		std::cin >> nextMove;
	} while (nextMove < 2 || nextMove > 8);

	return nextMove;
}

int checkCollision(int player[2], int move, int map[MAP_SIZE][MAP_SIZE])
{
	int x = 0, y = 1;
	int hit = EMPTY;

	switch (move)
	{
		case UP:
			if (player[y] - 1 < 0)
			{
				hit = WALL;
			}
			else if (map[player[y] - 1][player[x]] != EMPTY)
			{
				hit = map[y - 1][x];
			}
			break;
		case DOWN: 
			if (player[y] + 1 > MAP_SIZE - 1)
			{
				hit = WALL;
			}
			else if (map[player[y] + 1][player[x]] != EMPTY)
			{
				hit = map[player[y] + 1][player[x]];
			}
			break;
		case LEFT: 
			if (player[x] - 1 < 0)
			{
				hit = WALL;
			}
			else if (map[player[y]][player[x] - 1] != EMPTY)
			{
				hit = map[player[y]][player[x] - 1];
			}
			break;
		case RIGHT:
			if (player[x] + 1 > MAP_SIZE - 1)
			{
				hit = WALL;
			}
			else if (map[player[y]][player[x] + 1] != EMPTY)
			{
				hit = map[player[y]][player[x] + 1];
			}
			break;
		case QUIT:
			hit = QUIT;
	}

	return hit;
}

[deleted account xyzzy]

any suggestions for improvement

1. use enums.

enum square    { EMPTY, WALL, TRAP, GOAL, PLAYER };
enum direction { UP = 8, RIGHT = 6, DOWN = 2, LEFT = 4, QUIT = 5 };

2. Use an STL container, such as std::vector or std::array, instead of C-style arrays.

STL containers can be actual 2D containers, or simulated 2D in 1D.

https://stackoverflow.com/questions/28663299/declaring-a-2d-vector

https://stackoverflow.com/questions/7689288/less-verbose-way-to-declare-multidimensional-stdarray

(Personally I prefer 2D std::vectors over 2D std::arrays, the dimension "ordering" is easier to read. Plus a std::vector can be sized at run-time.)


3. When writing C++ code use the C++ <random> and <chrono> libraries.

https://web.archive.org/web/20180123103235/http://cpp.indi.frih.net/blog/2014/12/the-bell-has-tolled-for-rand/

https://channel9.msdn.com/Events/GoingNative/2013/rand-Considered-Harmful

[jonnin]

you can probably initialize map without a manual loop. It depends; if its done once for the program, you can, if you start over and run again, you need the loop to reset it.
I only see 1 call to populate in main, so I think you could just directly initialize it.

std::system("CLS");
system is highly frowned upon in serious code. the reason is that hacking at your program and seeing this call in it, I can then write cls.bat in your folder that calls my program with your credentials. It is perfectly fine at home or in school etc, but be aware of the issue.

68-75 you may prefer to ensure you get N traps uniquely: random as you did it can over-write and have less than N traps. I mention it because you have an if statement in there. But if you want to ensure you get N traps, you should increment traps in the if, and take it out of the loop increment area:

for (int traps = 0; traps < TRAP_COUNT; )
	{
		int x = rand() % 10, y = rand() % 10;
		if (map[y][x] == EMPTY)
		{
			map[y][x] = TRAP;
                         traps++;

it looks like left/right/up/down can be condensed. the code is really similar, can you make a little function that does the work and call it with parameters there? I am not 100% sure, but it looks like 'maybe'.

[dhayden]

Comment your code. Put a brief comment before each function explaining what it does. Trust me, this is a good habit to get into.

There are two ways to simplify the logic for hitting a wall.

The first is to encode the map so that it's two spaces larger than what you show the user. Put walls all along the corners. So a 5x5 map might display like this:

G - - - -
- - - T -
- T - - -
- - - - -
- - - - X

but what's actually stored in the array is this:

W W W W W W W
W G - - - - W
W - - - T - W
W - T - - - W
W - - - - - W
W - - - - X W
W W W W W W W

Now checkCollision is almost trivial:

int x = player[0];
int y = player[1];
switch(move) {
case UP: y--; break;
case DOWN; y++; break
case LEFT: x--; break;
case RIGHT: x++; break;
}
hit = map[x][y];

The downside is that you have to change a lot of other code to deal with the new coordinates.

The second suggestion is to compute the new proposed x and y values above and then do the checks:

int x = player[0];
int y = player[1];

switch(move) {
case UP: y--; break;
case DOWN; y++; break
case LEFT: x--; break;
case RIGHT: x++; break;
}

if (x<0 || x>= MAP_SIZE ||
    y<0 || y >== MAPSIZE) {
    hit = WALL;
} else {
    hit = map[x][y];
}

// override if they quit
if (move == QUIT) hit = QUIT;

[MrCluckyToYou]

Thanks everyone. I will try implementing some of these suggestions before moving on to adding enemies. Furry Guy, after reading through the link you posted about rand and looking at the code snippets I suddenly have a feeling of inadequacy again... And here I thought I was doing so well! I'm nervous about my advanced classes now. I'm going to try and wrap my head around this though. Thanks again.

[jonnin]

I would not be hard on myself about that. rand() is *still* taught in schools and pops up in about 85% of all web-code where random numbers are needed.
Its part of a persistent 'old way of doing things' out in the wild.

[deleted account xyzzy]

@MrCluckyToYou,

No need to feel inadequate, C++11 (and now C++14/17) has added considerable "heft" to the language. Something most courses of instruction don't cover since using srand/rand is "what has always been done."

The C++ <random> library looks intimidating because it has so many choices beyond what C can do. The first time I looked at the library I certainly was baffled.

A bit of history and theory on generating random numbers might help:

https://www.learncpp.com/cpp-tutorial/59-random-number-generation/

A working group paper in PDF form I found useful in getting a grip on what <random> can do.

http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2013/n3551.pdf

I have shamelessly adapted the "tool-kit" idea on my own, with considerable help from others here with a firm grasp on generating random numbers with C++.

Now when I want to generate random numbers I simply include my custom header file and *voila!*

Here's what I've mashed together, if you want to take a look:

/* A simple toolkit to help beginners using <random> library an easier task */

// shamelessly stolen and adapted from a C++ working paper: WG21 N3551
// http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2013/n3551.pdf

#ifndef __RANDOM_TOOLKIT_HPP__
#define __RANDOM_TOOLKIT_HPP__

#include <chrono>
#include <random>
#include <stdexcept>

namespace rtk
{
   static bool seeded = false;

   inline std::default_random_engine& urng()
   {
      static std::default_random_engine URNG { };

      return URNG;
   }

   inline void srand(bool FORCE_SEED = false)
   {
      static const std::seed_seq::result_type seeds[] { std::random_device {}(),
                                                        std::seed_seq::result_type(std::chrono::system_clock::now().time_since_epoch().count()) };
      static std::seed_seq sseq(std::begin(seeds), std::end(seeds));

      // static unsigned seed = static_cast<unsigned> (std::chrono::system_clock::now().time_since_epoch().count());

      // the URNG can't be reseeded unless forced
      if (!seeded || FORCE_SEED)
      {
         urng().seed(sseq);

         seeded = true;
      }
   }

   inline void srand(unsigned seed, bool FORCE_SEED = false)
   {
      // the URNG can't be reseeded unless forced
      if (!seeded || FORCE_SEED)
      {
         urng().seed(seed);

         seeded = true;
      }
   }

   // two function overloads to obtain uniform distribution ints and doubles
   inline int rand(int from, int to)
   {
      static std::uniform_int_distribution<> dist { };

      if (from > to) { throw std::invalid_argument("bad int params"); }

      return dist(urng(), decltype(dist)::param_type { from, to });
   }

   inline double rand(double from, double to)
   {
      static std::uniform_real_distribution<> dist { };

      if (from > to) { throw std::invalid_argument("bad double params"); }

      return dist(urng(), decltype(dist)::param_type { from, to });
   }

   // function for rolling dice, and checking if the # of pips is nonstandard
   inline int roll_die(int pips)
   {
      //check to see if the number of die pips is less than 2
      if (pips < 2)
      {
         return 0;
      }

      return rand(1, pips);
   }
}

#endif 

And some code to test various parts of the tool-kit:

#include <iostream>
#include "random_toolkit.hpp"

int main()
{
   // "create" a random engine and randomize it
   rtk::srand();

   // let's roll a die 50 times
   for (unsigned loop { }; loop < 50; loop++)
   {
      std::cout << rtk::roll_die(6) << ' ';

      if ((loop + 1) % 20 == 0) { std::cout << '\n'; }
   }
   std::cout << "\n\n";

   // lambda to "flip a coin," returning a text representation of coin side
   auto flip_coin = [] () { return (rtk::rand(0, 1) ? "Heads" : "Tails"); };

   for (size_t loop { }; loop < 25; loop++)
   {
      std::cout << flip_coin() << '\t';

      if ((loop + 1) % 8 == 0) { std::cout << '\n'; }
   }
   std::cout << '\n';

   std::cout << "\nLet's see if we can have a non-standard die.....\nA die with 1 side: ";
   std::cout << rtk::roll_die(1) << '\n';

   std::cout << "A die with zero sides: ";
   std::cout << rtk::roll_die(0) << '\n';

   std::cout << "A die with negative sides: ";
   std::cout << rtk::roll_die(-6) << "\n\n";

   // let's try to create a bad distribution
   std::cout << "Creating a bad random distribution, it should be be\n";
   std::cout << "encapsulated within a try/catch block.  Unless you want to crash.\n";

   try
   {
      auto test { rtk::rand(5, 2) };
   }

   catch (const std::exception & e)
   {
      std::cout << "\n>>> A standard exception was caught, with message '" << e.what() << "'\n";
   }
}

Do I expect you to understand the code above? HA! It took me quite a lot of time before I understood the working paper examples enough to even begin making my own changes.

I have lots of "test code" snippets I've created whenever I am trying to understand some C/C++ feature I've never really used before. Or used without understanding why. Each time I try a change I make mistakes, sometimes something crashes rather horribly, I learn something new.

I refuse to let the language "beat" me. :D