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

remove duplicate conditions. add missing ; #918

Conversation

a-tarasyuk
Copy link
Contributor

No description provided.

const macAccessibilityProps: MacComponentAccessibilityProps = accessibilityProps as any;
if (this.props.tabIndex !== -1) {

if (this.props.tabIndex && this.props.tabIndex >= 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't correct. If tabIndex is 0, this condition will fail and the code below is not run
If tab index is undefined and onPress is defined, the code below should be run but is not

The conditional that was previously above is needed on this block, so I don't think it really optimizes anything by moving it

@a-tarasyuk a-tarasyuk force-pushed the feature/remove-duplicate-conditions branch from 149bd76 to f02798a Compare November 15, 2018 16:53
const macAccessibilityProps: MacComponentAccessibilityProps = accessibilityProps as any;
if (this.props.tabIndex !== -1) {

if (this.props.tabIndex !== undefined && this.props.tabIndex >= 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still a change in behaviour. If tabIndex is undefined and onPress is defined, the below code should be run.
You're going to end up with the same conditional that was above

@a-tarasyuk
Copy link
Contributor Author

Oh, ok. For the first look, it is a little bit confused :). Have closed PR., In my opinion, this condition is too long and would be better to make it simpler or make a method with a readable name which describes the purpose of the condition. Seems case when tabIndex less than -1 causes the error...

@a-tarasyuk a-tarasyuk closed this Nov 16, 2018
@a-tarasyuk a-tarasyuk deleted the feature/remove-duplicate-conditions branch November 16, 2018 12:12
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.

None yet

2 participants