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.
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.
Some explanation of what the argument to the Metro constructor would be useful.
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.
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.