patterncppMinor
C++ port of my Ruby narcotic solitaire simulator
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
```
#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
Unless you have a compelling reason to use
Don't use
In a few places you use dynamic allocation with
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
Declare variables
Too much
This may be open to some debate, but I think you use
which isn't necessarily bad, but I find it more readable to use the older declaration style
in cases like this, or where I'm initializing primitive types. Although
Edward's example has changed my mind. Your use of
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
newIn 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 correctnessDeclare 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
autoThis 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.