Skip to content

Use best practices for determining the size of an array element #222

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

Closed
wants to merge 1 commit into from
Closed

Use best practices for determining the size of an array element #222

wants to merge 1 commit into from

Conversation

per1234
Copy link
Collaborator

@per1234 per1234 commented Jan 23, 2019

The previous code does not automatically adapt to changes in the type of meusInts.

Reference: arduino/reference-en@b0791f1

The previous code does not automatically adapt to changes in the type of myInts.
@robsoncouto
Copy link
Contributor

@per1234, I have thought about this one. I will use the English example here.

for (i = 0; i < (sizeof(myInts)/sizeof(myInts[0])); i++) {
  // do something with myInts[i]
}

It would be better to add the array atop the existing code for clarity

int myInts[10] // You can change the size and type of the array without modifying the code below. 
for (i = 0; i < (sizeof(myInts)/sizeof(myInts[0])); i++) {
  // do something with myInts[i]
}

But of course having an array called myInts of type float or char would be confusing, so I would recommend changing it to myNumbers, myValues or anything generic like that.

Tell me what you think, if you agree, this can be done in the reference-en repo first and then sync'd here.

@per1234
Copy link
Collaborator Author

per1234 commented Feb 8, 2019

Good idea for improving the example! I think this code could be a little confusing to a beginner and it's a very important fundamental of writing good for loop code to work with arrays.

After a look at that section of the page with "beginner's eyes", I found that the lack of the array definition in the code, combined with no mention of arrays in the description increased my confusion. I have implemented your suggestion in arduino/reference-en#538, with some changes of my own. Please let me know if you want me to make any changes.

@robsoncouto
Copy link
Contributor

Your implementation at arduino/reference-en#538 is just perfect :)
I will wait for the new issue here, and leave this PR open for the while.
This idea fits very well a repository of mine, will implement it there as well.
Thanks per.

@per1234
Copy link
Collaborator Author

per1234 commented Feb 28, 2019

@robsoncouto did you still want to merge this? If so, I'll fix the merge conflict. I think it's best for the translation repositories to mirror reference-en, other than that they are translated to a different language.

This was a bit of a weird one because I pushed directly to the reference-en repository, rather than submitting a pull request (at that time, I had gotten the OK to do that from someone at Arduino). However, after making the commit to reference-en, I found that the automated issue reports are only submitted to the translation repositories when a pull request is merged. I submitted pull requests to all the translation repositories, so that wasn't a big problem, but it means there's no issue report to reference to as there usually is. Instead, I just referenced the commit in reference-en: arduino/reference-en@b0791f1

@robsoncouto
Copy link
Contributor

Hi per!
No, I don't. I just left it open as a reminder. I will close this when I submit the pull request to translate arduino/reference-en#538 which is issue #289 in this repository, as I said.
I don't really understand what you mean about mirroring reference-en, though.

@per1234
Copy link
Collaborator Author

per1234 commented Mar 1, 2019

By "mirroring reference-en", I mean that every time there is a change to the reference-en repository, the equivalent change is made in each of the translation repositories.

@robsoncouto
Copy link
Contributor

Closed in favor of #341.

@robsoncouto robsoncouto closed this Mar 1, 2019
@per1234 per1234 deleted the calculate-element-size branch May 17, 2019 06:45
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.

2 participants