13
\$\begingroup\$

This uses a linked list to store the data. I used a linked list as it's the only type of data structure that I know currently. Using chrono, I can get microseconds into my timer and <random> for a completely random scramble. A main goal of this program is to be fast in the sense of less keystrokes to get scramble and start/stop timer, so I used the Getkeystate as an input method. It works, but I don't have to press enter every time I enter a command, so it guess it fulfilled my goal.

I'm looking for any advice about how to improve my coding. I feel like that the programming course was pretty much throw you into the deep end and just "make things work" so my code is not a robust as it can be. I also pretty much don't know any common practices that people usually implement, so any advice on that would be good too. I tried my best to make the code look good in terms of spacing and the style, but I have it all in one file. Should I have made an include.h for my classes?

#include <iostream> #include <chrono> #include <algorithm> #include <random> #include <vector> #include <windows.h> class times { struct split { //linked list to store data std::chrono::microseconds elapsed; split *next; }; split *head; split best; int num_split; public: //int num_split; times(); void new_split(std::chrono::microseconds add); void display(split); void display_all(); void display_latest(); void display_best(); void clear(); void avg(); void avg5(); void avg10(); }; times::times() { //initialize head and num_split head = 0; num_split = 0; } void times::new_split(std::chrono::microseconds add) { //always push onto head of the list if (num_split == 0) { //if list is empty split *temp = new split; temp->elapsed = add; head = temp; temp->next = 0; num_split++; //assign best split best.elapsed = add; best.next = 0; } else { split *temp = new split; temp->elapsed = add; temp->next = head; head = temp; num_split++; //check for best time if (add < best.elapsed) { best.elapsed = add; } } } void times::display(split x) { using namespace std::chrono; //cout time in format "min:sec:milli:micro" if (num_split == 0) { std::cerr << "No splits recorded" << std::endl; return; } //conversion from duration unsigned long long int total = x.elapsed.count(); unsigned min = 0; unsigned sec = 0; unsigned milli = 0; unsigned micro = 0; min = total/60000000; total = total%60000000; sec = total/1000000; total = total%1000000; milli = total/1000; total = total%1000; micro = total; std::cout << min << ":" << sec << ":" << milli << ":" << micro << std::flush; return; } void times::display_best() { //displays best time display(this->best); return; } void times::display_latest() { //displays latest time display(*head); return; } void times::display_all() { //displays all recorded time, starting from most recent if (num_split == 0) { //error when list empty std::cerr << "No splits recorded." << std::endl; } int j = num_split; //numbers each displayed time split *i = head; //starting from latest (head) while (i) { std::cout << j << ". "; display(*i); // using display() to print std::cout << std::endl; i = i->next; j--; } return; } void times::clear() { //clear linked list, reset num_split, reset head; split *i = head; while (i != 0) { split *j = i->next; delete i; i = j; num_split--; } head = 0; } void times::avg() { //finds and display avg of all recorded time if (num_split == 1) { display_latest(); return; } split *i = head; split temp; temp.elapsed = std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::steady_clock::duration::zero()); while (i) { temp.elapsed = temp.elapsed + i->elapsed; //sum up total time i = i->next; } temp.elapsed = temp.elapsed/num_split; //divided by number of splits display(temp); } void times::avg5() { //find avg of top 5 time using namespace std::chrono; if (num_split < 5) { std::cerr << "Less than 5 recorded splits."; return; } //create array with all recorded time microseconds a_sort[num_split]; split *i = head; for (int j = 0; j < num_split; j++) { a_sort[j] = i->elapsed; i = i->next; } //using vectors, partial sort for top 5 spllit std::vector<microseconds> v_sort(a_sort, a_sort + num_split); std::partial_sort (v_sort.begin(), v_sort.begin()+5, v_sort.end()); //total time for top 5 split microseconds top5; for (int i = 0; i < 5; i++) { top5 += v_sort[i]; } //find avg of top 5 split avg_of_5; avg_of_5.elapsed = top5/5; display (avg_of_5); return; } void times::avg10() { //find avg of top 10 time using namespace std::chrono; if (num_split < 10) { std::cerr << "Less than 10 recorded splits." << std::endl; return; } //create array with all recorded time microseconds a_sort[num_split]; split *i = head; for (int j = 0; j < num_split; j++) { a_sort[j] = i->elapsed; i = i->next; } //using vectors, partial sort for top 10 spllit std::vector<microseconds> v_sort(a_sort, a_sort + num_split); std::partial_sort (v_sort.begin(), v_sort.begin() + 10, v_sort.end()); //total time for top 10 split microseconds top10; for (int i = 0; i < 10; i++) { top10 += v_sort[i]; } //find avg of top 10, return as split split avg_of_10; avg_of_10.elapsed = top10/10; display(avg_of_10); return; } class stop_watch { std::chrono::high_resolution_clock::time_point start_time; //time point start std::chrono::high_resolution_clock::time_point stop_time; //time point stop public: stop_watch(); //constuctor to initialize stop & stop to time_point::min void start(); void stop(); std::chrono::microseconds get_time(); //returns duration in microseconds }; stop_watch::stop_watch() { //initialize time_point to min start_time = std::chrono::high_resolution_clock::time_point::min(); stop_time = std::chrono::high_resolution_clock::time_point::min(); } void stop_watch::start() { //start stop watch, initialize time_point start start_time = std::chrono::high_resolution_clock::now(); } void stop_watch::stop() { //stops stop watch, initialize time_point stop stop_time = std::chrono::high_resolution_clock::now(); } std::chrono::microseconds stop_watch::get_time() { //find duration between stop & start in microseconds using namespace std::chrono; return duration_cast<microseconds>(stop_time - start_time); } class rubik { std::string notation[18] = {"U ","L ","F ","R ", "B ", "D ", " U' ", "L' ", "F' ", "R' ", "B' ", "D' ", "U2 ","L2 ","F2 ","R2 ","B2 ", "D2 "}; std::mt19937 engine; //generate mt19937 engine public: rubik(); int r(); void scramble(); }; rubik::rubik() { //generate random device, seed mt19937 with random device std::random_device seeder; engine.seed(seeder()); } int rubik::r() { //run MT engine and return random number std::uniform_int_distribution<int> distribution(0,17); return distribution(engine); } void rubik::scramble() { //display 25 random rotations //display in groups of 5 for (int i = 0; i < 5; i++) { for (int j = 0; j < 5; j++) { std::cout << notation[r()] << std::flush; } if (i != 4) { std::cout << "| " << std::flush; } } } void key_start() { //start button using left & right shift key bool x = false; while (true) { if (GetAsyncKeyState(VK_LSHIFT) < 0 && GetAsyncKeyState(VK_RSHIFT) < 0) { while (true) { if (GetAsyncKeyState(VK_LSHIFT) >= 0 && GetAsyncKeyState(VK_RSHIFT) >= 0) { x = true; break; } } } if (x == true) { return; } } } void key_stop() { //stop button using space bar while (true) { if (GetAsyncKeyState(VK_SPACE) < 0) { return; } } } void display_menu1() { using namespace std; cout << " 'Left Shift' AND 'Right Shift' - Timer start" << endl << " 'Space' - Timer stop" << endl; } void display_menu2() { using namespace std; cout << " 'Space' - Reset & generate new scramble" << endl << " 'A' - Show all recorded times" << endl << " 'C' - Clear all recorded times" << endl << " 'ESC' - Exit program" << endl; //<< " 'S' - Save time as txt" << endl; //implement this feature later, prompt user for file name } int options() { //using keystate to select options while (true) { if (GetAsyncKeyState(0x43) < 0) return 1; // 'C' else if(GetAsyncKeyState(VK_ESCAPE) < 0) return 2; // 'ESC' else if (GetAsyncKeyState(VK_SPACE) < 0) return 3; // spacebar; else if (GetAsyncKeyState(0x41) < 0) return 4; // 'A'; } } void clear_console() { //clears clear_console for (int i = 0; i < 100; i++) { std::cout << std::endl; } } int main() { using namespace std; //class decleartions stop_watch timer; times my_time; rubik cube; while (true) { clear_console(); display_menu1(); cout << endl; cube.scramble(); cout << endl; //display scramble //timer key_start(); timer.start(); key_stop(); timer.stop(); my_time.new_split(timer.get_time()); //save time, add to linked list cout << endl << "Latest: "; my_time.display_latest(); cout << endl << endl; cout << "Best: "; my_time.display_best(); cout << endl; cout << "Average: "; my_time.avg(); cout << endl; cout << "Average of 5: "; my_time.avg5(); cout << endl; cout << "Average of 10: "; my_time.avg10(); cout << endl; display_menu2(); cout << endl; Sleep(400); int input = options(); if (input != 3) { while (true) { //clear linked list if (input == 1) { my_time.clear(); cout << endl << "Cleared!" << endl; Sleep(500); clear_console(); display_menu2(); Sleep(400); input = options(); if (input == 3) break; } if (input == 4) { //display all recorded time clear_console(); my_time.display_all(); cout << endl; display_menu2(); cout << endl; Sleep(400); input = options(); if (input == 3) break; } if (input == 2) { //exit program return 0; } } } } }
\$\endgroup\$

