Button library improvements #23
Conversation
eojreyem
added
documentation
|
I think this looks good, Joe. Pretty easy to follow, and seems easy to implement. |
|
@sanine-a I'm guessing you are not on board with all of these decisions (private vars, constructor vs setup). Next time we're both in the shop we can discuss this. Not sure if I made my case well above, and you may want to make the case for doing it a different way. Merging this is not pressing. I'm happy to take the time to discuss changes as this should be something we all agree is useful. |
There was a problem hiding this comment.
Hey @eojreyem
This looks like a good improvement over what we had before. Good job on that front! It makes more sense to move all those variables over to private scope and to have the setup function abstracted away like this.
I do have some questions about code styling. The curly bracket styling got changed, which is fine and I feel like I've seen y'all use it regularly. I think it would be good to commit to uniform styling to help make code cleaner and easier to maintain. Would all of you agree this style of curly brackets is how we collectively want to standardize our code in the future:
void func()
{
// logic
}
The other thing is listener name change. I think that name captures what it is doing more, but if that's the case, we should make a ticket to explicitly rename all of the libraries' update functions to listener.
|
I'm fine with either. I've seen both ways, though I've seen most example
code written as below.
void func() {
//logic
}
------------------------------
<https://www.smm.org/>
*David Bailey*
*Exhibit Fabricator IIII*
e: [email protected]
o: (651) 456-8242
…On Thu, Jan 21, 2021 at 10:36 AM Brandon W. Kipp ***@***.***> wrote:
***@***.**** approved this pull request.
Hey @eojreyem <https://github.com/eojreyem>
This looks like a good improvement over what we had before. Good job on
that front! It makes more sense to move all those variables over to
private scope and to have the setup function abstracted away like this.
I do have some questions about code styling. The curly bracket styling got
changed, which is fine and I feel like I've seen y'all use it regularly. I
think it would be good to commit to uniform styling to help make code
cleaner and easier to maintain. Would all of you agree this style of curly
brackets is how we collectively want to standardize our code in the future:
void func()
{
// logic
}
The other thing is listener name change. I think that name captures what
it is doing more, but if that's the case, we should make a ticket to
explicitly rename all of the libraries' update functions to listener.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#23 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALFATRNSTKOO6CPLLEWLLBTS3BJZJANCNFSM4WLG3QWQ>
.
|
|
Nice work @eojreyem. I'm curious, is I think it might be a little misleading to use a noun as the method name. I understand by adding that line of code inside the main Also, I think what's nice about |
|
As you reach agreements here please add any notes to our Code Style Guide article. Please build off of the guides in the community. E.g., https://www.arduino.cc/en/Reference/APIStyleGuide |
|
Thank you guys for the feedback! @brandonwkipp {} - That's something that hasn't been on our radar at all down here in the shop. I'm really enjoying using VSCode and using the C/C++ extension you found. I've grown so use to it formatting my code when I save that I now rely on ctrl+S to format rather than typing the indents myself. @tnordberg and @brandonwkipp I think the convention that has informal agreement down in the shop was 'update'. Given Trygve's framing, I guess it isn't as inaccurate as I though. I'm going to revert that back to update. |
|
I'd like to merge this week. @sanine-a do you have any feedback? |
|
Do it!
------------------------------
<https://www.smm.org/>
*David Bailey*
*Exhibit Fabricator IIII*
e: [email protected]
o: (651) 456-8242
…On Wed, Feb 17, 2021 at 9:26 AM Joe Meyer ***@***.***> wrote:
I'd like to merge this week. @sanine-a <https://github.com/sanine-a> do
you have any feedback?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#23 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALFATRN4G52QRYF4OPAY7OTS7PNZTANCNFSM4WLG3QWQ>
.
|
|
My only feedback is that I agree with Brandon that we should standardize our curly bracket style. I'm partial to the brackets being on different lines because I think it helps serve to break up function declarations and their code blocks, and it makes formatting constructor initialization lists a little nicer, but those are purely aesthetic considerations and I'd be perfectly fine if we standardized in the other direction. (I might open an issue on this subject...) This looks great -- I vote merge it! |
|
Kate,
Do you mean this:
void loop(){
}
or this:
void loop()
{
}
I feel like the first one is almost universally used in Arduino code
examples.
Dave
------------------------------
<https://www.smm.org/>
*David Bailey*
*Exhibit Fabricator IIII*
e: [email protected]
o: (651) 456-8242
…On Wed, Feb 17, 2021 at 9:49 AM sanine ***@***.***> wrote:
My only feedback is that I agree with Brandon that we should standardize
our curly bracket style. I'm partial to the brackets being on different
lines because I think it helps serve to break up function declarations and
their code blocks, and it makes formatting constructor initialization lists
a little nicer, but those are purely aesthetic considerations and I'd be
perfectly fine if we standardized in the other direction. (I might open an
issue on this subject...) This looks great -- I vote merge it!
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#23 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALFATRI4WBIUSK3ZBBCEKV3S7PQQJANCNFSM4WLG3QWQ>
.
|
|
I argue for keeping it on the same line. In most Arduino sample code I've seen, it is on the same line. If we use 3rd party libraries, as we often do, they typically use that formatting. |
Readability
I had a hard time following how this worked so...
update() renamed to listener()
This may be pedantic (but most of programming is?) and I know we already changed it once.
We are not "updating" the button, we are adding a "listener" to a loop in our sketch.
If this is the only thing you're hung up on, I can revert this change.
Removed setup() in favor of doing button setup in the constructor
Previously this was an empty constructor and one would run setup(pin,callback,etc) in the arduino sketch setup function.
I think there are advantages to getting rid of the setup function.
An example sketch
Examples>Button>Button.ino
I put a button example sketch in the example folder with my current understanding of best practices.
I updated the readMe file accordingly.
Since Arduino doesn't allow for relative library paths in the include statements, this must be modified to compile. :(
note: pass in a pointer to a function rather than putting a bunch of code in the button constructor (formerly in the sketch's setup function)!
Private variables
All variables were public, which I believe enables some bad coding practices.
If you want to know the state of the button use the function getState(). If you can see a reason to access the other variables involved please let me know and give an example.