Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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;
        }
      }
    }
  }
}
share|improve this question
up vote 12 down vote accepted

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.

share|improve this answer
    
The container being described looks more like std::forward_list than std::list. – Morwenn Jan 11 at 20:15
    
@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. – Barry Jan 11 at 20:49
    
Accuracy is always your friend :p – Morwenn Jan 11 at 20:54
    
Thanks for the great advice man, learned what const is today! Again thanks will definitely improve next time! Amazing community. – Swluo Jan 12 at 1:04

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();
    }
  }
}
share|improve this answer
    
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! – Swluo Jan 12 at 1:02
    
@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. – Dannnno Jan 12 at 1:05
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.

share|improve this answer
    
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. – Swluo Jan 12 at 1:06
    
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. – theosza Jan 12 at 10:22
    
@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.. – alecail Jan 12 at 10:57

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

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.