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

Code Cleanup | 123 == x => x == 123 etc #2766

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

benrr101
Copy link
Contributor

@benrr101 benrr101 commented Aug 9, 2024

Description: In the latest in my series of code cleanup PRs to remove Yoda conditions, this PR flips conditions where 0 == x to x == 0. This is also applied for >, >=, <, <=, and !=.

AI Analysis:
image

Testing: Again, if it builds and passes CI, I think it's safe.

@benrr101 benrr101 added the ➕ Code Health Issues/PRs that are targeted to source code quality improvements. label Aug 9, 2024
@saurabh500
Copy link
Contributor

Have you validated the AI analysis? Usually these come with an asterisk saying "AI content may be incorrect"

@saurabh500
Copy link
Contributor

Also @benrr101 please open a tracking issue for this work and describe the significance of doing this for tracking purposes.
I see references to Yoda conditions but the term hasn't been defined.
Please provide citations for the significance of these changes.
They may be safe, but I am not sure what would happen if we dont take these changes.
As of now I am seeing more problems with such large code hygeine PRs than the benefits that may come due to it.

@saurabh500
Copy link
Contributor

Please hold off on the merge of this PR till we drive #2714 and JSON work to conclusion.

@benrr101
Copy link
Contributor Author

benrr101 commented Aug 9, 2024

@saurabh500 the "ai analysis" is provided for your convenience. If you don't trust it, you are free to analyze all 104 changed files, but considering most people's eyes will glaze over after about 5 files, this analysis of the patch file is provided to give a summary of the changes and their safety.

Yoda conditions are a name for conditions whose expressions are reversed from their typical ordering. https://en.wikipedia.org/wiki/Yoda_conditions The reason for making these changes is to make the code more readable and supportable by changing the order to what is typical for both programming and the mental english translation ("x is greater than 0" is more natural than "0 is less than x").

If we do not take these changes, nothing happens, although I will be mentally cursing at the code each time I encounter one of these backwards conditions when I have to support it in the future. Considering the support that was given to making similar changes for null comparisons, I believe other engineers will be mentally cursing the code each time they encounter them, as well. Code hygiene is important and in the absence of anyone else feeling brave enough stand up for it, I'll gladly take up that torch.

@DavoudEshtehari DavoudEshtehari added this to the 6.0-preview2 milestone Sep 18, 2024
@cheenamalhotra
Copy link
Member

Moving to Backlog due to unresolved conflicts.

@cheenamalhotra cheenamalhotra removed this from the 6.0-preview2 milestone Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
➕ Code Health Issues/PRs that are targeted to source code quality improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants