Chess countdown timer for Arduino LCD Keypad

I'm very new to C++ and Arduino, with a background in Python. I've written this code for a countdown timer and a simple menu to set the time available to each player, using button handling code from here.

It works as expected, but I don't know whether this is good idiomatic C++/Arduino code or if I could be using better data structures, standard library functions, etc and would be grateful for any tips.

```
// Chess clock using the LiquidCrystal library
#include

/*

This program uses the LCD panel and keypad to create a
chess timer.

The main display, which shows the active player and the
two countdown timers, is as follows:

P1 ChessClock P2
MM:SS -- MM:SS

The menu display, used to set the minutes available for
each player, is:

MENU
Player1 mins: MM

Jamie Bull, February 2016

/
//================================================================
// This code is from a sample library on how to use the LCD keypad
// http://www.dfrobot.com/wiki/index.php?title=Arduino_LCD_KeyPad_Shield_%28SKU:_DFR0009%29

// select the pins used on the LCD panel
LiquidCrystal lcd(8, 9, 4, 5, 6, 7);
// define some values used by the panel and buttons
int lcd_key = 0;
int adc_key_in = 0;
#define btnRIGHT 0
#define btnUP 1
#define btnDOWN 2
#define btnLEFT 3
#define btnSELECT 4
#define btnNONE 5
// read the buttons
int read_LCD_buttons()
{
adc_key_in = analogRead(0); // read the value from the sensor
// buttons when read are centered at these values: 0, 144, 329, 504, 741
// we add approx 50 to those values and check to see if we are close

// We make this the 1st option for speed reasons since it will be the most likely result
if (adc_key_in > 1000) return btnNONE;
if (adc_key_in minutes minutes += 1;
}
};

void Player::DecrementMinutes() {
if (this->minutes > 1) {
this->minutes -= 1;
}
};

int Player::SecondsRemain

There are many cleanups you could apply to this code.

Avoid using mutable global data

Your program is small, but it already deals with a lot of mutable global data. This makes your code much harder to understand and reason about, because the relations between data and code are not clear. Any global var can pretty much be changed anywhere.

If you restructure your code to use function parameters, most of those globals can be declared as local function-level variables. You can also use more classes to group related state. You should definitely consider consolidating all that into a ChessClock class that represents your application. Then the loop function could look something like this:

void loop()
{
    ChessClock clock;
    clock.run();
}

Use typed constants or enum

#define macros are not the best option for numeric constants. It is better to use a typed constant instead when you just need to declare some constant number, for instance:

const int FOO = 123;

But in your case, an enum would be an even better fit, since then values are in sequence:

enum Button
{
    BTN_RIGHT,
    BTN_UP,
    BTN_DOWN,
    BTN_LEFT,
    BTN_SELECT,
    BTN_NONE
};

Notice that you don't have to explicitly say the values in the enum. The first constant starts at 0 and increments from there. You can also use the enum type Button to declare variables that will hold a constant from that enumerated type, e.g.:

Button currentButton = BTN_NONE;

Avoid repetition / Don't Repeat Yourself (DRY)

You have quite a few blocks of code there that are just copy-paste changing one or two parameters. Code duplication is a source of many problems and pretty much the reason why programmers invented constructs like functions and classes. For instance, in updateCounters:

void updateCounters() {
 if (p0.isActive) {
   p0.secondsRun += millis()/1000 - (
     p0.secondsRun + 
     p1.secondsRun + 
     p2.secondsRun
     );
   }
 if (p1.isActive) {
   p1.secondsRun += millis()/1000 - (
     p0.secondsRun + 
     p1.secondsRun + 
     p2.secondsRun
     );
   }
 if (p2.isActive) {
   p2.secondsRun += millis()/1000 - (
     p0.secondsRun + 
     p1.secondsRun + 
     p2.secondsRun
     );
   }
}

Notice how the three blocks are identical, except for the first player instance? That calls for a separate function! Make that secondsRun update logic a member of Player and let each player handle its update. Be sure to pass the other two player instances as function parameters, to avoid relying on globals.

Moving to loop(), there are repeated blocks in the main switch as well. Try to identify the repeated blocks of code and refactor then into auxiliary functions, always using function parameters when you need to pass them some data.

Use standard bool

This boolean type your are using is probably an extension declared by some library that you imported. The standard boolean type in C++ is spelled bool. Try to always use the standard types.

Don't this-> qualify member variable access

This is not a common practice in C++ and only results in more code verbosity. It can also lead to potentially dangerous hiding of name collisions.

Two spaces indent?

This is my personal opinion, but I find just two spaces for each tab/indent to be a bit short. Makes it a little harder to identify the code structure, even though this style is popular in some circles because it produces shorter lines, I don't think it pays off in readability. Suggested 4 spaces for tabs/indent.

StackExchange Code Review Q#120023, answer score: 5