HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

C++ port of my Ruby narcotic solitaire simulator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
solitairenarcoticportrubysimulator

Problem

Having undergone a little "rediscovery" of C++, I decided to port my Ruby narcotic solitaire simulator program to it. My goal here is efficiency. This program is already about 5 times faster than the Ruby version on my computer; I want to make it even better.

```
#include
#include
#include
#include
#include
#include
#include

#include "stopwatch.h"

constexpr auto NUM_PILES = 4;
constexpr auto CARD_PLACE_SECONDS = 1;
constexpr auto GATHER_DECK_SECONDS = 5;
constexpr auto CARDS_IN_DECK = 52;
constexpr auto PERCENT = 100;
constexpr auto SECONDS_IN_MINUTE = 60;
constexpr auto MINUTES_IN_HOUR = 60;

using std::endl;

struct IterationData
{
int num_decks;
int completion_time;
bool did_loop;
};

int get_insertion_position(
std::map& value_to_pile_map, int current_index,
int card_value)
{
auto insertion_position = current_index;
auto existing_pile_it = value_to_pile_map.find(card_value);
if (existing_pile_it != value_to_pile_map.end()) {
insertion_position = value_to_pile_map[card_value];
} else {
value_to_pile_map[card_value] = current_index;
}

return insertion_position;
}

/ Erases the last NUM_PILES elements from an std::list. /
void erase_matching_cards(std::list& pile)
{
auto card_remover_it = pile.rbegin();
std::advance(card_remover_it, NUM_PILES);
pile.erase(card_remover_it.base(), pile.end());
}

/ Returns whether all the inserted cards entered the same pile. /
bool distribute_cards(std::list& deck, std::list piles[])
{
auto all_identical = true;
auto value_to_pile_map = std::map();
for (auto i = '\0'; i splice(correct_pile->end(), deck, --deck.end());
}

return all_identical;
}

IterationData narcotic_iterations(std::list& deck)
{
auto completion_time = 0;
auto num_decks = 0;
auto did_loop = false;
std::set> past_decks;

while (!deck.empty()) {
std::list piles[NUM_PILES];
auto insertion_result = past_decks.insert(deck

Solution

Before critiquing your code, I'd like to compliment you. In general I found your code readable and appreciated that you've broken things into short functions, and your variables were well-named.
Do you really need std::list?

Unless you have a compelling reason to use std::list, you should probably stick to std::vector. std::vector will be better for data locality, and offers random-access iterators. That means that your erase_matching_cards can be made to erase items without needing to traverse any of the data structure. I would expect switching away from std::list to improve performance.
Don't use new

In a few places you use dynamic allocation with new and return a pointer to an object. This creates a burden on the programmer to make sure delete is called appropriately, and is not exception safe. Prefer instead to return by value. The object copies can be elided by the compiler.
Don't use arrays

C-style arrays generally aren't the best data structure for, well, anything. They don't know their own size, and decay to pointers when used as function arguments. At the very least, use std::array instead.
const correctness

Declare variables const wherever you can. This lets the compiler guarantee that variables that aren't supposed to change, can't change. Less moving parts makes your code easier to reason about.
Too much auto

This may be open to some debate, but I think you use auto too much. Sometimes you initialize a container like this:

auto shuffler_container = std::vector(CARDS_IN_DECK);


which isn't necessarily bad, but I find it more readable to use the older declaration style

std::vector shuffler_container(CARDS_IN_DECK);


in cases like this, or where I'm initializing primitive types. Although auto num_decks = 0; and int num_decks = 0; are identical, I find the second to be easier to read. No need to wonder about what the inferred type is. That said, keep using auto in cases where the types are tedious (like iterators), and pair it with const for even more fun!

Edward's example has changed my mind. Your use of auto is fine.

Code Snippets

auto shuffler_container = std::vector<int>(CARDS_IN_DECK);
std::vector<int> shuffler_container(CARDS_IN_DECK);

Context

StackExchange Code Review Q#87484, answer score: 2

Revisions (0)

No revisions yet.