Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Rule: defer-assignment #1215

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Rule: defer-assignment #1215

merged 1 commit into from
Oct 22, 2024

Conversation

anderseknert
Copy link
Member

Finally a new rule in the performance category! And this is quite a fun one. I was curious to see how many violations this would find in the Regal codebase, if any. It did find a few, and good ones to fix!

In order to keep it simple, this first implementation is perhaps a bit too cautious about what it considers deferrable. Check the implementation to see what exactly, but pretty much only assignments where the next line is a not an assignment, a potential loop, or something else that might make deferring the assignment undesirable.

Fixes #1147

Finally a new rule in the `performance` category! And this is quite
a fun one. I was curious to see how many violations this would find
in the Regal codebase, if any. It did find a few, and good ones to
fix!

In order to keep it simple, this first implementation is perhaps a
bit too cautious about what it considers deferrable. Check the
implementation to see what exactly, but pretty much only assignments
where the next line is a not an assignment, a potential loop, or
something else that *might* make deferring the assignment undesirable.

Fixes #1147

Signed-off-by: Anders Eknert <anders@styra.com>
Copy link
Member

@charlieegan3 charlieegan3 left a comment

Choose a reason for hiding this comment

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

This is a great rule 👏


not _ref_with_vars(rhs)

# for now, only simple var assignment counts.. later we can
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# for now, only simple var assignment counts.. later we can
# for now, only simple var assignment counts, later we can


Assignments are normally cheap, but certainly not always. If the right-hand side of an assignment is expensive,
deferring the assignment to where it's needed can save a considerable amount of time. Even for less expensive
assignments, code tends to be more readable when assignments are placed close to where they're used.
Copy link
Member

Choose a reason for hiding this comment

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

code tends to be more readable when assignments are placed close to where they're used.

I think this is the number one reason, cheap/expensive is the secondary reason for this rule since it does not always apply / is more complicated.

Something like this might work:

Placing assignments close to where they’re used improves code readability. Following this rule is also particularly important when the right-hand side of an assignment is expensive, such as making an HTTP call, as deferring the assignment can also save significant time.

Copy link
Member Author

Choose a reason for hiding this comment

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

This rule is in the performance category, which is why it's emphasized here. I think there are good examples of both really, and there are of course examples of both where location doesn't matter much.

@anderseknert anderseknert merged commit 877372b into main Oct 22, 2024
4 checks passed
@anderseknert anderseknert deleted the defer-assignment branch October 22, 2024 11:21
charlieegan3 pushed a commit to charlieegan3/regal that referenced this pull request Jan 6, 2025
Finally a new rule in the `performance` category! And this is quite
a fun one. I was curious to see how many violations this would find
in the Regal codebase, if any. It did find a few, and good ones to
fix!

In order to keep it simple, this first implementation is perhaps a
bit too cautious about what it considers deferrable. Check the
implementation to see what exactly, but pretty much only assignments
where the next line is a not an assignment, a potential loop, or
something else that *might* make deferring the assignment undesirable.

Fixes StyraInc#1147

Signed-off-by: Anders Eknert <anders@styra.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule: defer-assignment
2 participants