4 Answers 4

Reset to default
13
\$\begingroup\$

Use the standard library

One of the things that immediately jumps out at me is this:

struct split { //linked list to store data std::chrono::microseconds elapsed; split *next; }; split *head;

You don't have a destructor or copy constructor provided, which means you're leaking memory. Also, you're making the whole problem harder on yourself since the standard library provides a linked list container for you already: std::list:

std::list<std::chrono::microseconds> splits;

Although vector would definitely be the preferred container since you're always appending and traversing in order - and never doing any of the operations for which linked list is favored:

std::vector<std::chrono::microseconds> splits;

This also obviates the need for num_split, since now you can do splits.size(). Using the standard container avoids the whole memory leak issue completely - and now you don't need to write a destructor or copy constructor. It also makes all your operations way easier. For example:

void times::new_split(std::chrono::microseconds add) { splits.push_back(add); } void times::display_all() { if (splits.empty()) { // log your error } for (size_t i = 0; i < splits.size(); ++i) { std::cout << i << ". "; display(splits[i]); } } void times::clear() { splits.clear(); }

And so on.

avg(), avg5(), and avg10()

The first function takes the average of ALL times, but the next two take the average of the TOP times for some number. That number should be an argument and the function should be named differently. In any event, avg() should probably return the average, not just print it:

std::chrono::microseconds average(); std::chrono::microseconds average_top(size_t );

You could implement both with an iterator-pair helper function:

using MicroIter = std::vector<std::chrono::microseconds>::const_iterator; std::chrono::microseconds average_iter(MicroIter first, MicroIter last);

So that the first average just forwards everything:

std::chrono::microseconds average() { return average_iter(splits.begin(), splits.end()); }

And the second one copies and does a partial sort: 

std::chrono::microseconds average_top(size_t n) { std::vector<std::chrono::microseconds> copy = splits; std::partial_sort(copy.begin(), copy.begin() + n, copy.end(), std::greater<>{}); // need this part too! return average_iter(copy.begin(), copy.begin() + n); }

That way you only implement the average logic once.

If a member function doesn't change the object, make it const

Good practice. A lot of your member functions (e.g. average(), display_best(), etc.) don't make any modifications to split. So they should be const.

Just expose the splits

Rather than having display(split ), display_all(), display_latest(), display_best(), just expose everything to the user:

MicroIter begin() const; MicroIter end() const;

By making it const, they can't edit it anyway, and then they can display anything that they want to display.

\$\endgroup\$
4
  • \$\begingroup\$ The container being described looks more like std::forward_list than std::list. \$\endgroup\$ Commented Jan 11, 2016 at 20:15
  • \$\begingroup\$ @Morwenn I don't think that matters at all? OP doesn't need a singly linked list any more than OP needs a doubly linked one. \$\endgroup\$ Commented Jan 11, 2016 at 20:49
  • \$\begingroup\$ Accuracy is always your friend :p \$\endgroup\$ Commented Jan 11, 2016 at 20:54
  • \$\begingroup\$ Thanks for the great advice man, learned what const is today! Again thanks will definitely improve next time! Amazing community. \$\endgroup\$ Commented Jan 12, 2016 at 1:04
10
\$\begingroup\$

Typically user-defined types are named with a leading capital, i.e. Times, Split, etc.

You should put your interface in a header (hpp) file, and your implementation in a cpp file.

I like to explicitly label the private sections of my classes.

Unlike in C, when you have a pointer they are traditionally declared as Type* name not Type *name, but this is a personal preference thing.

You probably don't want best (I assume this is the best split they've gotten) to be a copy - prefer a pointer.

You don't need to grow your own data structure to hold the data - just use std::vector. I don't know what courses you've taken, but generally linked-lists are known for having good complexity (O(1)) for arbitrary insertion and erasing, but practically speaking I've never seen a timing test where a linked-list outperformed a vector, even tests designed to favor linked-lists. I also find vectors easier to reason about.

I generally prefer unsigned int to int whenever I'm counting something and a negative value doesn't make sense.

Prefer to use a colon initialized constructor - this avoids setting values too many times or wasting memory.

// this is better Times::Times() : _best(std::chrono::microseconds::max()) { // pick a reasonable number, whatever that may be for your application _splits.reserve(100); } // than this times::times() { //initialize head and num_split head = 0; num_split = 0; }

new_split is probably better named as add_split. I don't see any particular reason that they have to be put at the front (if there is, you just have to change this to use std::vector::insert instead.

Times::add_split(std::chrono::microseconds add) { _splits.push_back(add); if (add < best) { best = add; } }

Generally I don't like to use a namespace inside of a function. Either put that at the top of the file so its easy to see, or don't do it. Additionally, you should be using named constants instead of magic numbers. Even better, however, would be to use duration_cast instead of what you were doing.

void Times::display(microseconds split) { if (_splits.empty()) { std::cerr << "No splits recorded." << std::endl; return; } const unsigned int microsPerMinute = 60000000; const unsigned int microsPerSecond = 1000000; const unsigned int microsPerMillisecond = 1000; //conversion from duration unsigned long long int totalMicroseconds = split.count(); unsigned min = duration_cast<minutes>(split).count(); unsigned sec = duration_cast<seconds>(totalMicroseconds %= microsPerMinute).count(); unsigned milli = duration_cast<milliseconds>(totalMicroseconds %= microsPerSecond).count(); unsigned micro = totalMicroseconds % microsPerMillisecond; std::cout << min << ":" << sec << ":" << milli << ":" << micro << std::flush; }

This function also seems like it could be private - it seems to expect a split that is within the vector of splits, which isn't a guarantee if it's public.

Prefer range-based for loops instead of doing it manually.

void Times::display_all() { if (_splits.empty()) { std::cerr << "No splits recorded." << std::endl; return; } for (const auto& split : _splits) { display(split); } }

Similarly, prefer to use standard algorithms instead of doing stuff by hand.

void Times::avg() { if (_splits.empty()) { std::cerr < "No splits recorded." << std::endl; return; } microseconds duration = std::accumulate( _splits.begin(), _splits.end(), duration_cast<microseconds>(steady_clock::duration::zero()); duration /= _splits.size(); display(duration); }

Be DRYer.

void Times::average_all() const { if (_splits.empty()) { std::cerr < "No splits recorded." << std::endl; return; } average_top_n(_splits.size()); } void Times::average_top_five() const { average_top_n(5); } void Times::average_top_ten() const { average_top_n(10); } void Times::average_top_n(unsigned int numToAverage) const { if (_splits.size() < numToAverage) { std::cerr << "Less than " << numToAverage << " recorded splits."; return; } std::vector<microseconds> v_sorted(_splits); std::partial_sort(v_sorted.begin(), v_sorted.begin()+numToAverage, v_sorted.end()); microseconds duration = std::accumulate( _splits.begin(), _splits.begin()+5, duration_cast<microseconds>(steady_clock::duration::zero()); duration /= numToAverage; display(duration); }

Prefer preincrement and unsigned ints when iterating. You also don't need to flush that often.

void RubiksCube::scramble() { //display 25 random rotations //display in groups of 5 for (unsigned int i = 0; i < 5; ++i) { for (unsigned int j = 0; j < 5; ++j) { std::cout << notation[get_random_number()]; } if (i != 4) { std::cout << "| "; } } }

You can avoid nesting by being more clever with your booleans.

void key_start() { //start button using left & right shift key bool started = false; while (true) { if (GetAsyncKeyState(VK_LSHIFT) < 0 && GetAsyncKeyState(VK_RSHIFT) < 0) { while (!started) { started = GetAsyncKeyState(VK_LSHIFT) >= 0 && GetAsyncKeyState(VK_RSHIFT) >= 0; } } if (started) { return; } } } void key_stop() { //stop button using space bar while (GetAsyncKeyState(VK_SPACE) >= 0); return; }

Don't ever do using namespace std (or really any namespace).

Prefer constants to magic numbers, and enums make it even better

enum Option { CLEAR, EXIT, END, DISPLAY_ALL }; Option options() { const unsigned int C_KEY = 0x43; const unsiged int A_KEY = 0x41; //using keystate to select options while (true) { if (GetAsyncKeyState(C_KEY) < 0) return CLEAR; // 'C' else if(GetAsyncKeyState(VK_ESCAPE) < 0) return EXIT; // 'ESC' else if (GetAsyncKeyState(VK_SPACE) < 0) return END; // spacebar; else if (GetAsyncKeyState(A_KEY) < 0) return DISPLAY_ALL; // 'A'; } }

Here is the fully rewritten code (I'm on a computer with no compiler, so no guarantee all of this works exactly). I also made more changes within that I didn't specifically enumerate above - if you have questions about those please let me know. In general I tried to clean things up to reduce nesting and repetition, to add include guards for safety, to make things const as needed, and remove unnecessary comments.

rubiks_cube_splits.hpp

#ifndef RUBIKS_CUBE_SPLITS_HPP #define RUBIKS_CUBE_SPLITS_HPP #include <vector> #include <chrono> class Times { public: Times(); void add_split(std::chrono::microseconds add); void display_all() const; void display_latest() const; void display_best() const; void clear(); void average_all() const; void average_top_five() const; void average_top_ten() const; private: void display(std::chrono::microseconds split) const; void average_top_n(unsigned int numToAverage) const; std::vector<std::chrono::microseconds> _splits; std::chrono::microseconds _best; }; #endif

rubiks_cube_splits.cpp

#include "rubiks_cube_splits.hpp" #include <iostream> #include <chrono> #include <algorithm> #include <numeric> #include <vector> using namespace std::chrono; Times::Times() : _best(microseconds::max()) { // pick a reasonable number, whatever that may be for your application _splits.reserve(100); } Times::add_split(microseconds add) { _splits.push_back(add); if (add < _best) { _best = add; } } void Times::display(microseconds split) const { if (_splits.empty()) { std::cerr << "No splits recorded." << std::endl; return; } const unsigned int microsPerMinute = 60000000; const unsigned int microsPerSecond = 1000000; const unsigned int microsPerMillisecond = 1000; //conversion from duration unsigned long long int totalMicroseconds = split.count(); unsigned min = duration_cast<minutes>(split).count(); unsigned sec = duration_cast<seconds>(totalMicroseconds %= microsPerMinute).count(); unsigned milli = duration_cast<milliseconds>(totalMicroseconds %= microsPerSecond).count(); unsigned micro = totalMicroseconds % microsPerMillisecond; std::cout << min << ":" << sec << ":" << milli << ":" << micro << std::endl; } void Times::display_best() const { if (_splits.empty()) { std::cerr << "No splits recorded." << std::endl; return; } display(_best); } void Times::display_latest() const { if (_splits.empty()) { std::cerr << "No splits recorded." << std::endl; return; } display(_splits.back()); } void Times::display_all() const { if (_splits.empty()) { std::cerr << "No splits recorded." << std::endl; return; } for (const auto& split : _splits) { display(split); } } void Times::clear() { _splits.clear(); _best = microseconds::max(); } void Times::average_all() const { if (_splits.empty()) { std::cerr < "No splits recorded." << std::endl; return; } average_top_n(_splits.size()); } void Times::average_top_five() const { average_top_n(5); } void Times::average_top_ten() const { average_top_n(10); } void Times::average_top_n(unsigned int numToAverage) const { if (_splits.size() < numToAverage) { std::cerr << "Less than " << numToAverage << " recorded splits."; return; } std::vector<microseconds> v_sorted(_splits); std::partial_sort(v_sorted.begin(), v_sorted.begin()+numToAverage, v_sorted.end()); microseconds duration = std::accumulate( _splits.begin(), _splits.begin()+5, duration_cast<microseconds>(steady_clock::duration::zero()); duration /= numToAverage; display(duration); }

rubiks_cube_stopwatch.hpp

#ifndef RUBIKS_CUBE_STOP_WATCH #define RUBIKS_CUBE_STOP_WATCH #include <chrono> class StopWatch { public: StopWatch(); void start(); void stop(); std::chrono::microseconds get_time() const; private: std::chrono::high_resolution_clock::time_point start_time; std::chrono::high_resolution_clock::time_point stop_time; }; #endif

rubiks_cube_stopwatch.cpp

#include "rubiks_cube_stopwatch.hpp" using std::chrono; StopWatch::StopWatch() : _start(high_resolution_clock::time_point::min()), _stop(high_resolution_clock::time_point::min()) {} void StopWatch::start() { _start = std::chrono::high_resolution_clock::now(); } void StopWatch::stop() { _stop = std::chrono::high_resolution_clock::now(); } microseconds StopWatch::get_time() const { return duration_cast<microseconds>(stop_time - start_time); }

rubiks_cube.hpp

#ifndef RUBIKS_CUBE_HPP #define RUBIKS_CUBE_HPP #include <string> #include <random> class RubiksCube { public: RubiksCube(); void scramble(); private: int get_random_number(); const std::string _notation[18] = { "U ","L ","F ","R ", "B ", "D ", "U' ", "L' ", "F' ", "R' ", "B' ", "D' ", "U2 ","L2 ","F2 ","R2 ","B2 ", "D2 "}; std::mt19937 _engine; std::uniform_int_distribution<int> _distribution; }; #endif

rubiks_cube.cpp

#include "rubiks_cube.hpp" #include <string> #include <random> RubiksCube::RubiksCube() _distribution(0, 17){ std::random_device seeder; engine.seed(seeder()); } int RubiksCube::get_random_number() { return _distribution(engine); } void RubiksCube::scramble() { //display 25 random rotations //display in groups of 5 for (unsigned int i = 0; i < 5; ++i) { for (unsigned int j = 0; j < 5; ++j) { std::cout << notation[get_random_number()]; } if (i != 4) { std::cout << "| "; } } }

main.cpp

#include "rubiks_cube_stopwatch.hpp" #include "rubiks_cube_splits.hpp" #include "rubiks_cube.hpp" #include <windows.h> #include <iostream> enum Option { CLEAR, EXIT, END, DISPLAY_ALL }; void key_start() { //start button using left & right shift key bool started = false; while (true) { if (GetAsyncKeyState(VK_LSHIFT) < 0 && GetAsyncKeyState(VK_RSHIFT) < 0) { while (!started) { started = GetAsyncKeyState(VK_LSHIFT) >= 0 && GetAsyncKeyState(VK_RSHIFT) >= 0; } } if (started) { return; } } } void key_stop() { //stop button using space bar while (GetAsyncKeyState(VK_SPACE) >= 0); return; } void display_menu1() { std:: cout << " 'Left Shift' AND 'Right Shift' - Timer start\n" << " 'Space' - Timer stop" << endl; } void display_menu2() { std::cout << " 'Space' - Reset & generate new scramble\n" << " 'A' - Show all recorded times\n" << " 'C' - Clear all recorded times\n" << " 'ESC' - Exit program" << std::endl; //<< " 'S' - Save time as txt" << std::endl; //implement this feature later, prompt user for file name } Option options() { const unsigned int C_KEY = 0x43; const unsiged int A_KEY = 0x41; //using keystate to select options while (true) { if (GetAsyncKeyState(C_KEY) < 0) return CLEAR; // 'C' else if(GetAsyncKeyState(VK_ESCAPE) < 0) return EXIT; // 'ESC' else if (GetAsyncKeyState(VK_SPACE) < 0) return END; // spacebar; else if (GetAsyncKeyState(A_KEY) < 0) return DISPLAY_ALL; // 'A'; } } void clear_console() { for (int i = 0; i < 100; i++) { std::cout << std::endl; } } int main() { StopWatch timer; Times my_time; RubiksCube cube; while (true) { clear_console(); display_menu1(); std::cout << std::endl; cube.scramble(); std::cout << std::endl; //display scramble key_start(); timer.start(); key_stop(); timer.stop(); my_time.add_split(timer.get_time()); std::cout << "\nLatest: "; my_time.display_latest(); std::cout << "\n\nBest: "; my_time.display_best(); std::cout << "\nAverage: "; my_time.avg(); std::cout << "\nAverage of 5: "; my_time.avg5(); std::cout << "\nAverage of 10: "; my_time.avg10(); std::cout << std::endl; display_menu2(); std::cout << std::endl; Sleep(400); Option input = options(); while (input != END) { if (input == CLEAR) { my_time.clear(); std::cout << "\nCleared!\n"; Sleep(500); clear_console(); display_menu2(); Sleep(400); } else if (input == DISPLAY_ALL) { //display all recorded time clear_console(); my_time.display_all(); cout << endl; display_menu2(); cout << endl; Sleep(400); } else if (input == EXIT) { return 0; } input = options(); } } }
\$\endgroup\$
2
  • \$\begingroup\$ Holy cow dude! Thanks so much for this i've read through everything and will definitely be learning from my mistake to improve next time. A question question is how long do you think it took you to just rewrite the code itself? Again thanks! \$\endgroup\$ Commented Jan 12, 2016 at 1:02
  • \$\begingroup\$ @Swluo no problem, I enjoy it. Also definitely make sure to read through Barry's answer - it's shorter, but it makes the most important points of the review there. I fully endorse every one of his suggestions, several of which I didn't make very clear in my review. \$\endgroup\$ Commented Jan 12, 2016 at 1:05
3
\$\begingroup\$ 
class rubik { std::string notation[18] = {"U ","L ","F ","R ", "B ", "D ", " U' ", "L' ", "F' ", "R' ", "B' ", "D' ", "U2 ","L2 ","F2 ","R2 ","B2 ", "D2 "}; std::mt19937 engine; //generate mt19937 engine public: rubik(); int r(); void scramble(); }; void rubik::scramble() { //display 25 random rotations //display in groups of 5 for (int i = 0; i < 5; i++) { for (int j = 0; j < 5; j++) { std::cout << notation[r()] << std::flush; } if (i != 4) { std::cout << "| " << std::flush; } } }

This will generate sequences which are compressible to a simpler sequence, or worse, to nil..

For example:

(R2UFB'R2F'BUR2)^3 is nil, but that's not easy to see. This is a sequence for PLL, which rotates three faces.

But.. UUU' = U, FF' = nil

At least, don't allow a subsequence to be reduced to nil

Then, you could prevent three contiguous rotations to be in the same plane.

(LR)U is ok. But LRL is not. It should always be rewritten to L2R.

\$\endgroup\$
3
  • \$\begingroup\$ Mhmm i completely didn't think about how by implementing a completely random scrambler that there will be such repeats. Thanks for pointing it out. \$\endgroup\$ Commented Jan 12, 2016 at 1:06
  • \$\begingroup\$ Making a good cube scrambler is surprisingly hard. Might not matter when you just want to practise, but a competition-level scrambler should give an unbiased scramble (all cubes equally likely). The way to do this is generate a random (solvable) cube. Solve it using a near-optimal algorithm like Kociemba's algorithm. Then the scramble is the inverse of the solution moves. Neither the generating nor solving of the cube are trivial. \$\endgroup\$ Commented Jan 12, 2016 at 10:22
  • \$\begingroup\$ @theosza Right.. At least I believe if he should try to avoid trivial nil operations, like reverse palindromic subsequences, and put the scramble sequence in a standard form, but as you said, just generating unbiased scramble is certainly not possible by just throwing a bunch a random moves.. \$\endgroup\$ Commented Jan 12, 2016 at 10:57
3
\$\begingroup\$

Not a massive fix but

int options() { //using keystate to select option while (true) { if (GetAsyncKeyState(0x43) < 0) return 1; // 'C' else if(GetAsyncKeyState(VK_ESCAPE) < 0) return 2; // 'ESC' else if(GetAsyncKeyState(VK_SPACE) < 0) return 3; // spacebar; else if(GetAsyncKeyState(0x41) < 0) return 4; // 'A'; } }

else does not add any functionality to the code

if(GetAsyncKeyState(VK_ESCAPE) < 0) does not depend on
if(GetAsyncKeyState(0x43) < 0) being false.

All elses can be removed

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.