Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update BlinkWithoutDelay.ino example #7239

Closed
wants to merge 1 commit into from
Closed

Update BlinkWithoutDelay.ino example #7239

wants to merge 1 commit into from

Conversation

codetheorist
Copy link

@codetheorist codetheorist commented Feb 18, 2018

  • Remove unused ledPin constant
  • Make comments more natural language
  • Change if/else statement to demonstrate shorthand variable flipping

* Remove unused ledPin constant
* Make comments more natural language
* Change if/ese statement to demonstrate shorthand variable flipping
@cousteaulecommandant
Copy link
Contributor

cousteaulecommandant commented Feb 22, 2018

I think that, although the code becomes more compact and simple, it also becomes less explicit and deviates from the typical Arduino code. Keep in mind that this is an example and thus needs to be easy to understand and modify.

  • Remove unused ledPin constant

I don't think this is a good idea. Sure, you could just use the macro every time instead of assigning it to a constant, but then if you want to change the pin you use you have to change EVERY line where it appears and risk forgetting one of them; it's better practice to define the pin at the beginning so that it can be modified easily.

  • Change if/else statement to demonstrate shorthand variable flipping

I think this is not the point of this example; better to leave it as an explicit "if it's low then set it high, otherwise set it low" and leave the usage of ! to another example.
Also I'd say HIGH and LOW are not to be interpreted as true and false boolean values in Arduino, but rather as two abstract pin states (which just happen to be defined as 1 and 0, but that's an implementation detail that isn't even in their documentation). Think of them as an enum.

  • Make comments more natural language

This part was good in my opinion (except for the Constants won't change/Variables will change part; I think it's useful to specify that distinction since they're part of what's being explained here).


PS: I forgot to mention, but it seems that you removed the newline at the end of the file. Although C++ allows it and Arduino might be fine with that, it's better practice to always have your text files ending in a newline (for example it's illegal not to do so in C).

@DRSDavidSoft
Copy link

@codetheorist With the introduction of LED_BUILTIN, I thought this example should have been modified according to your edit. However, as @cousteaulecommandant already pointed out, that' not the point of the example.

However, as a better coding pattern for amateurs, I'd appreciate if @codetheorist still somehow makes its way to the examples.

@cousteaulecommandant
Copy link
Contributor

I disagree. Although LED_BUILTIN is a useful macro for referring to the built-in LED, I'd rather keep it this way, since this macro refers to a specific LED whereas the constant intends to refer to "whichever LED the design uses". It's not really redundant since both refer to different concepts ("LED used in this program" vs "LED built into board").

Personally I strongly believe it's better practice to define all constants that will be used in the application in a single place near the beginning (as constants or as macros, which may or may not refer to other macros). I have found that it eventually simplifies modifying the program later.

@per1234
Copy link
Collaborator

per1234 commented Sep 26, 2020

Moved to arduino/arduino-examples#15

@per1234 per1234 closed this Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants