Skip to content

Conversation

yezz123
Copy link
Contributor

@yezz123 yezz123 commented Aug 28, 2021

  • Merges multiply nested if conditions into one in this Function SQLModel.__setattr__

Reading deeply nested code is confusing since you have to keep track of which conditions relate to which levels. We, therefore, strive to reduce nesting where possible, and the situation where two if conditions can be combined using and is an easy win.

  • Replaces conditional assignment to a variable with an if expression in this Function SQLModel.from_orm

Once the change is made there's only one statement where x is defined as opposed to having to read two statements plus the if-else lines.

  • Removes unnecessarily verbose boolean comparisons in this Function SQLModel._calculate_keys

It is unnecessary to compare boolean values to True or False in the test of an if condition. Removing these unnecessary checks makes the code slightly shorter and easier to parse.

  • Hoist repeated code outside conditional statement and Swap if/else to remove empty if body in this Function GUID.process_result_value

By taking the assignment outside of the conditional we have removed a duplicate line of code, and made it clearer what the conditional is actually controlling.

Copy link
Member

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

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

Thanks for the interest! I have some comments, all this is mainly about taste. 🤷 😅

@codecov
Copy link

codecov bot commented Aug 27, 2022

Codecov Report

Merging #53 (0c054ab) into main (7d3bf70) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #53      +/-   ##
==========================================
- Coverage   97.58%   97.58%   -0.01%     
==========================================
  Files         182      182              
  Lines        6054     6053       -1     
==========================================
- Hits         5908     5907       -1     
  Misses        146      146              
Impacted Files Coverage Δ
sqlmodel/main.py 83.58% <100.00%> (-0.05%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 2a91647 at: https://630a8dd6fbefa94700c71038--sqlmodel.netlify.app

@yezz123
Copy link
Contributor Author

yezz123 commented Aug 27, 2022

Hello @tiangolo all the changes you asked for are done 🍰

@yezz123 yezz123 requested a review from tiangolo August 27, 2022 21:46
@github-actions
Copy link
Contributor

📝 Docs preview for commit 0c054ab at: https://630a90e8801d6f48588c8617--sqlmodel.netlify.app

@tiangolo tiangolo changed the title Chore: Merges multiply nested if conditions into one and Replaces conditional assignment ♻ Refactor internal statements to simplify code Aug 27, 2022
@tiangolo
Copy link
Member

Awesome, thanks @yezz123! 🍰

@tiangolo tiangolo merged commit 6216409 into fastapi:main Aug 27, 2022
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