Critique My Arduino Code

The code:

The goal is to give the user a point if he presses the button while the LED is on, if the user presses the button while the LED is off they loose a point. Any ways you would do this or make it better? I am a beginner in Arduino and code  I always feel like my code could be better/more effecient.

Have you tested it? From a brief look it appears to be all good. My only recommendation is that unless you plan to have points beyond 2147483648 or less than -2147483648, you should use some "short"s, same goes for that pointState variable. That said I don't have an Arduino myself, but I head and teach programming for my Schools FIRST robotics team.

It works mostly how I want it, the main thing I don't like is, though it that it doesn't add a point right away, it has to wait for the next Metro Interval to add it. 

So if I press the button the first time the LED flashed, it doesn't get added and printed until the second time it flashes.

I'm trying to think of a way to fix this.

boolean ledState = LOW;

The only values that should be assigned to a boolean variable are true or false. If you want to use other value, boolean is not the right type.

Metro ledButton = Metro(0);

Some explanation of what the argument to the Metro constructor would be useful.

int button = 2;
int led = 13;
int points = 0;
int pointState = 3;

In the same way that state variables should have state in the name, so should pin number variables. That helps tremendously when you go to read a pin and save it's state. Which variable goes where is self-documenting.

        if (pointState == HIGH){
          pointState = 3;

WTF? Mixing LOW and HIGH and 3 in assigning a value to a variable with state (it should be either HIGH or LOW) makes no sense.


If you worked for me, you'd get the rest of the day off. Without pay. NEVER print a value without some explanatory text. That makes debugging way harder than it needs to be.

And, remember, you asked.


I have this on the Arduino forums as well and someone responded with this, is he making good points? or is he just being a d**k?

To me it seems to be because you are calling  Serial.println(); only after the "else" case when checking button presses. Remove the call from the nested if/else block and it should print your points every loop. That is what you want as I understand it? If I could find my Arduino I'd test it for you but from eye-balling it everything looks ok :-)

Well I do agree with him when it comes to naming conventions for writing Arduino code. It helps a bunch when you have to come back to it after a month and try to figure out what the code is supposed to do. I'd personally do pin_led = 13; var_points = 0; etc just so that I know what each variable is for and how it's supposed to work. But as for the rest of his post, he's just being a dick. :-/

He replied again clarifying some things, I agree that I should use 0, 1, and 2 for the pointStates instead of LOW, HIGH, and 2. It just makes it easier to read in now that I step back and look at it. And as for "I shouldn't assign LOW or HIGH to a boolean I'm not sure about, considering they both equat to the same thing.

While they are both the same thing when compiled the convention with a boolean variable is true or false.  Think of boolean as the answer to a statement.  A statement is either true or false such as an if statement.  High and low are used to describe the state of an input or output.  I cannot recall using high and low outside of IO pins for the arduino for purposes of clarity.

His ending statement is pretty harsh as I would imagine you do not have professional formal training otherwise you would not be asking this question in the first place.   

Okay, I definately get it now. I guess it's sort of just common sense now that I think about it.