Rubik's cube timer & scrambler

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 ` 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
#include
#include
#include
#include
#include

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;

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 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 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::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 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.

StackExchange Code Review Q#116425, answer score: 12